Simplify IRSA to S3 + OIDC provider

* build oidc discovery doc * remove pod-identity-webhook deployment * Add documentation for SA Issuer Discovery & IRSA * remove unused reconcileKubeAPIParameters()
kubernetes-sigs · Aug 21, 2024 · c9dd099 · c9dd099
1 parent 659e0e7
commit c9dd099
Show file tree

Hide file tree

Showing 26 changed files with 260 additions and 2,484 deletions.
diff --git a/api/v1beta2/awscluster_spec.go b/api/v1beta2/awscluster_spec.go
@@ -0,0 +1,22 @@
+package v1beta2
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"sigs.k8s.io/cluster-api-provider-aws/v2/feature"
+)
+
+// Validate will validate the spec fields.
+func (s *AWSClusterSpec) Validate() []*field.Error {
+	var errs field.ErrorList
+
+	// Check the feature gate is enabled for OIDC Provider.
+	if s.AssociateOIDCProvider && !feature.Gates.Enabled(feature.OIDCProviderSupport) {
+		errs = append(errs,
+			field.Forbidden(field.NewPath("spec", "associateOIDCProvider"),
+				"can be enabled only if the OIDCProviderSupport feature gate is enabled"),
+		)
+		return errs
+	}
+
+	return errs
+}
diff --git a/api/v1beta2/awscluster_types.go b/api/v1beta2/awscluster_types.go
@@ -106,15 +106,14 @@ type AWSClusterSpec struct {
 	IdentityRef *AWSIdentityReference `json:"identityRef,omitempty"`
 
 	// S3Bucket contains options to configure a supporting S3 bucket for this
-	// cluster - Used  for nodes requiring Ignition
-	// (https://coreos.github.io/ignition/) for bootstrapping (requires
-	// BootstrapFormatIgnition feature flag to be enabled) and for storing OIDC endpoint
-	// certificates for use with IRSA
+	// cluster - Used for nodes requiring Ignition (https://coreos.github.io/ignition/) for bootstrapping (requires
+	// BootstrapFormatIgnition feature flag to be enabled) and/or for storing OIDC endpoint certificates for use
+	// with IRSA (requires OIDCProviderSupport feature flag to be enabled).
 	// +optional
 	S3Bucket *S3Bucket `json:"s3Bucket,omitempty"`
 
-	// AssociateOIDCProvider can be enabled to automatically create an identity
-	// provider and install the pod identity webhook from AWS for use with IRSA.
+	// AssociateOIDCProvider can be enabled to automatically publish the clusters Service Account issuer OIDC discovery
+	// documents to S3, create an AWS IAM OIDC identity provider and configure it to trust the cluster issuer.
 	// This will only work if the S3Bucket is configured properly.
 	// +kubebuilder:default=false
 	AssociateOIDCProvider bool `json:"associateOIDCProvider,omitempty"`

diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go
@@ -53,6 +53,7 @@ var (
 func (r *AWSCluster) ValidateCreate() (admission.Warnings, error) {
 	var allErrs field.ErrorList
 
+	allErrs = append(allErrs, r.Spec.Validate()...)
 	allErrs = append(allErrs, r.Spec.Bastion.Validate()...)
 	allErrs = append(allErrs, r.validateSSHKeyName()...)
 	allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...)

diff --git a/api/v1beta2/s3bucket.go b/api/v1beta2/s3bucket.go
@@ -38,9 +38,9 @@ func (b *S3Bucket) Validate() []*field.Error {
 	}
 
 	// Feature gate is not enabled but ignition is enabled then send a forbidden error.
-	if !feature.Gates.Enabled(feature.BootstrapFormatIgnition) {
+	if !feature.Gates.Enabled(feature.BootstrapFormatIgnition) && !feature.Gates.Enabled(feature.OIDCProviderSupport) {
 		errs = append(errs, field.Forbidden(field.NewPath("spec", "s3Bucket"),
-			"can be set only if the BootstrapFormatIgnition feature gate is enabled"))
+			"can be set only if the BootstrapFormatIgnition or OIDCProviderSupport feature gate is enabled"))
 	}
 
 	if b.PresignedURLDuration == nil {

diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
@@ -928,10 +928,10 @@ spec:
                 type: object
               associateOIDCProvider:
                 default: false
-                description: AssociateOIDCProvider can be enabled to automatically
-                  create an identity provider and install the pod identity webhook
-                  from AWS for use with IRSA. This will only work if the S3Bucket
-                  is configured properly.
+                description: |-
+                  AssociateOIDCProvider can be enabled to automatically publish the clusters Service Account issuer OIDC discovery
+                  documents to S3, create an AWS IAM OIDC identity provider and configure it to trust the cluster issuer.
+                  This will only work if the S3Bucket is configured properly.
                 type: boolean
               bastion:
                 description: Bastion contains options to configure the bastion host.
@@ -1754,10 +1754,9 @@ spec:
               s3Bucket:
                 description: |-
                   S3Bucket contains options to configure a supporting S3 bucket for this
-                  cluster - currently used for nodes requiring Ignition
-                  (https://coreos.github.io/ignition/) for bootstrapping (requires
-                  BootstrapFormatIgnition feature flag to be enabled) and for storing OIDC 
-                  endpoint certificates for use with IRSA.
+                  cluster - Used for nodes requiring Ignition (https://coreos.github.io/ignition/) for bootstrapping (requires
+                  BootstrapFormatIgnition feature flag to be enabled) and/or for storing OIDC endpoint certificates for use
+                  with IRSA (requires OIDCProviderSupport feature flag to be enabled).
                 properties:
                   bestEffortDeleteObjects:
                     description: BestEffortDeleteObjects defines whether access/permission

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
@@ -518,10 +518,10 @@ spec:
                         type: object
                       associateOIDCProvider:
                         default: false
-                        description: AssociateOIDCProvider can be enabled to automatically
-                          create an identity provider and install the pod identity
-                          webhook from AWS for use with IRSA. This will only work
-                          if the S3Bucket is configured properly.
+                        description: |-
+                          AssociateOIDCProvider can be enabled to automatically publish the clusters Service Account issuer OIDC discovery
+                          documents to S3, create an AWS IAM OIDC identity provider and configure it to trust the cluster issuer.
+                          This will only work if the S3Bucket is configured properly.
                         type: boolean
                       bastion:
                         description: Bastion contains options to configure the bastion
@@ -1354,10 +1354,9 @@ spec:
                       s3Bucket:
                         description: |-
                           S3Bucket contains options to configure a supporting S3 bucket for this
-                          cluster - currently used for nodes requiring Ignition
-                          (https://coreos.github.io/ignition/) for bootstrapping (requires
-                          BootstrapFormatIgnition feature flag to be enabled) and
-                          for storing OIDC endpoint certificates for use with IRSA.
+                          cluster - Used for nodes requiring Ignition (https://coreos.github.io/ignition/) for bootstrapping (requires
+                          BootstrapFormatIgnition feature flag to be enabled) and/or for storing OIDC endpoint certificates for use
+                          with IRSA (requires OIDCProviderSupport feature flag to be enabled).
                         properties:
                           bestEffortDeleteObjects:
                             description: BestEffortDeleteObjects defines whether access/permission

diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml
@@ -19,7 +19,7 @@ spec:
       containers:
       - args:
         - "--leader-elect"
-        - "--feature-gates=EKS=${CAPA_EKS:=true},EKSEnableIAM=${CAPA_EKS_IAM:=false},EKSAllowAddRoles=${CAPA_EKS_ADD_ROLES:=false},EKSFargate=${EXP_EKS_FARGATE:=false},MachinePool=${EXP_MACHINE_POOL:=false},EventBridgeInstanceState=${EVENT_BRIDGE_INSTANCE_STATE:=false},AutoControllerIdentityCreator=${AUTO_CONTROLLER_IDENTITY_CREATOR:=true},BootstrapFormatIgnition=${EXP_BOOTSTRAP_FORMAT_IGNITION:=false},ExternalResourceGC=${EXP_EXTERNAL_RESOURCE_GC:=false},AlternativeGCStrategy=${EXP_ALTERNATIVE_GC_STRATEGY:=false},TagUnmanagedNetworkResources=${TAG_UNMANAGED_NETWORK_RESOURCES:=true},ROSA=${EXP_ROSA:=false}"
+        - "--feature-gates=EKS=${CAPA_EKS:=true},EKSEnableIAM=${CAPA_EKS_IAM:=false},EKSAllowAddRoles=${CAPA_EKS_ADD_ROLES:=false},EKSFargate=${EXP_EKS_FARGATE:=false},MachinePool=${EXP_MACHINE_POOL:=false},EventBridgeInstanceState=${EVENT_BRIDGE_INSTANCE_STATE:=false},AutoControllerIdentityCreator=${AUTO_CONTROLLER_IDENTITY_CREATOR:=true},BootstrapFormatIgnition=${EXP_BOOTSTRAP_FORMAT_IGNITION:=false},OIDCProviderSupport=${EXP_OIDC_PROVIDER_SUPPORT:=false},ExternalResourceGC=${EXP_EXTERNAL_RESOURCE_GC:=false},AlternativeGCStrategy=${EXP_ALTERNATIVE_GC_STRATEGY:=false},TagUnmanagedNetworkResources=${TAG_UNMANAGED_NETWORK_RESOURCES:=true},ROSA=${EXP_ROSA:=false}"
         - "--v=${CAPA_LOGLEVEL:=0}"
         - "--diagnostics-address=${CAPA_DIAGNOSTICS_ADDRESS:=:8443}"
         - "--insecure-diagnostics=${CAPA_INSECURE_DIAGNOSTICS:=false}"

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -442,22 +442,3 @@ rules:
   - get
   - patch
   - update
-- apiGroups:
-  - cert-manager.io
-  resources:
-  - certificates
-  - issuers
-  verbs:
-    - create
-    - get
-    - list
-    - delete
-- apiGroups:
-    - controlplane.cluster.x-k8s.io
-  resources:
-    - kubeadmcontrolplanes
-  verbs:
-    - get
-    - list
-    - patch
-    - update
diff --git a/controllers/awscluster_controller.go b/controllers/awscluster_controller.go
@@ -385,26 +385,6 @@ func (r *AWSClusterReconciler) reconcileNormal(ctx context.Context, clusterScope
 		return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
 	}
 
-	if awsCluster.Status.Network.APIServerELB.DNSName == "" {
-		conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameReason, clusterv1.ConditionSeverityInfo, "")
-		clusterScope.Info("Waiting on API server ELB DNS name")
-		return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
-	}
-
-	clusterScope.Debug("looking up IP address for DNS", "dns", awsCluster.Status.Network.APIServerELB.DNSName)
-	if _, err := net.LookupIP(awsCluster.Status.Network.APIServerELB.DNSName); err != nil {
-		clusterScope.Error(err, "failed to get IP address for dns name", "dns", awsCluster.Status.Network.APIServerELB.DNSName)
-		conditions.MarkFalse(awsCluster, infrav1.LoadBalancerReadyCondition, infrav1.WaitForDNSNameResolveReason, clusterv1.ConditionSeverityInfo, "")
-		clusterScope.Info("Waiting on API server ELB DNS name to resolve")
-		return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
-	}
-	conditions.MarkTrue(awsCluster, infrav1.LoadBalancerReadyCondition)
-
-	awsCluster.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{
-		Host: awsCluster.Status.Network.APIServerELB.DNSName,
-		Port: clusterScope.APIServerPort(),
-	}
-
 	for _, subnet := range clusterScope.Subnets().FilterPrivate() {
 		found := false
 		for _, az := range awsCluster.Status.Network.APIServerELB.AvailabilityZones {

diff --git a/controllers/awscluster_controller_test.go b/controllers/awscluster_controller_test.go
@@ -269,7 +269,7 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
 				IsPublic:         false,
 			},
 		})
-		_, err = reconciler.reconcileNormal(context.TODO(), cs)
+		_, err = reconciler.reconcileNormal(ctx, cs)
 		g.Expect(err).To(BeNil())
 		g.Expect(cs.VPC().ID).To(Equal("vpc-exists"))
 		expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{
@@ -376,7 +376,7 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
 				IsPublic:         false,
 			},
 		})
-		_, err = reconciler.reconcileNormal(context.TODO(), cs)
+		_, err = reconciler.reconcileNormal(ctx, cs)
 		g.Expect(err).To(BeNil())
 		g.Expect(cs.VPC().ID).To(Equal("vpc-exists"))
 		expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{
@@ -565,7 +565,7 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
 			return ec2Svc
 		}
 
-		_, err = reconciler.reconcileNormal(context.TODO(), cs)
+		_, err = reconciler.reconcileNormal(ctx, cs)
 		g.Expect(err.Error()).To(ContainSubstring("The maximum number of VPCs has been reached"))
 
 		err = reconciler.reconcileDelete(ctx, cs)

diff --git a/controllers/awscluster_controller_unit_test.go b/controllers/awscluster_controller_unit_test.go
@@ -244,7 +244,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 						IsPublic:         false,
 					},
 				})
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionTrue, "", ""}})
 				g.Expect(awsCluster.GetFinalizers()).To(ContainElement(infrav1.ClusterFinalizer))
@@ -299,7 +299,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).Should(Equal(expectedErr))
 			})
 			t.Run("Should fail AWSCluster create with ClusterSecurityGroupsReadyCondition status false", func(t *testing.T) {
@@ -320,7 +320,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.ClusterSecurityGroupsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.ClusterSecurityGroupReconciliationFailedReason}})
 			})
@@ -343,7 +343,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.BastionHostReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.BastionHostFailedReason}})
 			})
@@ -367,7 +367,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).ToNot(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityWarning, infrav1.LoadBalancerFailedReason}})
 			})
@@ -391,7 +391,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 					},
 				)
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameReason}})
 			})
@@ -416,7 +416,7 @@ func TestAWSClusterReconcileOperations(t *testing.T) {
 				)
 				awsCluster.Status.Network.APIServerELB.DNSName = "test-apiserver.us-east-1.aws"
 				g.Expect(err).To(BeNil())
-				_, err = reconciler.reconcileNormal(context.TODO(), cs)
+				_, err = reconciler.reconcileNormal(ctx, cs)
 				g.Expect(err).To(BeNil())
 				expectAWSClusterConditions(g, cs.AWSCluster, []conditionAssertion{{infrav1.LoadBalancerReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityInfo, infrav1.WaitForDNSNameResolveReason}})
 			})

diff --git a/controllers/awsmachine_controller_test.go b/controllers/awsmachine_controller_test.go
@@ -19,6 +19,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"k8s.io/utils/ptr"
 	"testing"
 	"time"
 
@@ -455,7 +456,7 @@ func getMachineScope(cs *scope.ClusterScope, awsMachine *infrav1.AWSMachine) (*s
 				},
 				Spec: clusterv1.MachineSpec{
 					Bootstrap: clusterv1.Bootstrap{
-						DataSecretName: aws.String("bootstrap-data"),
+						DataSecretName: ptr.To[string]("bootstrap-data"),
 					},
 				},
 			},

diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go
@@ -136,7 +136,7 @@ func TestAWSMachineReconciler(t *testing.T) {
 					Spec: clusterv1.MachineSpec{
 						ClusterName: "capi-test",
 						Bootstrap: clusterv1.Bootstrap{
-							DataSecretName: aws.String("bootstrap-data"),
+							DataSecretName: ptr.To[string]("bootstrap-data"),
 						},
 					},
 				},