Review request for Microsoft.ContainerService/aks to add version stable/2024-07-01

[FumingZhang]

ARM (Control Plane) API Specification Update Pull Request

Tip

Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

Purpose of this PR

What's the purpose of this PR? Check the specific option that applies. This is mandatory!

• New resource provider.
• New API version for an existing resource provider. (If API spec is not defined in TypeSpec, the PR should have been created in adherence to OpenAPI specs PR creation guidance).
• Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
• Update existing version to fix OpenAPI spec quality issues in S360.
• Convert existing OpenAPI spec to TypeSpec spec (do not combine this with implementing changes for a new API version).
• Other, please clarify:
  • edit this with your clarification

Due diligence checklist

To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:

• I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications.
• I have reviewed following Resource Provider guidelines, including
  ARM resource provider contract and
  REST guidelines (estimated time: 4 hours).
  I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.

Additional information

Viewing API changes

For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.

Suppressing failures

If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.

Getting help

• First, please carefully read through this PR description, from top to bottom. Please fill out the Purpose of this PR and Due diligence checklist.
• If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
• To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
• For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
  and https://aka.ms/ci-fix.
• For help with ARM review (PR workflow diagram Step 2), see https://aka.ms/azsdk/pr-arm-review.
• If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run.
  This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
• If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

[openapi-pipeline-app]

Next Steps to Merge

✅ All automated merging requirements have been met! To get your PR merged, see aka.ms/azsdk/specreview/merge.

[openapi-pipeline-app]

Generated ApiView

Language	Package Name	ApiView Link
Go	sdk/resourcemanager/containerservice/armcontainerservice	https://apiview.dev/Assemblies/Review/2f19cca64456411e9c342082a432418f?revisionId=43624f9a484143b0a90860831478e120
JavaScript	@azure/arm-containerservice	https://apiview.dev/Assemblies/Review/dabf043916d14dc38e0c2d456d89f55a?revisionId=69257a8adc3845608ecdae500bad23f2
Swagger	Microsoft.ContainerService	https://apiview.dev/Assemblies/Review/71d5cd3d2d1c46b59eabc692899ea0b4?revisionId=d4c314e3b6a746989fedd33e6592495c

[FumingZhang]

For failed CI Swagger Avocado, all detected API paths are deprecated and not included in the previous stable API version 2024-05-01, see comment.

[mentat9]

"AgentPoolSecurityProfile": {

ARM recommends enum instead of boolean for many reasons. Consider something like this:

trustedPlatformModule: { None, Virtual }
kernelModeBinarySigning: { Optional, Required }

... or something similar/better you come up with.

---

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:6248 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

[mentat9]

      "description": "Arm resource id of the machine. It can be used to GET underlying VM Instance",

Use "Azure" instead of "Arm" in client-visible names and messages.

---

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7656 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

[mentat9]

    "resourceId": {

Looks like this is always a VM resource ID, so maybe vmResourceId or virtualMachineResourceId>

---

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7655 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

[mentat9]

      "format": "arm-id",

You can also specify resource types using x-ms-arm-id-details extension. Ref: https://github.com/Azure/autorest/blob/main/docs/extensions/readme.md#x-ms-arm-id-details.

---

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7657 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

[lilypan26]

"AgentPoolSecurityProfile": {

ARM recommends enum instead of boolean for many reasons. Consider something like this:

trustedPlatformModule: { None, Virtual } kernelModeBinarySigning: { Optional, Required }

... or something similar/better you come up with.

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:6248 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

A similar comment was left when added to the preview API: #27431 (comment). We decided it makes most sense to keep the fields as booleans.

[mentat9]

"AgentPoolSecurityProfile": {

ARM recommends enum instead of boolean for many reasons. Consider something like this:
trustedPlatformModule: { None, Virtual } kernelModeBinarySigning: { Optional, Required }
... or something similar/better you come up with.
Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:6248 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

A similar comment was left when added to the preview API: #27431 (comment). We decided it makes most sense to keep the fields as booleans.

@lilypan26 - OK. Could you explain the reasoning? I find many, many arguments in favor of enums over booleans, and not a single argument in favor of booleans over enums.

I'll give some here:

• Enums support clear, helpfully-named values and separate descriptions for member values
• Enums support two values, or more than two values seamlessly
• Enums better support selecting among valid options across the whole API instead of partitioning the selections into a bitvector. Often some sets of values of the bitvector aren't even valid.
• Over time as APIs evolve to support new functionality APIs that use enums tend to be more adaptable to new scenarios.
• Enums are easier for clients to read, learn, understand, and use, especially as the number of EnableXXX booleans in an API increases.
• The ARM team is working hard to help RP APIs to evolve better consistency across the entire Azure API surface. RP teams can help us make progress by adopting common guidelines of this kind.

Those are some of the reasons we give this feedback. I won't block ARM signoff on this, but I hope you will incorporate some of this thinking into future API designs.
.
It looks like I didn't set the label after providing feedback, so doing it now. There are a few other unaddressed feedback comments above.

[matthchr]

"AgentPoolSecurityProfile": {

ARM recommends enum instead of boolean for many reasons. Consider something like this:
trustedPlatformModule: { None, Virtual } kernelModeBinarySigning: { Optional, Required }
... or something similar/better you come up with.
Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:6248 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

A similar comment was left when added to the preview API: #27431 (comment). We decided it makes most sense to keep the fields as booleans.

@lilypan26 - OK. Could you explain the reasoning? I find many, many arguments in favor of enums over booleans, and not a single argument in favor of booleans over enums.

@mentat9 - we generally agree, if you look at the comment @lilypan26 linked the reasoning was:

These two new boolean fields are made to match the fields exposed by VMSS. Those fields are shown here.

Since these fields are just proxying VMSS fields, which themselves are booleans, it doesn't make sense to use an enum.

Maybe we should be more willing to diverge from the underlying compute API here (or maybe they should be more willing to make fields such as these enums), but there's a tension between us forging our own path and looking different, versus us doing what the underlying compute API does and looking the same, and in this case we erred on the side of consistency with the compute API because the intent was to exactly proxy those features. Setting these flags on your AgentPool (which is backed by a single VMSS) maps exactly to setting those same fields on the VMSS.

[skuchipudi295]

    "resourceId": {

Looks like this is always a VM resource ID, so maybe vmResourceId or virtualMachineResourceId>

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7655 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

so arm-id + x-ms-arm-id-details already says what the format of this ID is without needing to change the property name, plus it's already on an object called Machine, so it should be clear already that it is the resourceId of the machine. So, I wouldn't want to change the field name here.

[skuchipudi295]

      "format": "arm-id",

You can also specify resource types using x-ms-arm-id-details extension. Ref: https://github.com/Azure/autorest/blob/main/docs/extensions/readme.md#x-ms-arm-id-details.

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7657 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

addressed this one.

[skuchipudi295]

      "description": "Arm resource id of the machine. It can be used to GET underlying VM Instance",

Use "Azure" instead of "Arm" in client-visible names and messages.

Refers to: specification/containerservice/resource-manager/Microsoft.ContainerService/aks/stable/2024-07-01/managedClusters.json:7656 in 70de27d. [](commit_id = 70de27d, deletion_comment = False)

addressed this one.

[FumingZhang]

@msyyc, could you please help approve the breaking change of Python SDK?