Disable local accounts for aks aad clusters #4008

LochanRn · 2023-09-18T20:37:40Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Support to disable local accounts for aks aad 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 #2129

Special notes for your reviewer:

As access for admin kubeconfig will be disabled, the user must ensure to add the service principle to the admin group configured for enabling AAD.

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Support disabling of local accounts for AKS clusters with Azure Active Directory enabled. Disables the back door to get admin kubeconfig for AAD based clusters. The user must ensure to add the service principle to admin groups in azure AAD for capz to be able to deploy the target cluster..

codecov · 2023-09-19T04:50:58Z

Codecov Report

Attention: 81 lines in your changes are missing coverage. Please review.

Comparison is base (b9430aa) 57.61% compared to head (1841617) 57.65%.
Report is 26 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4008      +/-   ##
==========================================
+ Coverage   57.61%   57.65%   +0.03%     
==========================================
  Files         188      188              
  Lines       19200    19320     +120     
==========================================
+ Hits        11062    11138      +76     
- Misses       7507     7550      +43     
- Partials      631      632       +1

Files	Coverage Δ
api/v1beta1/azuremanagedcontrolplane_types.go	`20.00% <ø> (ø)`
api/v1beta1/azuremanagedcontrolplane_webhook.go	`89.67% <100.00%> (+0.59%)`	⬆️
azure/scope/managedcontrolplane.go	`25.97% <65.21%> (+3.30%)`	⬆️
azure/services/managedclusters/spec.go	`52.95% <0.00%> (-0.99%)`	⬇️
azure/services/managedclusters/client.go	`0.00% <0.00%> (ø)`
controllers/azuremanagedcontrolplane_reconciler.go	`33.68% <0.00%> (-1.10%)`	⬇️
azure/services/managedclusters/managedclusters.go	`55.00% <28.00%> (-20.35%)`	⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Jont828

Nice work! Just some small changes

azure/scope/managedcontrolplane.go

azure/services/managedclusters/managedclusters.go

azure/services/managedclusters/spec.go

api/v1beta1/azuremanagedcontrolplane_types.go

azure/scope/managedcontrolplane.go

azure/services/managedclusters/managedclusters.go

LochanRn · 2023-10-04T15:45:40Z

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

LochanRn · 2023-10-04T17:01:59Z

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

controllers/azuremanagedcontrolplane_reconciler.go

willie-yao · 2023-10-09T22:26:42Z

PR looks good pending the unit test for ReconcileKubeconfig! Although I don't think it would be blocking the PR for merge.

/lgtm

Jont828 · 2023-10-10T22:32:02Z

LGTM aside from pending comments and once tests pass. Great work so far!

LochanRn · 2023-10-11T03:38:50Z

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

CecileRobertMichon

/lgtm
/approve

Thank you @LochanRn!

@nojnhuh FYI this PR might be disruptive for both #4052 and #4069 but it's worth merging as-is and rebasing the two big PRs on top since it's a feature that's been asked for for a while

k8s-ci-robot · 2023-10-11T14:44:41Z

LGTM label has been added.

Git tree hash: 519a076c6a9c1a104262a5bd495484b0403633ca

k8s-ci-robot · 2023-10-11T14:44:41Z

[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

LochanRn · 2023-10-11T15:37:38Z

@CecileRobertMichon PTAL i added docs !

CecileRobertMichon

minor nits

docs/book/src/topics/managedcluster.md

CecileRobertMichon

/lgtm

k8s-ci-robot · 2023-10-11T16:23:56Z

LGTM label has been added.

Git tree hash: c66bb0f3835c52b003dd275b21c18b5be312144b

nojnhuh · 2023-10-11T22:41:16Z

azure/services/managedclusters/managedclusters.go

+  If we disable local accounts for aad clusters we do not have access to admin kubeconfig, hence we need to create
+  the admin kubeconfig by authenticating with the user credentials and retrieving the token for kubeconfig.
+  The token is used to create the admin kubeconfig.


Is this comment correct or should it be referring to the user kubeconfig instead of the admin one?

This comment is correct, if we refer to the user kubeconfig we will need kubelogin to access the cluster.

docs/book/src/topics/managedcluster.md

k8s-ci-robot requested review from Jont828 and willie-yao September 18, 2023 20:38

LochanRn force-pushed the support-for-disabling-local-accounts branch from 6405e5a to ba6b705 Compare September 19, 2023 04:35

Jont828 reviewed Sep 19, 2023

View reviewed changes

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

LochanRn force-pushed the support-for-disabling-local-accounts branch 2 times, most recently from ba887fb to 800b3ea Compare September 20, 2023 03:46

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

CecileRobertMichon assigned Jont828 Sep 21, 2023

willie-yao reviewed Sep 27, 2023

View reviewed changes

azure/scope/managedcontrolplane.go Outdated Show resolved Hide resolved

azure/services/managedclusters/managedclusters.go Outdated Show resolved Hide resolved

azure/services/managedclusters/managedclusters.go Show resolved Hide resolved

LochanRn force-pushed the support-for-disabling-local-accounts branch 2 times, most recently from 5cf2542 to b7b2ef1 Compare October 3, 2023 20:50

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

LochanRn force-pushed the support-for-disabling-local-accounts branch from b7b2ef1 to 023875d Compare October 6, 2023 09:58

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 6, 2023

LochanRn force-pushed the support-for-disabling-local-accounts branch 2 times, most recently from 12f51a5 to 0abf40e Compare October 6, 2023 18:16

CecileRobertMichon reviewed Oct 6, 2023

View reviewed changes

controllers/azuremanagedcontrolplane_reconciler.go Show resolved Hide resolved

k8s-ci-robot assigned willie-yao Oct 9, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2023

k8s-ci-robot requested a review from willie-yao October 10, 2023 18:21

LochanRn force-pushed the support-for-disabling-local-accounts branch from 71e6f25 to 670e1b2 Compare October 10, 2023 18:31

LochanRn force-pushed the support-for-disabling-local-accounts branch 3 times, most recently from 1f8f84c to d94010e Compare October 11, 2023 14:42

CecileRobertMichon approved these changes Oct 11, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Oct 11, 2023

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023

LochanRn force-pushed the support-for-disabling-local-accounts branch from d94010e to 951e829 Compare October 11, 2023 15:36

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023

k8s-ci-robot requested a review from CecileRobertMichon October 11, 2023 15:36

CecileRobertMichon reviewed Oct 11, 2023

View reviewed changes

Disable local accounts for aks aad clusters

1841617

LochanRn force-pushed the support-for-disabling-local-accounts branch from 951e829 to 1841617 Compare October 11, 2023 16:14

CecileRobertMichon reviewed Oct 11, 2023

View reviewed changes

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

k8s-ci-robot merged commit 7fc4734 into kubernetes-sigs:main Oct 11, 2023
11 checks passed

k8s-ci-robot added this to the v1.12 milestone Oct 11, 2023

nojnhuh reviewed Oct 11, 2023

View reviewed changes

nojnhuh reviewed Oct 12, 2023

View reviewed changes

docs/book/src/topics/managedcluster.md Show resolved Hide resolved

nojnhuh added a commit to nojnhuh/cluster-api-provider-azure that referenced this pull request Oct 14, 2023

merge to resolve conflicts with kubernetes-sigs#4008

2fa8af2

nojnhuh mentioned this pull request Oct 16, 2023

convert managedclusters and agentpools to ASO #4069

Merged

4 tasks

nojnhuh mentioned this pull request Oct 24, 2024

handle Entra auth for ASO API managed clusters #5211

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable local accounts for aks aad clusters #4008

Disable local accounts for aks aad clusters #4008

LochanRn commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

Jont828 left a comment

LochanRn commented Oct 4, 2023

LochanRn commented Oct 4, 2023

willie-yao commented Oct 9, 2023

Jont828 commented Oct 10, 2023 •

edited

Loading

LochanRn commented Oct 11, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 11, 2023

k8s-ci-robot commented Oct 11, 2023

LochanRn commented Oct 11, 2023

CecileRobertMichon left a comment

CecileRobertMichon left a comment

k8s-ci-robot commented Oct 11, 2023

nojnhuh Oct 11, 2023

LochanRn Oct 12, 2023

Disable local accounts for aks aad clusters #4008

Disable local accounts for aks aad clusters #4008

Conversation

LochanRn commented Sep 18, 2023 • edited Loading

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

Jont828 left a comment

Choose a reason for hiding this comment

LochanRn commented Oct 4, 2023

LochanRn commented Oct 4, 2023

willie-yao commented Oct 9, 2023

Jont828 commented Oct 10, 2023 • edited Loading

LochanRn commented Oct 11, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 11, 2023

k8s-ci-robot commented Oct 11, 2023

LochanRn commented Oct 11, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 11, 2023

nojnhuh Oct 11, 2023

Choose a reason for hiding this comment

LochanRn Oct 12, 2023

Choose a reason for hiding this comment

LochanRn commented Sep 18, 2023 •

edited

Loading

codecov bot commented Sep 19, 2023 •

edited

Loading

Jont828 commented Oct 10, 2023 •

edited

Loading