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.

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

azure AD identity support for AzureAD-enabled AKS clusters #205

Merged

Conversation

erhancagirici
Copy link
Collaborator

Description of your changes

Enables configuring Azure AD authentication via kubelogin integration. Introduces the type AzurePrincipalCredentials in ProviderConfig .spec.identity section.

The specified Azure Service Principal credentials provided via Secret, for authenticating to AKS cluster by obtaining a token through kubelogin

An example ProviderConfig using AzurePrincipalCredentials as identity for authenticating to AzureAD

apiVersion: helm.crossplane.io/v1beta1
kind: ProviderConfig
metadata:
  name: helm-provider
spec:
  credentials:
    source: Secret
    secretRef:
      name: cluster-config
      namespace: crossplane-system
      key: kubeconfig
  identity:
    type: AzurePrincipalCredentials
    source: Secret
    secretRef:
      name: azure-credentials
      namespace: crossplane-system
      key: credentials.json

Fixes #180

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Tested with 2 different AKS clusters.

  1. Azure AD auth enabled with Kubernetes RBAC
  2. Azure AD auth enabled with Azure RBAC

The provider configs consist of the following:

  • For credentials: the kubeconfig AKS cluster, obtained via az aks get-credentials --resource-group myresourcegroup --name myclustername, as described in the Azure Portal > AKS cluster > Connect section
  • For identity: an azure service principal

For cluster 1 (AzureAD Auth + k8s RBAC): The service principal is added to a Azure AD group, then that group is added to Cluster admin ClusterRoleBinding at Azure Portal > The AKS cluster > Cluster Configuration

For cluster 2 (AzureAD Auth + Azure RBAC): The service principal is assigned Azure Kubernetes Service RBAC Cluster Admin built-in role for the particular AKS cluster, through Azure Portal > The AKS cluster > Access Control (IAM) > Add Role Assignment. If desired, a custom role can be built that has less privileges and assigned to the service principal.

Using those ProviderConfigs, an example Helm Release manifest is created and reconciled successfully.

@erhancagirici erhancagirici changed the title azure AD identity support for AzureAD clusters azure AD identity support for AzureAD-enabled AKS clusters Dec 5, 2023
@turkenh turkenh self-requested a review December 5, 2023 12:59
@erhancagirici erhancagirici force-pushed the azure-kubelogin-support branch 3 times, most recently from fcf92bd to f5aa5e1 Compare December 5, 2023 15:11
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici erhancagirici force-pushed the azure-kubelogin-support branch from f5aa5e1 to 8db230f Compare December 5, 2023 15:21
@haarchri
Copy link
Member

haarchri commented Dec 5, 2023

what happen if you use the kubeconfig from connectionSecret ?

@erhancagirici
Copy link
Collaborator Author

erhancagirici commented Dec 5, 2023

what happen if you use the kubeconfig from connectionSecret ?

@haarchri summarizing our slack chat for reference here:
it does not make a difference, since this PR is irrelevant of how you supply the the kubeconfig, and does not change anything regarding that.
The content of the kubeconfig is important here, which is produced based on the relevant AKS cluster configuration:

  • If the AKS cluster was configured as Azure AD auth with k8s RBAC or Azure AD Auth with Azure RBAC the produced kubeconfig has the content that require kubelogin, therefore the new identity section in ProviderConfig
  • If configured with Local accounts with k8s RBAC the produced kubeconfig can work standalone, without any helper tool. In this case, you won't need the identity section in ProviderConfig

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@haarchri
Copy link
Member

haarchri commented Dec 6, 2023

tested is working:

kubectl get releases
NAME        CHART   VERSION  SYNCED  READY  STATE   REVISION  DESCRIPTION    AGE
test-aks-7djh7  argo-cd  5.51.1  True   True  deployed  1     Install complete  19m

@haarchri
Copy link
Member

haarchri commented Dec 6, 2023

one side note:

kubectl logs -n upbound-system       crossplane-contrib-provider-helm-877f6679cb2f-84c54c7747-r2qsg 
[controller-runtime] log.SetLogger(...) was never called; logs will not be displayed.
Detected at:
	>  goroutine 161 [running]:
	>  runtime/debug.Stack()
	>  	runtime/debug/stack.go:24 +0x64
	>  sigs.k8s.io/controller-runtime/pkg/log.eventuallyFulfillRoot()
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/log/log.go:60 +0xf4
	>  sigs.k8s.io/controller-runtime/pkg/log.(*delegatingLogSink).WithValues(0x4000059640, {0x40006021c0, 0x2, 0x2})
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/log/deleg.go:168 +0x44
	>  github.com/go-logr/logr.Logger.WithValues(...)
	>  	github.com/go-logr/logr@v1.3.0/logr.go:336
	>  sigs.k8s.io/controller-runtime/pkg/builder.(*Builder).doController.func1(0x40006021a0)
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/builder/controller.go:400 +0x130
	>  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0x4000686fa0, {0x1f4bc60, 0x4000681720}, {0x19d4120?, 0x40006020c0?})
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:306 +0x114
	>  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0x4000686fa0, {0x1f4bc60, 0x4000681720})
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:266 +0x198
	>  sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:227 +0x74
	>  created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2 in goroutine 56
	>  	sigs.k8s.io/controller-runtime@v0.16.3/pkg/internal/controller/controller.go:223 +0x43c

@turkenh
Copy link
Collaborator

turkenh commented Dec 6, 2023

one side note:

Same as crossplane-contrib/provider-upjet-aws#854 (comment)

…essage

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici erhancagirici force-pushed the azure-kubelogin-support branch from e0b38df to 3ace72a Compare December 6, 2023 23:04
Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Great work @erhancagirici 💪

Left a couple of comments but nothing major.

@@ -50,12 +50,14 @@ type IdentityType string
// Supported identity types.
const (
IdentityTypeGoogleApplicationCredentials = "GoogleApplicationCredentials"

IdentityTypeAzurePrincipalCredentials = "AzurePrincipalCredentials"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
IdentityTypeAzurePrincipalCredentials = "AzurePrincipalCredentials"
IdentityTypeAzurePrincipalCredentials = "AzureServicePrincipalCredentials"

Not sure if there is something called as AzurePrincipal vs AzureServicePrincipal but more explicit would be better in the API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Principal is a generic term for referring to an authentication subject. I named it like this in case we support other principals like MSI or workload identity and not directly suggest Service Principals. I am OK with changing to AzureServicePrincipalCredentials or some other name

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest so considering we only support ServicePrincipalLogin and to be explicit there.

cmd/provider/main.go Outdated Show resolved Hide resolved
pkg/clients/azure/azure.go Outdated Show resolved Hide resolved
pkg/clients/azure/azure.go Outdated Show resolved Hide resolved
pkg/controller/release/release.go Outdated Show resolved Hide resolved
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh force-pushed the azure-kubelogin-support branch from f9c60fc to 17ceb75 Compare December 7, 2023 07:12
…eject injected identity source

- update comments for opt parsing, cleanup comments

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici erhancagirici requested a review from turkenh December 7, 2023 09:03
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Copy link
Collaborator

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

Thank you @erhancagirici 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate kubelogin for Azure AD Authentication to AKS Clusters
3 participants