Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementing agentpool API #5352
Implementing agentpool API #5352
Changes from 5 commits
6ac7354
bed90f3
01448f8
5d5bd63
cda2833
9b8b68c
ed7db73
db67eaa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we call the behavior for 'third' as 'PUTCH' (unofficial term :)) as PUT is behaving like PATCH and it is not recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Fixed. Added default response.