Azure active directory support for managedclusters #1560

LochanRn · 2021-07-26T13:48:35Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds Azure active directory support for user authentication in managed clusters

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1407

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Spec aadProfile contains a Legacy Spec for legacy AAD support and Managed Spec for new/managed AAD support.

k8s-ci-robot · 2021-07-26T13:48:43Z

Hi @LochanRn. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

CecileRobertMichon · 2021-07-26T17:17:15Z

docs/book/src/topics/managedcluster.md

@@ -197,6 +197,31 @@ spec:
 ---
 ```

+### AKS AAD
+
+Azure Kubernetes Service can be configured to use Azure Active Directory for user authentication.


can you please add a link to the AKS AAD documentation here? https://docs.microsoft.com/en-us/azure/aks/managed-aad

CecileRobertMichon · 2021-07-26T17:20:43Z

/ok-to-test

LochanRn · 2021-07-28T06:50:07Z

/test pull-cluster-api-provider-azure-e2e-windows

LochanRn · 2021-07-28T09:04:42Z

/test pull-cluster-api-provider-azure-e2e-exp

LochanRn · 2021-07-28T09:08:53Z

/test pull-cluster-api-provider-azure-e2e

LochanRn · 2021-07-28T10:39:48Z

/test pull-cluster-api-provider-azure-e2e

LochanRn · 2021-07-28T12:00:58Z

/test pull-cluster-api-provider-azure-e2e-exp

alexeldeib · 2021-08-05T22:11:14Z

azure/services/managedclusters/managedclusters.go

+			}
+		}
+
+		if existingMC.AadProfile != nil {


is there any normalization happening here? if no, you can simply compare the raw structs.

ah, is tenant ID immutable?

yeah amcp spec is normalized to aadProfile and version.

alexeldeib · 2021-08-05T22:13:11Z

config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedcontrolplanes.yaml

+                description: AadProfile is Azure Active Directory configuration to
+                  integrate with AKS for aad authentication.
+                properties:
+                  legacyAadProfile:


I would almost be tempted to reject this for creates inside the reconciler? I would be okay allowing this for adoption scenarios, but we shouldn't be encouraging it for new clusters.

That is a bit aggressive of a constraint, but I also explicitly didn't support Basic LB or VMAS.

What do you think?

It is a good point even I agree with that. I will make changes accordingly.

alexeldeib · 2021-08-05T22:13:51Z

exp/api/v1alpha3/azuremanagedcontrolplane_types.go

+type LegacyAAD struct {
+	// ClientAppID - The client AAD application ID.
+	// +kubebuilder:validation:Required
+	ClientAppID *string `json:"clientAppID"`


any reason these are pointers if they are required, non-omitempty?

In the begining I was validating it a bit differently, so I needed it. I will change this to normal strings.

I did not have legacyaad and managedaad, all the fields where under one spec AADProfile. So for validation purpose i took them all as a pointer, but this was very tedious and not very clean. Latter I iterated to have legacy and managed as two separate specs. When I iterated i did not change it. But will change it now.

alexeldeib · 2021-08-05T22:14:12Z

exp/api/v1alpha3/azuremanagedcontrolplane_types.go

+
+	// AdminGroupObjectIDs - AAD group object IDs that will have admin role of the cluster.
+	// +kubebuilder:validation:Required
+	AdminGroupObjectIDs *[]string `json:"adminGroupObjectIDs"`


nit: pointer to a slice is awkward

(vs a raw slice)

alexeldeib · 2021-08-05T22:16:07Z

exp/api/v1alpha4/azuremanagedcontrolplane_webhook.go

+		}
+
+		if new.ManagedAAD == nil {
+			if !reflect.DeepEqual(new.LegacyAAD, old.LegacyAAD) {


prefer not to use reflection....but I guess it's fine here since all members are simple types anyway..

alexeldeib · 2021-08-05T22:17:29Z

exp/api/v1alpha4/azuremanagedcontrolplane_webhook.go

+}
+
+func (r *AzureManagedControlPlane) validateLegacyAadProfile() bool {
+	return r.Spec.AADProfile.LegacyAAD != nil


is this missing some additional validation?

Nope I guess I can remove the function here and directly do a check above. But this would be required only during an update now. Support during creation will be removed.

alexeldeib · 2021-08-05T22:17:54Z

minor comments, looks pretty clean on a first pass. nice work!

LochanRn · 2021-08-09T16:38:04Z

/test pull-cluster-api-provider-azure-e2e-exp

alexeldeib · 2021-08-11T00:38:44Z

/lgtm

on a roll! nice work :)

this might be a good one to cover in e2e? we can open an issue perhaps, I supposed it's a bit involved.

LochanRn · 2021-08-11T06:21:50Z

/lgtm

on a roll! nice work :)

this might be a good one to cover in e2e? we can open an issue perhaps, I supposed it's a bit involved.

yes you are right !! We need to cover this in e2e :)

CecileRobertMichon

/approve

k8s-ci-robot · 2021-08-12T17:18:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 26, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 26, 2021

k8s-ci-robot requested review from devigned and shysank July 26, 2021 13:48

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jul 26, 2021

CecileRobertMichon reviewed Jul 26, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 26, 2021

LochanRn force-pushed the aks-aad branch 4 times, most recently from d60aff0 to c2f7843 Compare July 27, 2021 22:19

LochanRn force-pushed the aks-aad branch from c2f7843 to 4b2a95a Compare July 28, 2021 08:00

LochanRn mentioned this pull request Aug 5, 2021

REQUEST: New membership for LochanRn kubernetes/org#2875

Closed

7 tasks

alexeldeib reviewed Aug 5, 2021

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 7, 2021

LochanRn force-pushed the aks-aad branch from 108fe94 to 4b2a95a Compare August 9, 2021 13:48

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

LochanRn force-pushed the aks-aad branch from 4b2a95a to e052294 Compare August 9, 2021 14:53

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

LochanRn force-pushed the aks-aad branch from e052294 to 4cb708c Compare August 9, 2021 15:00

azure active directory support for managedclusters

47d5dea

LochanRn force-pushed the aks-aad branch from 4cb708c to 47d5dea Compare August 9, 2021 15:34

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 9, 2021

k8s-ci-robot assigned alexeldeib Aug 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021

CecileRobertMichon reviewed Aug 12, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2021

k8s-ci-robot merged commit 3a1557d into kubernetes-sigs:master Aug 12, 2021

LochanRn deleted the aks-aad branch August 21, 2022 06:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure active directory support for managedclusters #1560

Azure active directory support for managedclusters #1560

LochanRn commented Jul 26, 2021 •

edited

Loading

k8s-ci-robot commented Jul 26, 2021

CecileRobertMichon Jul 26, 2021

LochanRn Jul 26, 2021

CecileRobertMichon commented Jul 26, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

alexeldeib Aug 5, 2021

alexeldeib Aug 5, 2021

LochanRn Aug 9, 2021

alexeldeib Aug 5, 2021

LochanRn Aug 6, 2021

alexeldeib Aug 5, 2021

LochanRn Aug 6, 2021 •

edited

Loading

LochanRn Aug 6, 2021

alexeldeib Aug 5, 2021

alexeldeib Aug 5, 2021

LochanRn Aug 6, 2021

alexeldeib Aug 5, 2021

alexeldeib Aug 5, 2021

LochanRn Aug 6, 2021

alexeldeib commented Aug 5, 2021

LochanRn commented Aug 9, 2021

alexeldeib commented Aug 11, 2021

LochanRn commented Aug 11, 2021

CecileRobertMichon left a comment

k8s-ci-robot commented Aug 12, 2021

Azure active directory support for managedclusters #1560

Azure active directory support for managedclusters #1560

Conversation

LochanRn commented Jul 26, 2021 • edited Loading

k8s-ci-robot commented Jul 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Jul 26, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

LochanRn commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LochanRn Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Aug 5, 2021

LochanRn commented Aug 9, 2021

alexeldeib commented Aug 11, 2021

LochanRn commented Aug 11, 2021

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 12, 2021

LochanRn commented Jul 26, 2021 •

edited

Loading

LochanRn Aug 6, 2021 •

edited

Loading