✨ feat: Add IRSA support for self-managed clusters (rebase) #5109

sl1pm4t · 2024-08-28T02:45:07Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds IRSA functionality to self-managed clusters. This will bring the self-managed clusters inline in functionality with Managed clusters that provide most of this functionality out of the box.

With this PR, the following new resources are created:

S3 bucket ( if not already created for the Ignition feature )
Two public world readable objects in the S3 bucket:
- <cluster_name>/.well-known/openid-configuration - OpenID Connect discovery document
- <cluster_name>/openid/v1/jwks - Service Account signing public key
AWS IAM Identity Provider, configured to trust the Issuer found at the S3 URL where the OIDC discovery doc and keys are published.

This is a continuation of an old unmerged PR #4094 - with some fixes and some functionality removed to reduce the scope of the PR.

The functionality removed includes:

This PR no longer deploys the amazon-pod-identity-webhook addon to the workload cluster. I felt there are already many ways to manage cluster addons, including ClusterResourceSets or CAAPH, and that it was unnecessary to install the addon via the controller which then becomes an ongoing maintenance burden. Instead, the requirement for the addon has been added to the documentation.
This PR no longer modifies the API Server service-account-issuer argument through kubeadm patches. This is easily covered in the documentation and only requires a single line of config to be added to the AWSCluster resource, but also during testing I experienced issues with this being applied inconsistently, resulting in different values across the control plane nodes.

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 #3560
Supersedes #4094

Special notes for your reviewer:

This PR adds a new ReconcileOIDCProvider to the AWSCluster reconciliation loop.

Created a new IAM service, in the future the same logic for EKS could be combined as previously the OIDC code was buried in the EKS service. Details on the reconciler can be found in comments.
Extend the S3 service to allow uploading data to arbitrary keys, that can also be marked as public (OIDC discovery docs need to be public for 3rd party systems to retrieve public keys etc.)
Exposed a ManagementClient and RemoteClient for both cluster types and exported Client.
Moved OIDCProvider status type to v1beta2 and migrated out of the EKS API to make one type both clusters can reference a single type.
This PR adds a new Experimental feature flag to enable this functionality. This feature is dependent on the S3 bucket associated with Ignition node configuration, but it felt unintuitive to need to enable the Ignition feature flag to get OIDC support.

Checklist:

Release note:

Add IRSA support for self-hosted clusters

k8s-ci-robot · 2024-08-28T02:45:17Z

Hi @sl1pm4t. 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-sigs/prow repository.

luthermonson · 2024-08-28T23:32:11Z

/ok-to-test

sl1pm4t · 2024-08-30T04:25:23Z

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

sl1pm4t · 2024-09-03T20:29:05Z

There is some investigation of the failing e2e test happening over here.
It would appear the failure is unrelated to this PR.

sl1pm4t · 2024-09-25T01:37:59Z

/retest-required

richardcase · 2024-09-30T09:33:11Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

sl1pm4t · 2024-10-22T02:49:02Z

/retest

richardcase · 2024-11-04T13:29:46Z

/test pull-cluster-api-provider-aws-e2e-eks

STASiAN · 2025-01-28T10:26:17Z

@sl1pm4t do you need help with rebase?

sl1pm4t · 2025-01-28T10:35:22Z

@sl1pm4t do you need help with rebase?

No I will be able to rebase. I had just forgotten this was needed. I'll try to do this in the next day or two.

k8s-ci-robot · 2025-01-28T19:40:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ankitasw for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

sl1pm4t · 2025-01-28T22:51:48Z

I rebased and re-ran make generate which updated config/rbac/role.yaml to what is seen in the latest commit.

richardcase · 2025-01-29T16:05:49Z

/test pull-cluster-api-provider-aws-e2e-eks
/test pull-cluster-api-provider-aws-e2e

sl1pm4t · 2025-01-29T22:41:35Z

/test pull-cluster-api-provider-aws-e2e-eks

richardcase · 2025-01-31T14:49:07Z

Both e2e have passed 🎉

richardcase · 2025-01-31T14:50:34Z

api/v1beta1/s3bucket.go

@@ -37,12 +35,6 @@ func (b *S3Bucket) Validate() []*field.Error {
 		errs = append(errs, field.Required(field.NewPath("spec", "s3Bucket", "name"), "can't be empty"))
 	}

-	// Feature gate is not enabled but ignition is enabled then send a forbidden error.
-	if !feature.Gates.Enabled(feature.BootstrapFormatIgnition) {


Was this removed on purpose?

I'm not sure, this PR was based on work started by @luthermonson so I don't have the full history.
I see the check still exists in the v1beta2 validation, so I'll revert this and do some tests.

richardcase · 2025-01-31T14:52:49Z

feature/feature.go

@@ -67,6 +67,9 @@ const (
 	// BootstrapFormatIgnition will allow an user to enable alternate machine bootstrap format, viz. Ignition.
 	BootstrapFormatIgnition featuregate.Feature = "BootstrapFormatIgnition"

+	// OIDCProviderSupport will allow a user to enable OIDC provider support for kubeadm clusters.
+	OIDCProviderSupport featuregate.Feature = "OIDCProviderSupport"


As we have OIDC provider support in EKS should the feature gate me renamed so that its explicit non-eks (i.e. unmanaged)?

I appreciate the comment says it's for kubeadm clusters but think we may need to be more clear as the gate name is used in the code.

Good point, I'll update this.

This feature flag has been renamed to OIDCProviderUnmanagedClusters

api/v1beta2/awscluster_types.go

richardcase · 2025-01-31T14:59:05Z

cmd/clusterawsadm/cloudformation/bootstrap/cluster_api_controller.go

@@ -185,6 +185,11 @@ func (t Template) ControllersPolicy() *iamv1.PolicyDocument {
 				"ec2:DeleteLaunchTemplateVersions",
 				"ec2:DescribeKeyPairs",
 				"ec2:ModifyInstanceMetadataOptions",
+				"iam:CreateOpenIDConnectProvider",


As the OIDC feature flag is false by default we may want to not include these extra permissions by default. And instead only add them if enabled via the config file.

This has been fixed in the latest commit.

richardcase · 2025-01-31T15:02:50Z

controllers/awscluster_controller.go

@@ -256,6 +267,14 @@ func (r *AWSClusterReconciler) reconcileDelete(ctx context.Context, clusterScope
 		allErrs = append(allErrs, errors.Wrap(err, "error deleting network"))
 	}

+	if err := iamService.DeleteOIDCProvider(ctx); err != nil {


Does this need to be dependent on the feature flag?

I added a feature flag gate here, and in the ReconcileNormal

richardcase · 2025-01-31T15:08:21Z

docs/book/src/topics/irsa-and-oidc-provider.md

+
+### 4 - Set Service Account Issuer URL in KubeadmControlPlane Configuration
+
+The KubeadmControlPlane configuration should be updated to set the apiServer `service-account-issuer` argument with the S3 buckets URL:


It might also be nice to create a new cluster flavor (i.e. template) in the templates directory.

A new cluster flavor has been added.

Although there is one gotcha - because I've included ClusterResourceSet for aws-pod-identity-webhook I also had to add cert-manager to allow the formers Webhooks to work.
The cert-manager CRD blows out the kubectl-last-applied annotation limit so the manifest must be applied with --server-side flag. e.g.

clusterctl generate cluster capa-oidc-test --from=templates/cluster-template-oidc-provider-unmanaged.yaml | k apply -f - --server-side

I was tempted to make this a HelmChartProxy addon instead, but that introduces a dependency on CAAPH.

richardcase · 2025-01-31T15:15:22Z

controllers/awscluster_controller.go

@@ -352,6 +372,12 @@ func (r *AWSClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScope)
 	}
 	conditions.MarkTrue(awsCluster, infrav1.S3BucketReadyCondition)

+	if err := iamService.ReconcileOIDCProvider(context.TODO()); err != nil {


Should we wrap this with a test of the feature being enabled to be explicit? I know the validating webook shouldn't allow setting the associate field when the feature flag is disabled. Undecided about this if i'm honest ;)

richardcase · 2025-01-31T15:16:44Z

pkg/cloud/services/iam/iam.go

+// 4. copy Service Account public signing JWKs to the s3 bucket.
+func (s *Service) ReconcileOIDCProvider(ctx context.Context) error {
+	if !s.scope.AssociateOIDCProvider() {
+		return nil


Could be good to have a log entry to say its disabled at a high log verbosity level for debugging purposes.

I added:

log.V(4).Info("OIDC provider association is disabled, skipping reconciliation")

richardcase · 2025-01-31T15:23:03Z

controllers/awsmachine_controller.go

@@ -673,6 +673,50 @@ func (r *AWSMachineReconciler) reconcileOperationalState(ec2svc services.EC2Inte
 		return err
 	}

+	// check if the remote kubeconfig works and annotate the cluster
+	if _, ok := machineScope.InfraCluster.InfraCluster().GetAnnotations()[scope.KubeconfigReadyAnnotation]; !ok && machineScope.IsControlPlane() {


I'm not sure about this as the mechanism to decide if we carry on with the OIDC reconciliation.

Added a suggestion in the awscluster_controller.go

This has been removed.

richardcase · 2025-01-31T15:36:29Z

controllers/awscluster_controller.go

+	if err := iamService.ReconcileOIDCProvider(context.TODO()); err != nil {
+		conditions.MarkFalse(awsCluster, infrav1.OIDCProviderReadyCondition, infrav1.OIDCProviderReconciliationFailedReason, clusterv1.ConditionSeverityError, "%s", err.Error())
+		clusterScope.Error(err, "failed to reconcile OIDC provider")
+		return reconcile.Result{RequeueAfter: 15 * time.Second}, nil


You could move if err := iamService.ReconcileOIDCProvider(context.TODO()); err != nil { to after awsCluster.Status.Ready = true and then within iamService.ReconcileOIDCProvider instead return a ctrl.Result so that you can say RequeueAfter if the downstream cluster isn't available yet. This would allow provisioning to continue for the control plane and then when the cluster is accessible iamService.ReconcileOIDCProvider would do the second part and deploy the config map and return an empty ctrl.Result so there is no requeue.

The benefit is this removes the need for the AWSMachines controller annotating AWSCluster and keeps everything it the control of the AWSCluster controller.

Hopefully that all makes sense. If it doesn't ping me on slack and we can chat or arrange a call.

I implemented your suggestion, with a slight modification.
In the IAM reconciler I change the way we build the jwks document, so that it can be done entirely locally. We already have the Service Account signing key in the <cluster>-sa secret, so I added logic to build the JWKS keyset in code instead of reaching across to the workload cluster to retrieve it. I lifted code from the kubernetes codebase to calculate key hashes etc, so the result is identical to what ends up installed in the cluster.

This means IdentityProvider and S3 objects can be fully provisioned before the workload cluster is available, so I removed the ready check + pause, and therefore didn't end up returning a ctrl.Result

I'm tempted to also remove the creation of the boilerplate-oidc-trust-policy ConfigMap on the remote cluster because I'm not really sure what value it provides?!

STASiAN · 2025-03-21T08:29:46Z

@sl1pm4t do you need help with rebase?

sl1pm4t · 2025-03-21T10:10:07Z

Yes @STASiAN I haven't had time to rebase and address the PR review comments.
If you want to contribute that would be welcome.

sl1pm4t · 2025-04-28T00:01:49Z

I'm picking this back up today. Will hopefully be able to push changes to address the review comments this week.

richardcase · 2025-04-28T11:02:21Z

Thats will be great @sl1pm4t , thank you. I was chatting with someone on Friday and they are really looking forward to this feature.

sl1pm4t · 2025-04-30T20:53:35Z

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

sl1pm4t · 2025-05-02T00:59:08Z

/retest-required

sl1pm4t · 2025-05-04T23:05:25Z

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

sl1pm4t · 2025-05-05T01:38:12Z

@richardcase I think this is ready for re-review. I have rebased and I think I have addressed your review comments.
I have not squashed the commits yet so it is easier for re-review.

sl1pm4t · 2025-05-05T01:42:07Z

/test pull-cluster-api-provider-aws-e2e-eks

Fix s3 tests

Co-authored-by: Richard Case <[email protected]> Make OIDC IAM permissions optional rename feature to distinguish from managed mode Rework OIDC reconcile to work mgmt side only Fixes and tidy up Add OIDC provider flavor use keyIDFromPublicKey from k8s code Gate OIDC reconcile behind feature flag. Revert unneeded awsmachine_controller test changes fix clusterawsadm template tests Fix lints & tests bump pod ID webhook to latest (v0.6.6) make generate Re-order reconcile steps. The Identity provider needs the S3 objects, so let's create them first and avoid flakey failures. Fixes after rebase

sl1pm4t · 2025-05-15T19:55:30Z

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks

lukasmrtvy · 2025-05-29T11:21:53Z

friendly ping

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 28, 2024

k8s-ci-robot requested review from Ankitasw and killianmuldoon August 28, 2024 02:45

k8s-ci-robot added needs-priority needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 28, 2024

sl1pm4t changed the title ~~feat: Add IRSA support for self-managed clusters (rebase)~~ ✨ feat: Add IRSA support for self-managed clusters (rebase) Aug 28, 2024

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 Aug 28, 2024

sl1pm4t force-pushed the awscluster-irsa-rebase branch 2 times, most recently from 184576c to 986add5 Compare August 29, 2024 07:26

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 986add5 to d265b48 Compare September 6, 2024 07:43

sl1pm4t force-pushed the awscluster-irsa-rebase branch from d265b48 to 909a464 Compare September 30, 2024 01:53

sl1pm4t mentioned this pull request Oct 15, 2024

Add IRSA Support to Self Managed Clusters #4094

Closed

4 tasks

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

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 909a464 to 8d30f71 Compare October 21, 2024 19:44

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

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 8d30f71 to 540420c Compare November 1, 2024 02:27

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

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 540420c to 71a97d5 Compare January 28, 2025 19:40

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

richardcase reviewed Jan 31, 2025

View reviewed changes

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

sl1pm4t force-pushed the awscluster-irsa-rebase branch 2 times, most recently from 4456a29 to eeed105 Compare April 30, 2025 05:00

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

feat: Add IRSA support for self-managed.

ad81054

Fix s3 tests

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 406a61e to 4bc93b4 Compare May 15, 2025 01:03

sl1pm4t force-pushed the awscluster-irsa-rebase branch from 4bc93b4 to 809d4ed Compare May 15, 2025 03:19


		### 4 - Set Service Account Issuer URL in KubeadmControlPlane Configuration

		The KubeadmControlPlane configuration should be updated to set the apiServer `service-account-issuer` argument with the S3 buckets URL:

✨ feat: Add IRSA support for self-managed clusters (rebase) #5109

Are you sure you want to change the base?

✨ feat: Add IRSA support for self-managed clusters (rebase) #5109

Uh oh!

Conversation

sl1pm4t commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 28, 2024

Uh oh!

luthermonson commented Aug 28, 2024

Uh oh!

sl1pm4t commented Aug 30, 2024

Uh oh!

sl1pm4t commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sl1pm4t commented Sep 25, 2024

Uh oh!

richardcase commented Sep 30, 2024

Uh oh!

sl1pm4t commented Oct 22, 2024

Uh oh!

richardcase commented Nov 4, 2024

Uh oh!

STASiAN commented Jan 28, 2025

Uh oh!

sl1pm4t commented Jan 28, 2025

Uh oh!

k8s-ci-robot commented Jan 28, 2025

Uh oh!

sl1pm4t commented Jan 28, 2025

Uh oh!

richardcase commented Jan 29, 2025

Uh oh!

sl1pm4t commented Jan 29, 2025

Uh oh!

richardcase commented Jan 31, 2025

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!

sl1pm4t Apr 30, 2025 • 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!

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!

sl1pm4t Apr 30, 2025 • 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!

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!

sl1pm4t commented Aug 28, 2024 •

edited

Loading

sl1pm4t commented Sep 3, 2024 •

edited

Loading

sl1pm4t Apr 30, 2025 •

edited

Loading

sl1pm4t Apr 30, 2025 •

edited

Loading

sl1pm4t Apr 30, 2025 •

edited

Loading

sl1pm4t commented Apr 28, 2025 •

edited

Loading

sl1pm4t commented May 5, 2025 •

edited

Loading