Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AKS] add new 2018-03-31 API #3131

Merged
merged 16 commits into from
Jun 1, 2018
Merged

[AKS] add new 2018-03-31 API #3131

merged 16 commits into from
Jun 1, 2018

Conversation

mboersma
Copy link
Member

@mboersma mboersma commented May 24, 2018 •
edited
Loading

The new AKS v20180331 API is already deployed in production. It contains several new features for cluster creation:

  • Kubernetes RBAC
  • Custom VNET
  • SSH key optional
  • Add-ons (supporting HTTP application routing and monitoring with OMSAgent)
  • Azure Active Directory auth integration

This also rectifies the longstanding issue of AKS not having an ARM Operations API endpoint.

cc: @JunSun17 @amanohar @weinong @sauryadas please help ensure this contains the new API changes and documents them correctly.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@azuresdkci azuresdkci requested a review from sarangan12 May 24, 2018 16:21
@AutorestCI
Copy link

AutorestCI commented May 24, 2018 •
edited
Loading

Automation for azure-libraries-for-java

The initial PR has been merged into your service PR:
AutorestCI/azure-libraries-for-java#174

@AutorestCI
Copy link

AutorestCI commented May 24, 2018 •
edited
Loading

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2848

@AutorestCI
Copy link

AutorestCI commented May 24, 2018 •
edited
Loading

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2186

@AutorestCI
Copy link

AutorestCI commented May 24, 2018 •
edited
Loading

Automation for azure-sdk-for-go

Encountered a Subprocess error: (azure-sdk-for-go)

Command: ['/usr/local/bin/autorest', '/tmp/tmpu0h7nzek/rest/specification/containerservices/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmpu0h7nzek/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '--use=@microsoft.azure/autorest.go@~2.1.100', '--use-onever', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4278/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4278)
   Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.100->2.1.100)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2017-09"} .
FATAL: swagger-document/compose - FAILED
FATAL: Error: '$.definitions.ContainerServiceVnetSubnetID.description' has incompatible values (---
VNet SubnetID specifies the vnet's subnet identifier.
, ---
  -
  VNet SubnetID specifies the vnet's subnet identifier. If you specify either
  master VNet Subnet, or agent VNet Subnet, you need to specify both. And they
  have to be in the same VNet.
).
Process() cancelled due to exception : '$.definitions.ContainerServiceVnetSubnetID.description' has incompatible values (---
VNet SubnetID specifies the vnet's subnet identifier.
, ---
  -
  VNet SubnetID specifies the vnet's subnet identifier. If you specify either
  master VNet Subnet, or agent VNet Subnet, you need to specify both. And they
  have to be in the same VNet.
).
Failure during batch task - {"tag":"package-2017-09"} -- Error: '$.definitions.ContainerServiceVnetSubnetID.description' has incompatible values (---
VNet SubnetID specifies the vnet's subnet identifier.
, ---
  -
  VNet SubnetID specifies the vnet's subnet identifier. If you specify either
  master VNet Subnet, or agent VNet Subnet, you need to specify both. And they
  have to be in the same VNet.
)..
  Error: '$.definitions.ContainerServiceVnetSubnetID.description' has incompatible values (---
VNet SubnetID specifies the vnet's subnet identifier.
, ---
  -
  VNet SubnetID specifies the vnet's subnet identifier. If you specify either
  master VNet Subnet, or agent VNet Subnet, you need to specify both. And they
  have to be in the same VNet.
).

@AutorestCI AutorestCI mentioned this pull request May 24, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented May 24, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-ruby#1342

This was referenced May 25, 2018
Merged
Merged
mboersma added 2 commits May 25, 2018 16:27
@mboersma
Reconcile two definitions of OSType enum.
2104f85
@mboersma
Rename to ManagedClusterAgentPoolProfile.
e365e10 
Its fields have diverged from ContainerServiceAgentPoolProfile.
@AutorestCI AutorestCI mentioned this pull request May 25, 2018
Merged
mboersma
mboersma commented May 25, 2018
View reviewed changes
specification/containerservices/resource-manager/readme.md
@@ -208,7 +208,7 @@ python:
payload-flattening-threshold: 2
namespace: azure.mgmt.containerservice
package-name: azure-mgmt-containerservice
package-version: 3.1.0
package-version: 4.0.0
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Yes, definitely breaking changes in here unfortunately.

mboersma added 2 commits May 25, 2018 16:54
@mboersma
Revert "Reconcile two definitions of OSType enum."
cc94d98 
This reverts commit 2104f85.
@mboersma
Reconcile two definitions of OSType enum.
8a8e4b8 
Added "modelAsString" to containerServices' definition.
@mboersma
Copy link
Member Author

@sarangan12 could I get a review on this soon? I have a Friday deadline for getting the related az CLI changes in.

@lmazuel lmazuel assigned lmazuel and unassigned sarangan12 May 30, 2018
@lmazuel lmazuel removed the request for review from sarangan12 May 30, 2018 18:47
@lmazuel lmazuel self-requested a review May 30, 2018 18:47
@lmazuel
Copy link
Member

lmazuel commented May 30, 2018

Stealing from you @sarangan12 as discussed

@lmazuel lmazuel added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label May 30, 2018
@lmazuel
Copy link
Member

lmazuel commented May 30, 2018

@ravbhatnagar Please review for new API. Thanks!

@lmazuel
Copy link
Member

lmazuel commented May 30, 2018

@mboersma For what I see in the Python code:

  • managed_clusters.get_access_profiles disapears
  • A bunch of new models and massive properties change in ManagedCluster

Does it sound accurate? Something you want to mention?

@mboersma
Copy link
Member Author

@lmazuel yes--the get_access_profiles method is deprecated in the previous API v20180831 and removed in this one. Now there's get_access_profile which uses POST to accomplish the same thing.
And yes, there are several new models adding properties to ManagedCluster. Also ACS and AKS use agent pools differently now, so I had to split out a ManagedClusterAgentPoolProfile from the ContainerService[...] one.

lmazuel
lmazuel approved these changes May 30, 2018
View reviewed changes
Copy link
Member

@lmazuel lmazuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, waiting ARM feedback

JunSun17
JunSun17 approved these changes May 30, 2018
View reviewed changes
Copy link
Contributor

@JunSun17 JunSun17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. There are a few comments related to custom VNET and would be great to have it updated.

For the other features, I roughly checked the semantics and looks good to me.

...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json Outdated
},
"ContainerServiceVnetSubnetID": {
"type": "string",
"description": "VNet SubnetID specifies the vnet's subnet identifier. If you specify either master VNet Subnet, or agent VNet Subnet, you need to specify both. And they have to be in the same VNet."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove "If you specify either master VNet Subnet, or agent VNet Subnet, you need to specify both. And they have to be in the same VNet.". In AKS case, we only need the agent subnet ID.

...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json
"clientId"
]
},
"ContainerServiceMasterProfile": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not sure we need all or maybe some of the master profiles in AKS. If it has been this way in previous version, it is probably ok to not cleaning it for now, maybe create a task to document this cleaning work for future?

There might be some other similar items that is in the ACS model but not in the AKS model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been this way since AKS began, so I'd rather leave it for now. I'll make a task to consider cleaning it up in the future.

...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json Outdated
},
"vnetSubnetID": {
"$ref": "#/definitions/ContainerServiceVnetSubnetID",
"description": "VNet SubnetID specifies the vnet's subnet identifier. If you specify either master VNet Subnet, or agent VNet Subnet, you need to specify both. And they have to be in the same VNet."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comments, remove some wording.

...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json Outdated
"type": "string",
"enum": [
"calico",
"cilium"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove cilium from the enum here, it is my mistake originally and I was told cilium does not need to be supported in the coming release.

...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json
},
"podCidr": {
"type": "string",
"pattern": "^([0-9]{1,3}\\.){3}[0-9]{1,3}(\\/([0-9]|[1-2][0-9]|3[0-2]))?$",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the pattern match!

Could you also add a default for pod cidr: "10.244.0.0/16" since other fields have a default value now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea--added. I just took the previous defaults from the portal UX and this one was empty there.

rjtsdl
rjtsdl approved these changes May 30, 2018
View reviewed changes
...rservices/resource-manager/Microsoft.ContainerService/stable/2018-03-31/managedClusters.json
},
"description": "Profile of managed cluster add-on."
},
"enableRBAC": {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, simple and accurate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so you know, autorest complains about this field not being proper CamelCase. (I guess it expected enableRbac.) Since it's in acs-engine and the RP, I don't think it's reasonable to change it now.

@mboersma
Copy link
Member Author

@ravbhatnagar could you review the new AKS API here? (I'm hoping to get this done today so there is a fighting chance of making the related CLI changes by their sprint deadline tomorrow.)

@lmazuel
Copy link
Member

lmazuel commented May 31, 2018

@mboersma Please also fix the examples on Travis CI:
https://travis-ci.org/Azure/azure-rest-api-specs/jobs/385909694

Also, you have a message that requires a fix or a suppression validated by @ravbhatnagar :

"Tracked resource 'ManagedCluster' must have patch operation that at least supports the update of tags. It's strongly recommended that the PATCH operation supports update of all mutable properties as well."

@mboersma
Copy link
Member Author

Tracked resource 'ManagedCluster' must have patch operation

Yes, this has been a (suppressed) error for ACS and AKS since their inception. We implemented the ARM operations API but don't have PATCH yet, so hopefully we can continue to suppress this for now.

@ravbhatnagar
Copy link
Contributor

One note about removal of the API on which Matt is going to start a thread and we will discuss there. That doesnt block this PR. Signing off from ARM side.

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels May 31, 2018
@lmazuel lmazuel merged commit c2ded50 into Azure:master Jun 1, 2018
@mboersma mboersma mentioned this pull request Jun 1, 2018
Merged
10 tasks
@tombuildsstuff tombuildsstuff mentioned this pull request Jun 1, 2018
Closed
@mboersma mboersma deleted the aks-2018-03-31 branch June 1, 2018 13:26
This was referenced Jun 1, 2018
Merged
Merged
@mboersma mboersma mentioned this pull request Jun 5, 2018
Merged
2 tasks
mccleanp pushed a commit that referenced this pull request Mar 23, 2022
@leni-msft
Merge pull request #3131 from Azure/dev/subbart/removeheader
4f0046d 
Removing header details in the swagger to work around RPSaaS bug...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JunSun17 JunSun17 JunSun17 approved these changes

@rjtsdl rjtsdl rjtsdl approved these changes

@lmazuel lmazuel lmazuel approved these changes

Assignees

@lmazuel lmazuel

Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mboersma @AutorestCI @lmazuel @ravbhatnagar @rjtsdl @JunSun17 @sarangan12