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

[Kiliandeca]

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

For regular nodes at least one of max_surge or max_unavailable must be specified but for Spot nodes theses properties are not supported and if you try to set them you'll get the followings errors:

│ Agent Pool Name: "foobar"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with response: {
│   "code": "InvalidParameter",
│   "details": null,
│   "message": "The value of parameter agentPoolProfile.upgradeSettings.maxUnavailable is invalid. Error details: Spot pools can't set maxUnavailable.",
│   "subcode": "",
│   "target": "agentPoolProfile.upgradeSettings.maxUnavailable"
│  }
│ 

│ Agent Pool Name: "foobar"): performing CreateOrUpdate: unexpected status 400 (400 Bad Request) with response: {
│   "code": "InvalidParameter",
│   "details": null,
│   "message": "The value of parameter agentPoolProfile.upgradeSettings.maxSurge is invalid. Error details: Spot pools can't set max surge.",
│   "subcode": "",
│   "target": "agentPoolProfile.upgradeSettings.maxSurge"
│  }
│ 

The others upgrade_settings properties are supported for Spot nodes so the provider should support that. Previously I used this workaround #26568 (comment) of setting max_surge to an empty string but it no longer works since #30563 broke it.

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)

I created a new subscription to run the tests I added but couldn't get it to works. I tried multiple region for ARM_TEST_LOCATION like eastus, westus and westeurope but I got the same error about the VM size not available on all of them:

 "message": "The VM size of Standard_DS2_v2 is not allowed in your subscription in location 'eastus'. The available VM sizes are

This VM size is used in a lot of tests and I didn't want to mess up things unrelated to this PR so can you please guide me on what region I should use to run the tests or tell me if I need to do something special on my subcription. Are all the tests borken for everyone because this VM size is no longer available anywhere?

Update: I could run the tests by changing the VM size of all of them to make sure they works, I will create a proper issue for that as it's the this PR goal to fix that

TF_ACC=true go test -v -run "TestAccKubernetesClusterNodePool_spotWithMaxSurge|TestAccKubernetesClusterNodePool_spotWithMaxUnavailable|TestAccKubernetesClusterNodePool_upgradeSettingsWithoutMaxSurgeOrMaxUnavailable|TestAccKubernetesClusterNodePool_spotWithOtherUpgradeSettings|TestAccKubernetesClusterNodePool_upgradeSettingsMaxSurge" ./internal/services/containers/ -timeout=30m

Change Log

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

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

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 #29318

AI Assistance Disclosure

• AI Assisted - This contribution was made by, or with the assistance of, AI/LLMs

I used an LLM for code suggestion and autocomplete in my IDE

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.

[Kiliandeca]

I managed to get the relevants tests running, could someone have a look at this PR please, this is preventing us (and everyone that used the same workaround) from upgrading.

[Kiliandeca]

Hello @catriona-m, sorry for tagging you directly but since you were the reviewer of the PR #30563 that introduced the regression I thought this might be in your scope. Well, I'm not sure this should be truly classified as a regression, as it was more of a workaround. Anyway, any help would be greatly appreciated 🙏

For others facing the same issue, you can vote on the PR with a 👍 reaction so it can be prioritised.

[catriona-m]

Thanks for fixing this @Kiliandeca - I left a suggestion inline about moving the checks to a CustomizeDiff, but otherwise this is looking good. Thanks!

[Kiliandeca]

Thanks for reviewing this PR, I made the changes you requested and also ran the tests again. Let me know if you need anything else.

[catriona-m]

Thanks for fixing this up @Kiliandeca - just left one minor comment, but otherwise I think this is looking good. Thanks!

[catriona-m]

Thanks for fixing this up @Kiliandeca - it looks like there are a couple of linting errors that are needing addressed, but this is nearly there I think. Thanks!

[Kiliandeca]

Sorry about the linter errors, it should be good now.

[catriona-m]

Thanks for fixing this up @Kiliandeca - LGTM!