Implementing agentpool API #5352
Conversation
Automation for azure-sdk-for-pythonA PR has been created for you: |
Automation for azure-sdk-for-jsA PR has been created for you: |
Automation for azure-sdk-for-rubyA PR has been created for you: |
Automation for azure-sdk-for-nodeA PR has been created for you: |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Automation for azure-sdk-for-goA PR has been created for you: |
|
Can one of the admins verify this patch? |
jhendrixMSFT
added
the
WaitForARMFeedback
|
This is adding a new resource type.. please schedule a meeting with ARM team. |
|
@sanjaiganesh, we have done the ARM review meeting for agent pool last year. @kkmsft , @yangl900 can confirm. We are only adding the API into swagger now because the implementation is deployed now. |
...e-manager/Microsoft.ContainerService/stable/2019-02-01/examples/AgentPoolsCreate_Update.json
Show resolved
Hide resolved
| "description": "The name of the managed cluster resource." | ||
| } | ||
| ], | ||
| "responses": { |
There was a problem hiding this comment.
Please add "default" response to all the API responses. This would capture the error state and the schema must match the error schema as described in ARM RPC. See Batch RP swagger for example
There was a problem hiding this comment.
Good point. Fixed. Added default response.
| @@ -531,6 +531,227 @@ | |||
| } | |||
| } | |||
| }, | |||
| "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ContainerService/managedClusters/{managedClusterName}/agentPools": { | |||
There was a problem hiding this comment.
There is one major problem in these new APIs that are being added now. And yes we reviewed them in the past but never reached a conclusion on the optimal design. The problem is as follows -
Currently, before these new APIs, agentPools is modeled as a property on the managedClusters resource. Its modeled as an array. Now, you are also modeling agentPools as a child resource under managedClusters.
First, child resource payload MUST not be included in the parent resource. It makes PUT very dangerous and few big customers have recently been bitten by this inconsistent PUT behavior in Azure today. Modeling child resources in the parent can potentially make it that much more destructive. If you need a reference to the child resource in the parent, please do so using just ARM resource Ids.
Second, It's unclear how will the PUT on managedCluster work in this case. As part of PUT, can one or more agentPool resources be created?
Third, if you allow multiple to be created, how would subsequent PUTs behave if they dont include an agent Pool which was included in the array and created in the first PUT.
Fourth, such modeling can also potentially break RBAC. If someone was explicitly denied read at the child scope (for whatever reason), they can still read all child resource properties by having read access at the parent.
There was a problem hiding this comment.
Just want to clarify - the agentPools are proxy only resource, it's not tracked by ARM. Consider it's an API to patch the array in managedClusters. This is similar to Microsoft.Network/virtualNetworks/subnets. I hope this address the first concern.
Regarding the second, as part of PUT managedCluster, client can put multiple agentPools in the array.
Regarding the third, the current PUT API behaves as patch, so if a subsequent PUT doesn't include the agentpool in array, it's considered a patch to the array. In implementation it's a merge of the array, and the reason is that the agentPools is relatively heavy and we want the deletion to be more explicit.
Regarding the fourth the scenario doesn't really apply. Cluster is the minimum unit to manage AKS, if one can read cluster, they should see agentPools. This is similar to vnet/subnet case.
hope this help address the concerns?
There was a problem hiding this comment.
we call the behavior for 'third' as 'PUTCH' (unofficial term :)) as PUT is behaving like PATCH and it is not recommended.
ravbhatnagar
added
the
ARMChangesRequested
|
@kkmsft @sauryadas - FYI please take a look at comments above. |
|
Signing off from ARM side. Closed on the plan of action over email. Uber plan is to support multiple agent pool creation using explicit CRUD on child resource type and not through the array on managed cluster. |
ravbhatnagar
added
ARMSignedOff
|
@xizhamsft has this been deployed yet? |
|
@jhendrixMSFT yes. It has been deployed. But his merge is to the dev branch. I will send out another PR to merge the dev branch to master |
jhendrixMSFT
merged commit 354c84c
into
Azure:dev-containerservice-Microsoft.ContainerService-2019-02-01
Implementing agentpool API
There is no data model changes. Just adding the new agentpool (untracked child resource under container service) create/update/delete/list API.
Latest improvements:
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Contribution checklist:
ARM API Review Checklist
Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.
Please follow the link to find more details on API review process.