Osa cluster admin #9058

JackQuincy · 2019-04-10T21:42:58Z

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

* adding cluster admin option to osa create * adding direcctory read permissions to the aad app * making active directory app always require directory permissions * infra node to 3

* adding cluster admin option to osa create * adding direcctory read permissions to the aad app * making app always require directory permissions * clean + infra node to 3 * fixing typo * using new version of acs python sdk

mboersma

Looks good Jack, I had a couple minor comments.

mboersma · 2019-04-11T17:52:30Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

    short-summary: The CIDR used on the Subnet into which to deploy the cluster.
+  - name: --customer-admin-group-id
+    type: string
+    short-summary: The ID of an Azure Active Directory Group that memberships will get synced into the OpenShift group "osa-customer-admins". If not specified no cluster admin access will be granted


Could you change the second sentence here to have a comma and end with a period?
If not specified, no cluster admin access will be granted.

looks like this was already updated

mboersma · 2019-04-11T17:55:42Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

    agent_infra_pool_profile = OpenShiftManagedClusterAgentPoolProfile(
        name='infra',  # Must be 12 chars or less before ACS RP adds to it
-        count=int(2),
+        count=int(3),


Does this change the default node pool count from 2 to 3? That seems like a significant change (not related to this PR). Maybe it should be mentioned separately in the release notes.

Yes, we agreed on this to switch from 2 to 3

…nto osa-cluster-admin

yugangw-msft

A few minor comments. Please address and I will merge

yugangw-msft · 2019-04-16T05:57:37Z

src/command_modules/azure-cli-acs/HISTORY.rst


+2.3.20
++++++
+* adding customer-admin-group-id flag to az osa create


please move to the top

thanks, moved to top as 2.3.22 and bumped version

yugangw-msft · 2019-04-16T05:58:47Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

    short-summary: The CIDR used on the Subnet into which to deploy the cluster.
+  - name: --customer-admin-group-id
+    type: string
+    short-summary: The ID of an Azure Active Directory Group that memberships will get synced into the OpenShift group "osa-customer-admins". If not specified, no cluster admin access will be granted.


i believe it should be the object id of the AAD group? If yes, please make it clear

updated description

tjprescott · 2019-04-16T14:57:37Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/_help.py

  - name: Create an OpenShift cluster and auto create an AAD Client
    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN}
+  - name: Create an OpenShift cluster and auto create an AAD Client and setup cluster admin group
+    text: az openshift create -g MyResourceGroup -n MyManagedCluster --fqdn {FQDN} --customer-admin-group-id {GROUP_ID}


This is going to be incompatible with PR #9083 which removes FQDN.

@tjprescott FQDN is not being removed. Its being made optional. PR #9083 will be done on top of this PR.

tjprescott · 2019-04-16T14:58:10Z

src/command_modules/azure-cli-acs/azure/cli/command_modules/acs/custom.py

                                         additional_properties=None, type="Scope")
-        required_osa_aad_access = RequiredResourceAccess(resource_access=[resource_access],
+        # Read directory permissions on Windows Azure Active Directory API
+        directory_access = ResourceAccess(id="5778995a-e1bf-45b8-affa-663a9f3f4d04",


What is this hard-coded ID?

What is this hard-coded ID?

@tjprescott these are scope GUIDs for programmatic use and represent various permissions: https://docs.microsoft.com/en-us/graph/permissions-reference. Currently there isn't a straightforward documentation from AAD for this and this is obtained by looking at currently configured apps in AAD.

# Conflicts: # src/command_modules/azure-cli-acs/HISTORY.rst # src/command_modules/azure-cli-acs/setup.py

amanohar · 2019-04-19T19:24:39Z

@yugangw-msft @mboersma comments addressed by @sozercan. PTAL

tjprescott · 2019-04-19T19:44:52Z

@amanohar @sozercan need to get the CI green

mboersma · 2019-04-19T19:56:35Z

From the CI failures, maybe we just need to re-record the test_aks_create_service_no_wait test?

Edit: or all of them. AZURE_CLI_TEST_MODULES=acs azdev test

This reverts commit 8643736.

# Conflicts: # src/command_modules/azure-cli-acs/setup.py

sozercan · 2019-04-24T00:06:09Z

@tjprescott any ideas why PURPOSE='Load extension commands' test fails? https://travis-ci.org/Azure/azure-cli/jobs/523726845

Verifying extension: dev-spaces
The checksum of the extension does not match the expected value. Use --debug for more information.
The command "./scripts/ci/test_extensions.sh" exited with 1.

sozercan · 2019-04-24T09:57:26Z

@tjprescott @yugangw-msft tests are green now

JackQuincy and others added 4 commits March 27, 2019 15:42

adding cluster admin option to osa create

494a9c8

adding direcctory read permissions to the aad app (#1)

b244fa7

* adding cluster admin option to osa create * adding direcctory read permissions to the aad app * making active directory app always require directory permissions * infra node to 3

adding new version of python sdk (#3)

1356978

* adding cluster admin option to osa create * adding direcctory read permissions to the aad app * making app always require directory permissions * clean + infra node to 3 * fixing typo * using new version of acs python sdk

updating history.rst

0fcfc2e

tjprescott requested a review from mboersma April 11, 2019 17:14

Change type from scope to role

8fdd587

mboersma reviewed Apr 11, 2019

View reviewed changes

apply review

8084cc6

mboersma approved these changes Apr 11, 2019

View reviewed changes

julienstroheker and others added 4 commits April 11, 2019 14:57

Merge branch 'dev' into osa-cluster-admin

3e87b7c

pep fix

a96742c

Merge branch 'osa-cluster-admin' of github.com:JackQuincy/azure-cli i…

15133e9

…nto osa-cluster-admin

fix merge conflict

b8405a4

yugangw-msft reviewed Apr 16, 2019

View reviewed changes

tjprescott suggested changes Apr 16, 2019

View reviewed changes

tjprescott approved these changes Apr 18, 2019

View reviewed changes

tjprescott modified the milestone: Sprint 59 Apr 18, 2019

sozercan added 2 commits April 19, 2019 11:39

update history and help

1f75865

Merge branch 'dev' into osa-cluster-admin

5af166f

# Conflicts: # src/command_modules/azure-cli-acs/HISTORY.rst # src/command_modules/azure-cli-acs/setup.py

yugangw-msft approved these changes Apr 19, 2019

View reviewed changes

sozercan added 4 commits April 19, 2019 23:10

update recorded aks tests

8643736

Revert "update recorded aks tests"

1459498

This reverts commit 8643736.

change api version for tests

f58917a

Merge branch 'dev' into osa-cluster-admin

889e482

# Conflicts: # src/command_modules/azure-cli-acs/setup.py

tjprescott merged commit 70099c5 into Azure:dev Apr 24, 2019

sozercan deleted the osa-cluster-admin branch April 24, 2019 17:11

Osa cluster admin #9058

Osa cluster admin #9058

Uh oh!

Conversation

JackQuincy commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mboersma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanohar Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanohar commented Apr 19, 2019

Uh oh!

tjprescott commented Apr 19, 2019

Uh oh!

mboersma commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sozercan commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sozercan commented Apr 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JackQuincy commented Apr 10, 2019 •

edited

Loading

amanohar Apr 18, 2019 •

edited

Loading

mboersma commented Apr 19, 2019 •

edited

Loading

sozercan commented Apr 24, 2019 •

edited

Loading