containerservice: adding support for Custom Headers #18232
Conversation
|
Hi, @tombuildsstuff Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vscswagger@microsoft.com |
|
[Call for Action] To better understand Azure service dev/test scenario, and support Azure service developer better on Swagger and REST API related tests in early phase, please help to fill in with this survey https://aka.ms/SurveyForEarlyPhase. It will take 5 to 10 minutes. If you already complete survey, please neglect this comment. Thanks. |
Swagger Validation Report
|
| Rule | Message |
|---|---|
|
The order of parameter 'parameters' was changed. New: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L474:9 Old: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L474:9 |
|
The optional parameter 'AKSHTTPCustomFeatures' was added in the new version. New: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L474:9 |
️⚠️LintDiff: 0 Warnings warning [Detail]
- Linted configuring files (Based on source branch, openapi-validator v1.12.1 , classic-openapi-validator v1.2.2 )
- Linted configuring files (Based on target branch, openapi-validator v1.12.1 , classic-openapi-validator v1.2.2 )
Only 30 items are listed, please refer to log for more details.
| Rule | Message |
|---|---|
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2820 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2913 |
|
Inline/anonymous models must not be used, instead define a schema with a model name in the 'definitions' section and refer to it. This allows operations to share the models. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L4087 |
|
The response of operation:'ManagedClusters_Get' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L428 |
|
The response of operation:'ManagedClusters_CreateOrUpdate' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L468 |
|
The response of operation:'ManagedClusters_UpdateTags' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L587 |
|
The response of operation:'AgentPools_Get' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L924 |
|
The response of operation:'AgentPools_CreateOrUpdate' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L971 |
|
The response of operation:'PrivateEndpointConnections_Get' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L1492 |
|
The response of operation:'PrivateEndpointConnections_Update' is defined without 'systemData'. Consider adding the systemData to the response. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L1540 |
|
The response is defined but without a default error response implementation.Consider adding it.' Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L1196 |
|
The delete operation is defined without a 200 or 204 error response implementation,please add it.' Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L657 |
|
The delete operation is defined without a 200 or 204 error response implementation,please add it.' Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L1103 |
|
The response schema of operations API '/providers/Microsoft.ContainerService/operations' does not match the ARM specification. Please standardize the schema. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L37 |
|
The private endpoint model 'PrivateLinkResourcesListResult' schema does not conform to the common type definition. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L4889 |
|
The schema 'OperationListResult' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2225 |
|
The schema 'OperationValue' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2238 |
|
The schema 'OperationValueDisplay' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2258 |
|
The schema 'Resource' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2283 |
|
The schema 'SubResource' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2322 |
|
The schema 'TagsObject' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2343 |
|
The schema 'ManagedClusterServicePrincipalProfile' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2562 |
|
The schema 'ContainerServiceMasterProfile' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2578 |
|
The schema 'ManagedClusterAgentPoolProfileProperties' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2632 |
|
The schema '1' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2820 |
|
The schema 'AgentPoolListResult' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2881 |
|
The schema 'AgentPoolUpgradeSettings' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2898 |
|
The schema '1' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2913 |
|
The schema 'ManagedClusterWindowsProfile' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2925 |
|
The schema 'ContainerServiceLinuxProfile' is considered an object but without a 'type:object', please add the missing 'type:object'. Location: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L2991 |
️❌Avocado: 1 Errors, 0 Warnings failed [Detail]
| Rule | Message |
|---|---|
|
The example JSON file is not referenced from the swagger file. readme: specification/containerservice/resource-manager/readme.md json: stable/2022-01-01/examples/ManagedClustersCreate_CustomHeader.json |
️️✔️~[Staging] ApiReadinessCheck succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️Cross-Version Breaking Changes succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️SDK Track2 Validation succeeded [Detail] [Expand]
Validation passes for SDKTrack2Validation
- The following tags are being changed in this PR
️️✔️PrettierCheck succeeded [Detail] [Expand]
Validation passes for PrettierCheck.
️❌SpellCheck: 1 Errors, 0 Warnings failed [Detail]
| Rule | Message |
|---|---|
|
Unknown word (AKSHTTP), please fix the error or add words to ./custom-words.txt path: Microsoft.ContainerService/stable/2022-01-01/managedClusters.json#L5665:16 |
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
ghost
added
the
customer-reported
|
Thank you for your contribution tombuildsstuff! We will review the pull request and get back to you soon. |
Swagger Generation Artifacts
|
|
Hi @tombuildsstuff, Your PR has some issues. Please fix the CI sequentially by following the order of
|
|
NewApiVersionRequired reason: |
|
Hi @tombuildsstuff, one or multiple breaking change(s) is detected in your PR. Please check out the breaking change(s), and provide business justification in the PR comment and @ PR assignee why you must have these change(s), and how external customer impact can be mitigated. Please ensure to follow breaking change policy to request breaking change review and approval before proceeding swagger PR review. |
|
@tombuildsstuff - thanks for the energy on this topic (and for the PR). I'll be honest: We don't love the way we're using headers to configure feature enablement right now. It has a lot of flaws, not the least of which as you've pointed out is that it's not "officially documented" in the REST API. It also doesn't work with ARM templates, you can't always easily see what feature's you turned on after the fact, and so on (there are a lot of flaws). I'm not sure if you've noticed but we've recently starting publishing
Unfortunately that means we really would like to be moving in the opposite direction here: Less headers, less support (and use) of headers, until we can eventually stop supporting them altogether (that's probably a ways off but it's the goal). Adding the headers explicitly to the API doesn't solve any of their other problems and also commits us to supporting the pattern for longer than we'd like. |
|
@matthchr I understand the desire to move away from this - but as these are supported today they should be documented in the Swagger? Future API versions can opt to remove this field once it's no longer used, but as this API Version supports this field it should be documented, as it's optional and isn't returned from the API - removing this isn't a breaking change. cc @JeffreyRichter since I've seen this a few times and I'm wondering if there's some guidance missing from the ARM docs here? |
|
I don't have enough context to comment on this specific PR but I can talk to Azure's policies. If a feature is being "previewed" then it should be in a preview swagger. The whole purpose of a preview swagger is to try out API design and then to respond to feedback which MAY cause breaking changes. Of course, each new preview swagger must be backward compatible with the last stable swagger. If a service really, really needs to make a breaking change, we have a formal policy of notifying customers of the breaking change and keeping them running as-is on the stable swaggers for at least 3 years. During this time, we hope that customers will modify their code to the api-version that is breaking. If they do not, then their app may stop working entirely (after the 3 years have expired). |
As you have pointed out, them being in the Swagger implies more support than they really have. They definitely work (and are used) today. We are not planning to break them. On the other hand if the header mechanism is pulled into the Swagger then the whole mechanism is subject to the 3 year deprecation policy @JeffreyRichter alluded to. This is true even if there are no headers that actually do anything since we've committed to supporting the mechanism. AFAIK today headers are used to enable the following features:
With the exception of item 1 in that list, every one of these already has a timeboxed lifecycle of significantly less than 3 years because the Kubernetes versions it applies to are going out of support. We aren't planning on adding any more headers to this list going forward. We are discussing what to do about the remaining item (GPU VHDs) internally and I don't have an answer for if/how that will be exposed in the future. @alexeldeib may have more details. My 2c is that pulling the header mechanism into the Swagger and making it an obviously official part of the API with all of the support/lifecycle implications that comes with is less desirable than just letting these headers naturally age out and moving new features through the |
|
Here is an MS internal link to the official Azure Retirements Document. See the section on "Addition to the policy for Non Microsoft Components". I believe this scenario falls into this category of retirement. |
|
Hi, @tombuildsstuff. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
|
Hi, @tombuildsstuff. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
|
Hi, @tombuildsstuff. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove |
|
@tombuildsstuff , this PR is pending for your action to move on. Either get breaking change approval or revert those changes that caused breaking. Another action is to fix CI check failures. |
|
@raych1 this PR is waiting on the SDK/ARM Team to decide how to proceed: #18232 (comment) If the API no longer supports these fields as of today then that's fine and we can close this PR, since this PR is no longer up to date - but if the API still accepts these values then the Swagger should be updated to account for this, even if the intention is to deprecate/remove support for this in a future API Version (which can be done when there's no remaining features using this method). |
As of today all 4 possible usages for these headers are deprecated. No new usages are being added. It is true that existing APIs will continue accepting the headers for the k8s versions listed above until they go out of support, but given their deprecated status we believe it doesn't make sense to add them to the @tombuildsstuff, thanks for this PR. I'm going to close it given the above status. We really appreciate the conversation that it drove here as well as internally. Please feel free to respond if you strongly disagree and we can continue the discussion. Note that hashicorp/terraform-provider-azurerm#6793 lists a bunch of headers for various features (Gen2 Vms, Ephemeral OS, containerd, Ubuntu 18.04). None of the headers for those features are supported anymore anyway (they all don't do anything) |
@matthchr Using GPU VHD images Will these images be discontinued? |
|
@matthchr - existing APIs still support these, even if deprecated, so they are valid, people are using them, and should be represented in swagger |
we're focusing on optimizing the vanilla images for latency. the behavior is very nearly identical today. the main other difference is the nvidia device plugin. that is relatively easy to deploy as an end-user, or if there's demand we can expose that via normal APIs separately. for bonus points: with an expanding range of skus, we need to pivot driver version based on GPU model, which defeats the purpose of the preinstalled drivers, and actually added latency in some cases :/ |
|
@alexeldeib |
Fixes Azure/AKS#2757
Unblocks hashicorp/terraform-provider-azurerm#6793
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Add a changelog entry for this PR by answering the following questions:
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Otherwise your PR may be subject to ARM review requirements. Complete the following:
Check this box if any of the following apply to the PR so that label "WaitForARMFeedback" will be added automatically to begin ARM API Review. Failure to comply may result in delays to the manifest.
-[ ] To review changes efficiently, ensure you are using OpenAPIHub to initialize the PR for adding a new version. More details, refer to the wiki.
Ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If any of the following scenarios apply to the PR, request approval from the Breaking Change Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.