`azurerm_kubernetes_cluster_node_pool` - support for the `undrainable_node_behavior` and `max_unavailable` properties by ms-henglu · Pull Request #30563 · hashicorp/terraform-provider-azurerm

ms-henglu · 2025-09-05T05:50:56Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR adds features in #30515 that are reverted in #30554

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevant documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_resource - support for the thing1 property [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #0000

Rollback Plan

If a change needs to be reverted, we will publish an updated version of the provider.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

…- support for the `undrainable_node_behavior` and `max_unavailable` properties

ms-henglu · 2025-09-16T01:43:11Z

magodo

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

internal/services/containers/kubernetes_cluster_node_pool_resource.go

magodo · 2025-09-16T03:43:31Z

website/docs/r/kubernetes_cluster_node_pool.html.markdown

@@ -312,6 +312,12 @@ A `upgrade_settings` block supports the following:

 * `max_surge` - (Required) The maximum number or percentage of nodes which will be added to the Node Pool size during an upgrade.


This shall be Optional? Also, this changes the semantic a bit as I assume it is still required when max_unavailable is not specified?

Yes, it's Optional.

website/docs/r/kubernetes_cluster.html.markdown

ms-henglu · 2025-09-16T04:10:44Z

Thanks @magodo for the code review. I've updated the doc to change it to "Optional"

alexp-openai · 2025-09-25T19:39:18Z

Any updates on merging this PR? Look forward to use this.

magodo · 2025-09-26T01:26:02Z

Thanks @ms-henglu! This LGTM now!

alexp-openai · 2025-10-02T18:14:09Z

gentle bump on merging/shipping it :) Looks like PR is still stuck in "requested changes" state.

catriona-m

Thanks for giving me more info @ms-henglu . Based on what you've said, I think we should use ExactlyOneOf in the schema to ensure either max_surge or max_unavailable is set and when max_unavailable is set, we should set max_surge to 0 explicitly in the request.

Since the upgrade_settings block is always returned with a default value, we should make this Required in 5.0, however this could be done in a separate pr.

- Add ExactlyOneOf validation for max_surge and max_unavailable fields - Remove Default value "0" from max_unavailable field - Expand function sets opposite field to "0" when one is specified - Flatten function filters out "0" and "0%" values for both fields - Update test to remove explicit max_surge = "0" configuration

ms-henglu · 2025-11-03T02:40:01Z

Thanks for giving me more info @ms-henglu . Based on what you've said, I think we should use ExactlyOneOf in the schema to ensure either max_surge or max_unavailable is set and when max_unavailable is set, we should set max_surge to 0 explicitly in the request.

Got it, I've updated this PR as suggested.

Since the upgrade_settings block is always returned with a default value, we should make this Required in 5.0, however this could be done in a separate pr.

I'll open another PR and share it shortly.

ms-henglu · 2025-11-03T03:21:56Z

catriona-m

Thanks for making the changes @ms-henglu, I left a couple more comments inline but this is nearly good to go. Thanks!

catriona-m · 2025-11-04T16:21:09Z

website/docs/r/kubernetes_cluster_node_pool.html.markdown

+* `max_surge` - (Optional) The maximum number or percentage of nodes which will be added to the Node Pool size during an upgrade. Exactly one of `max_surge` or `max_unavailable` must be specified.
+
+* `max_unavailable` - (Optional) The maximum number or percentage of nodes which can be unavailable during the upgrade. Exactly one of `max_surge` or `max_unavailable` must be specified.


should we remove Exactly one of max_surgeormax_unavailable must be specified. from the end of these and add it as a note below instead?

catriona-m · 2025-11-04T16:21:55Z

internal/services/containers/kubernetes_cluster_node_pool_resource.go

 				return old != 0 && new == 0
 			}),
+
+			func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error {


could we remove this now that only one of max_surge and max_unavailable is allowed by the schema?

- Remove redundant CustomizeDiff validation for max_surge/max_unavailable since ExactlyOneOf constraint in schema already enforces this - Update documentation to use note block instead of repeating constraint in field descriptions for cleaner documentation format

ms-henglu · 2025-11-05T01:50:21Z

Thanks @catriona-m for the code review. I've updated this PR, please check. Thanks!

catriona-m

Thanks @ms-henglu, just one last note on the tests, apologies for missing this previously.

catriona-m · 2025-11-05T13:43:18Z

internal/services/containers/kubernetes_cluster_node_pool_resource_test.go

+		},
+		data.ImportStep(),
+		{
+			Config: r.upgradeSettingsMaxUnavailable(data, `50%`, 15, 8),


could we add an additional step where we remove all the optional properties in upgrade settings?

I added a step to remove the optional properties, but some of them can't be removed because

terraform-provider-azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go

Lines 84 to 88 in ef608da

// The behaviour of the API requires this, but this could be removed when https://github.com/Azure/azure-rest-api-specs/issues/27373 has been addressed

pluginsdk.ForceNewIfChange("upgrade_settings.0.drain_timeout_in_minutes", func(ctx context.Context, old, new, meta interface{}) bool {

return old != 0 && new == 0

}),

),

catriona-m

Thanks @ms-henglu I left a couple more comments for you to consider but hopefully this should be nearly there. Thanks!

catriona-m · 2025-11-06T17:32:17Z

internal/services/containers/kubernetes_cluster_node_pool_resource.go

+	}
+}
+
+func upgradeSettingsSchemaClusterDefaultNodePool() *pluginsdk.Schema {


should we move this to kubernetes_nodepool.go since this is no longer used in this file?

catriona-m · 2025-11-06T17:35:42Z

internal/services/containers/kubernetes_cluster_node_pool_resource.go

 			pluginsdk.ForceNewIfChange("upgrade_settings.0.drain_timeout_in_minutes", func(ctx context.Context, old, new, meta interface{}) bool {
 				return old != 0 && new == 0
 			}),
+			pluginsdk.ForceNewIfChange("upgrade_settings.0.undrainable_node_behavior", func(ctx context.Context, old, new, meta interface{}) bool {


can we add a note in the docs to explain that once added, this property cannot be removed? Also, could you check if this is also applicable to this property in kubernetes_cluster?

Sure. I also checked the kubernetes_cluster, it's also applicable, I made the same changes.

…grade_settings schema

catriona-m

Thanks for fixing this up @ms-henglu, LGTM!

comments addressed

github-actions · 2025-12-08T02:25:38Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ms-henglu added 4 commits September 1, 2025 11:17

azurerm_kubernetes_cluster, azurerm_kubernetes_cluster_node_pool …

81ba238

…- support for the `undrainable_node_behavior` and `max_unavailable` properties

fix code style and add note in doc

573e9ec

fix drift

dbd482b

fix doc

98d963d

github-actions bot added documentation size/M labels Sep 5, 2025

ms-henglu mentioned this pull request Sep 9, 2025

azurerm_kubernetes_cluster, azurerm_kubernetes_cluster_node_pool - support for the undrainable_node_behavior and max_unavailable properties #30515

Merged

14 tasks

ms-henglu added 2 commits September 10, 2025 11:24

Merge branch 'main' into branch-250819-aks-upgrade-settings

91d628b

add conflicts with and computed

07e62b5

github-actions bot added size/L and removed size/M labels Sep 10, 2025

rcskosir added the service/kubernetes-cluster label Sep 10, 2025

github-actions bot added the enhancement label Sep 10, 2025

fix tests

63d870d

ms-henglu marked this pull request as ready for review September 11, 2025 04:26

ms-henglu requested review from a team, WodansSon and magodo as code owners September 11, 2025 04:26

ms-henglu added 2 commits September 15, 2025 11:09

Merge branch 'main' into branch-250819-aks-upgrade-settings

871d336

fix test

e14d577

magodo self-assigned this Sep 16, 2025

magodo previously requested changes Sep 16, 2025

View reviewed changes

github-actions bot added the waiting-response label Sep 16, 2025

fix doc

3dab79d

github-actions bot removed the waiting-response label Sep 16, 2025

github-actions bot removed the waiting-response label Oct 24, 2025

catriona-m requested changes Oct 31, 2025

View reviewed changes

github-actions bot added the waiting-response label Oct 31, 2025

github-actions bot removed the waiting-response label Nov 3, 2025

catriona-m requested changes Nov 4, 2025

View reviewed changes

github-actions bot added the waiting-response label Nov 4, 2025

github-actions bot removed the waiting-response label Nov 5, 2025

catriona-m requested changes Nov 5, 2025

View reviewed changes

github-actions bot added the waiting-response label Nov 5, 2025

add a test step to remove the optional

4a904f2

github-actions bot removed the waiting-response label Nov 6, 2025

catriona-m requested changes Nov 6, 2025

View reviewed changes

github-actions bot added the waiting-response label Nov 6, 2025

ENHANCEMENT: add ForceNew for undrainable_node_behavior & relocate up…

3b041bb

…grade_settings schema

github-actions bot added size/XL and removed waiting-response size/L labels Nov 7, 2025

ms-henglu requested a review from catriona-m November 7, 2025 02:00

catriona-m approved these changes Nov 7, 2025

View reviewed changes

catriona-m merged commit eab30fa into hashicorp:main Nov 7, 2025
35 checks passed

github-actions bot added this to the v4.53.0 milestone Nov 7, 2025

Kiliandeca mentioned this pull request Nov 18, 2025

azurerm_kubernetes_cluster_node_pool: max_surge and max_unavailable should not be required for spot nodes pools #31129

Open

15 tasks

aydosman mentioned this pull request Dec 3, 2025

Feature Request: Support max_unavailable in upgrade_settings for AKS Node Pools #30381

Closed

1 task

		@@ -312,6 +312,12 @@ A `upgrade_settings` block supports the following:

		* `max_surge` - (Required) The maximum number or percentage of nodes which will be added to the Node Pool size during an upgrade.

		* `max_surge` - (Optional) The maximum number or percentage of nodes which will be added to the Node Pool size during an upgrade. Exactly one of `max_surge` or `max_unavailable` must be specified.

		* `max_unavailable` - (Optional) The maximum number or percentage of nodes which can be unavailable during the upgrade. Exactly one of `max_surge` or `max_unavailable` must be specified.

	// The behaviour of the API requires this, but this could be removed when https://github.com/Azure/azure-rest-api-specs/issues/27373 has been addressed
	pluginsdk.ForceNewIfChange("upgrade_settings.0.drain_timeout_in_minutes", func(ctx context.Context, old, new, meta interface{}) bool {
	return old != 0 && new == 0
	}),
	),

Conversation

ms-henglu commented Sep 5, 2025 • edited by rcskosir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Rollback Plan

Changes to Security Controls

Uh oh!

ms-henglu commented Sep 16, 2025

Uh oh!

magodo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ms-henglu commented Sep 16, 2025

Uh oh!

alexp-openai commented Sep 25, 2025

Uh oh!

magodo commented Sep 26, 2025

Uh oh!

alexp-openai commented Oct 2, 2025

Uh oh!

catriona-m left a comment

Choose a reason for hiding this comment

Uh oh!

ms-henglu commented Nov 3, 2025

Uh oh!

ms-henglu commented Nov 3, 2025

Uh oh!

catriona-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ms-henglu commented Nov 5, 2025

Uh oh!

catriona-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catriona-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catriona-m left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

ms-henglu commented Sep 5, 2025 •

edited by rcskosir

Loading