[AKS] add POST endpoint for fetching accessProfiles #3098

mboersma · 2018-05-18T02:16:46Z

Adds a new POST endpoint that is preferred to GET for fetching AKS access profiles (Kubernetes configs). The endpoint is already deployed to RPs in all AKS regions.

I've tested this with the Python SDK and simple CLI changes that are needed and it works well.

cc: @weinong @shrutir25 @rjtsdl @sauryadas

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-05-18T02:18:02Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-05-18T02:18:36Z

Automation for azure-sdk-for-node

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

AutorestCI · 2018-05-18T02:18:43Z

Automation for azure-libraries-for-java

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

AutorestCI · 2018-05-18T02:20:37Z

Automation for azure-sdk-for-go

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

lmazuel · 2018-05-18T17:21:58Z

...rservices/resource-manager/Microsoft.ContainerService/stable/2017-08-31/managedClusters.json

@@ -154,6 +154,58 @@
        }
      }
    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ContainerService/managedClusters/{resourceName}/accessProfiles/{roleName}/listCredential": {
+      "post": {


@mboersma If this operation is preferred to ManagedClusters_GetAccessProfiles, you should declare it as deprecated (deprecated: True and update the description to say something generic like "Use ListCredential instead").

@lmazuel done.

hovsepm

Operation name with prefix List assumes multiple return values in array. You already have "operationId": "ManagedClusters_GetAccessProfiles", which returns exactly the same type ManagedClusterAccessProfile. then what is a point to add this new endpoint with such a confusing method name?

mboersma · 2018-05-18T22:29:18Z

what is a point to add this new endpoint with such a confusing method name?

@hovsepm I was just using the name defined for the endpoint in the AKS RP. I agree it's not good terminology though. We can call it something different in the swagger if that's preferable. The idea here is we'd prefer POST to GET for this sensitive endpoint.

@weinong anything to add?

hovsepm · 2018-05-18T22:38:54Z

@mboersma yes please change the operation name. it is used to generate a method names in the SDKs in each language. Name should be self descriptive and discoverable. Please also revise the description because now for this 2 exposed operations

...
"operationId": "ManagedClusters_ListCredential",
"description": "Gets the accessProfile for the specified role name of the managed cluster with a specified resource group and name."
...
"operationId": "ManagedClusters_GetAccessProfiles"
"description": "Gets the accessProfile for the specified role name of the managed cluster with a specified resource group and name."

as a user of the generated SDK - how should I understand which method to call? What is the difference between them?

hovsepm · 2018-05-18T22:44:16Z

...rservices/resource-manager/Microsoft.ContainerService/stable/2017-08-31/managedClusters.json

@@ -116,7 +116,8 @@
        ],
        "operationId": "ManagedClusters_GetUpgradeProfile",
        "summary": "Gets upgrade profile for a managed cluster.",
-        "description": "Gets the details of the upgrade profile for a managed cluster with a specified resource group and name.",
+        "deprecated": true,
+        "description": "Use ManagedClusters_ListCredential instead.",


I think you are deprecating the wrong operation. I believe you should deprecate operationId ManagedClusters_GetAccessProfiles, right?

D'oh, my mistake. I'll fix.

hovsepm · 2018-05-21T20:57:27Z

@mboersma I'll merge this PR however there is an issue that needs to be fixed for each endpoint. Please take a look to #3113

tombuildsstuff · 2018-05-22T17:12:15Z

@mboersma 👋 out of interest, is there a timeframe for deprecating the old (GET) endpoint?

mboersma · 2018-05-22T22:38:15Z

@tombuildsstuff once this change makes it into the az CLI, we plan to start returning a (useful) error from the GET endpoint 14 days afterward. So most likely that means two weeks after June 5. Let me know if that's going to cause problems!

tombuildsstuff · 2018-05-23T09:53:13Z

@mboersma given that won't be available in the Go SDK until around that date, I don't feel that's a long enough period for deprecation IMO - we (Terraform) expose these values on our AKS resources, which we consume via the Azure SDK for Go. Whilst we're probably able to time a release shortly after this change goes live - however we should give users some time to update to the new version which uses Post instead of Get.

As such - would it be possible for the old endpoint to continue working until the start of July? Whilst I can understand this isn't ideal from your side, I think it's better to not break users workflows without a couple of weeks extra transition period for downstream dependencies once the SDK's are out :)

Thanks!

tombuildsstuff · 2018-05-23T09:53:21Z

cc @joshgav / @marstr / @jhendrixMSFT :)

mboersma · 2018-05-23T13:39:33Z

I don't feel that's a long enough period for deprecation IMO

Honestly, I don't think that's long enough either, I'm just reporting the current intent. I'll bring your feedback to the team and see if we can make the deprecation timeline more humane.

tombuildsstuff · 2018-05-23T14:11:55Z

@mboersma awesome, thanks :)

mboersma · 2018-06-01T13:14:15Z

@tombuildsstuff we will comply with the standard Azure deprecation policy and keep the GET endpoint functional for 12 more months. (We got similar feedback from ARM review on the newer API in #3131.)

tombuildsstuff · 2018-06-01T13:35:52Z

@mboersma that's awesome, thanks so much :)

[AKS] add POST endpoint for fetching accessProfiles

3c1665f

azuresdkci assigned hovsepm May 18, 2018

azuresdkci requested a review from hovsepm May 18, 2018 02:17

AutorestCI mentioned this pull request May 18, 2018

[AutoPR containerservices/resource-manager] [AKS] add POST endpoint for fetching accessProfiles Azure/azure-sdk-for-python#2583

Merged

AutorestCI mentioned this pull request May 18, 2018

[AutoPR containerservices/resource-manager] [AKS] add POST endpoint for fetching accessProfiles Azure/azure-sdk-for-node#2876

Merged

AutorestCI mentioned this pull request May 18, 2018

[AutoPR containerservices/resource-manager] [AKS] add POST endpoint for fetching accessProfiles AutorestCI/azure-libraries-for-java#173

Merged

AutorestCI mentioned this pull request May 18, 2018

[AutoPR containerservices/resource-manager] [AKS] add POST endpoint for fetching accessProfiles Azure/azure-sdk-for-go#1863

Merged

lmazuel reviewed May 18, 2018

View reviewed changes

mboersma added 2 commits May 18, 2018 11:30

Mark GET endpoint for accessProfiles as deprecated

a86af5d

Bump package version

14544bf

hovsepm approved these changes May 18, 2018

View reviewed changes

hovsepm suggested changes May 18, 2018

View reviewed changes

hovsepm reviewed May 18, 2018

View reviewed changes

mboersma added 2 commits May 18, 2018 17:15

Deprecate the intended endpoint

72b668f

Rename to GetAccessProfile

a69eb4b

hovsepm approved these changes May 21, 2018

View reviewed changes

hovsepm mentioned this pull request May 21, 2018

[AKS] each endpoint operation responses should include "default" that should describe error response type #3113

Open

hovsepm merged commit 37e2f7b into Azure:master May 21, 2018

mboersma deleted the aks-list-credential branch May 22, 2018 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AKS] add POST endpoint for fetching accessProfiles #3098

[AKS] add POST endpoint for fetching accessProfiles #3098

mboersma commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

lmazuel May 18, 2018

mboersma May 18, 2018

hovsepm left a comment

mboersma commented May 18, 2018

hovsepm commented May 18, 2018

hovsepm May 18, 2018

mboersma May 18, 2018

hovsepm commented May 21, 2018

tombuildsstuff commented May 22, 2018

mboersma commented May 22, 2018

tombuildsstuff commented May 23, 2018

tombuildsstuff commented May 23, 2018

mboersma commented May 23, 2018

tombuildsstuff commented May 23, 2018

mboersma commented Jun 1, 2018

tombuildsstuff commented Jun 1, 2018

[AKS] add POST endpoint for fetching accessProfiles #3098

[AKS] add POST endpoint for fetching accessProfiles #3098

Conversation

mboersma commented May 18, 2018 • edited Loading

PR information

Quality of Swagger

AutorestCI commented May 18, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented May 18, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented May 18, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented May 18, 2018 • edited Loading

Automation for azure-sdk-for-go

lmazuel May 18, 2018

Choose a reason for hiding this comment

mboersma May 18, 2018

Choose a reason for hiding this comment

hovsepm left a comment

Choose a reason for hiding this comment

mboersma commented May 18, 2018

hovsepm commented May 18, 2018

hovsepm May 18, 2018

Choose a reason for hiding this comment

mboersma May 18, 2018

Choose a reason for hiding this comment

hovsepm commented May 21, 2018

tombuildsstuff commented May 22, 2018

mboersma commented May 22, 2018

tombuildsstuff commented May 23, 2018

tombuildsstuff commented May 23, 2018

mboersma commented May 23, 2018

tombuildsstuff commented May 23, 2018

mboersma commented Jun 1, 2018

tombuildsstuff commented Jun 1, 2018

mboersma commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading

AutorestCI commented May 18, 2018 •

edited

Loading