applied a small number of refactorings as I read through the code

Author: Skarlso

api/v1beta1/zz_generated.conversion.go

@@ -173,3 +173,11 @@ const (
 	// S3BucketFailedReason is used when any errors occur during reconciliation of an S3 bucket.
 	S3BucketFailedReason = "S3BucketCreationFailed"
 )
+
+const (
+	// OIDCProviderReadyCondition indicates that the OIDC provider has been created successfully.
+	OIDCProviderReadyCondition = "OIDCProviderCreated"
+
+	// OIDCProviderReconciliationFailedReason is used if we can't reconcile the OIDC provider.
+	OIDCProviderReconciliationFailedReason = "OIDCProviderReconciliationFailed"
+)

api/v1beta2/conditions_consts.go

@@ -337,6 +337,7 @@ func (r *AWSClusterReconciler) reconcileNormal(ctx context.Context, clusterScope
 	}
 
 	if err := iamService.ReconcileOIDCProvider(ctx); err != nil {
+		conditions.MarkFalse(awsCluster, infrav1.OIDCProviderReadyCondition, infrav1.OIDCProviderReconciliationFailedReason, clusterv1.ConditionSeverityError, err.Error())
 		clusterScope.Error(err, "failed to reconcile OIDC provider")
 		return reconcile.Result{RequeueAfter: 15 * time.Second}, nil
 	}

controllers/awscluster_controller.go

@@ -309,7 +309,7 @@ func (r *AWSMachineReconciler) reconcileDelete(machineScope *scope.MachineScope,
 	}
 
 	instance, err := r.findInstance(machineScope, ec2Service)
-	if err != nil && err != ec2.ErrInstanceNotFoundByID {
+	if err != nil && !errors.Is(err, ec2.ErrInstanceNotFoundByID) {
 		machineScope.Error(err, "query to find instance failed")
 		return ctrl.Result{}, err
 	}
@@ -651,8 +651,7 @@ func (r *AWSMachineReconciler) reconcileOperationalState(ctx context.Context, ec
 	}
 
 	// check if the remote kubeconfig works and annotate the cluster
-	_, ok := machineScope.InfraCluster.InfraCluster().GetAnnotations()[scope.KubeconfigReadyAnnotation]
-	if !ok && machineScope.IsControlPlane() {
+	if _, ok := machineScope.InfraCluster.InfraCluster().GetAnnotations()[scope.KubeconfigReadyAnnotation]; !ok && machineScope.IsControlPlane() {
 		// if a control plane node is operational check for a kubeconfig and a working control plane node
 		// and set the annotation so any reconciliation which requires workload api access can complete
 		remoteClient, err := machineScope.InfraCluster.RemoteClient()
@@ -669,23 +668,29 @@ func (r *AWSMachineReconciler) reconcileOperationalState(ctx context.Context, ec
 		for i := range nodes.Items {
 			if util.IsNodeReady(&nodes.Items[i]) {
 				oneReady = true // if one control plane is ready return true
+				break
 			}
 		}
 
-		if oneReady {
-			awsCluster := &infrav1.AWSCluster{}
-			key := types.NamespacedName{Namespace: machineScope.InfraCluster.Namespace(), Name: machineScope.InfraCluster.Name()}
-			if err := r.Client.Get(ctx, key, awsCluster); err != nil {
-				return err
-			}
-			anno := awsCluster.GetAnnotations()
-			anno[scope.KubeconfigReadyAnnotation] = "true"
-			awsCluster.SetAnnotations(anno)
-			if err := r.Client.Update(ctx, awsCluster); err != nil {
-				return err
-			}
-		} else {
+		if !oneReady {
 			r.Log.Info("waiting for a control plane node to be ready before annotating the cluster, do you need to deploy a CNI?")
+
+			return nil
+		}
+
+		awsCluster := &infrav1.AWSCluster{}
+		key := types.NamespacedName{
+			Namespace: machineScope.InfraCluster.Namespace(),
+			Name:      machineScope.InfraCluster.Name(),
+		}
+		if err := r.Client.Get(ctx, key, awsCluster); err != nil {
+			return fmt.Errorf("failed to get aws cluster: %w", err)
+		}
+
+		awsCluster.Annotations[scope.KubeconfigReadyAnnotation] = "true"
+
+		if err := r.Client.Update(ctx, awsCluster); err != nil {
+			return fmt.Errorf("failed to update aws cluster with new annotation: %w", err)
 		}
 	}
 
@@ -1206,19 +1211,19 @@ func (r *AWSMachineReconciler) ensureStorageTags(ec2svc services.EC2Interface, i
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			prevAnnotations[volumeID] = newAnnotation
+			annotations[volumeID] = newAnnotation
 		} else {
 			newAnnotation, err := r.ensureVolumeTags(ec2svc, aws.String(volumeID), make(map[string]interface{}), additionalTags)
 			if err != nil {
 				r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 			}
-			prevAnnotations[volumeID] = newAnnotation
+			annotations[volumeID] = newAnnotation
 		}
 	}
 
 	if !cmp.Equal(prevAnnotations, annotations, cmpopts.EquateEmpty()) {
 		// We also need to update the annotation if anything changed.
-		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, prevAnnotations)
+		err = r.updateMachineAnnotationJSON(machine, VolumeTagsLastAppliedAnnotation, annotations)
 		if err != nil {
 			r.Log.Error(err, "Failed to fetch the changed volume tags in EC2 instance")
 		}

controllers/awsmachine_controller.go

@@ -3,13 +3,16 @@ package iam
 import (
 	"context"
 	"errors"
+	"fmt"
 	"regexp"
 
 	"github.com/aws/aws-sdk-go/service/iam/iamiface"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/services/s3"
+	"sigs.k8s.io/cluster-api/util/conditions"
 )
 
 type Service struct {
@@ -57,11 +60,11 @@ var (
 // ReconcileOIDCProvider replicates functionality already built into managed clusters by auto-deploying the
 // modifying kube-apiserver args, deploying the pod identity webhook and setting/configuring an oidc provider
 // for more details see: https://github.com/aws/amazon-eks-pod-identity-webhook/blob/master/SELF_HOSTED_SETUP.md
-// 1. create a self signed issuer for the mutating webhook
+// 1. create a self-signed issuer for the mutating webhook
 // 2. add create a json patch for kube-apiserver and use capi config to add to the kubeadm.yml
 // 3. create an oidc provider in aws which points to the s3 bucket
-// 4. pause until kubeconfig and cluster acccess is ready
-// 5. move openid config and jwks to the s3 bucket
+// 4. pause until kubeconfig and cluster access is ready
+// 5. move openid config and JWKs to the s3 bucket
 // 6. add the pod identity webhook to the workload cluster
 // 7. add the configmap to the workload cluster.
 func (s *Service) ReconcileOIDCProvider(ctx context.Context) error {
@@ -104,7 +107,13 @@ func (s *Service) ReconcileOIDCProvider(ctx context.Context) error {
 		return err
 	}
 
-	return s.reconcileTrustPolicyConfigMap(ctx)
+	if err := s.reconcileTrustPolicyConfigMap(ctx); err != nil {
+		return fmt.Errorf("failed to reconcile trust policy config map: %w", err)
+	}
+
+	conditions.MarkTrue(s.scope.InfraCluster(), infrav1.OIDCProviderReadyCondition)
+
+	return nil
 }
 
 // DeleteOIDCProvider will delete the iam resources note that the bucket is cleaned up in the s3 service

pkg/cloud/services/iam/iam.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"crypto/sha1"
 	"crypto/tls"
+	stderr "errors"
 	"fmt"
 	"path"
 	"strings"
@@ -247,29 +248,31 @@ func findAndVerifyOIDCProvider(issuerURL, thumbprint string, iamClient iamiface.
 	return "", nil
 }
 
-func fetchRootCAThumbprint(url string, port int) (ret string, err error) {
+func fetchRootCAThumbprint(url string, port int) (_ string, err error) {
 	// Parse cmdline arguments using flag package
 	conn, err := tls.Dial("tcp", fmt.Sprintf("%s:%d", url, port), &tls.Config{
 		MinVersion: tls.VersionTLS12,
 	})
 	if err != nil {
-		return
+		return "", err
 	}
+
 	defer func() {
-		err = conn.Close()
+		if cerr := conn.Close(); cerr != nil {
+			err = stderr.Join(err, cerr)
+		}
 	}()
-
 	// Get the ConnectionState struct as that's the one which gives us x509.Certificate struct
 	cert := conn.ConnectionState().PeerCertificates[0]
 	fingerprint := sha1.Sum(cert.Raw) //nolint:gosec // this is not used for real security
 	var buf bytes.Buffer
 	for _, f := range fingerprint {
-		if _, err = fmt.Fprintf(&buf, "%02X", f); err != nil {
-			return
+		if _, err := fmt.Fprintf(&buf, "%02X", f); err != nil {
+			return "", err
 		}
 	}
-	ret = strings.ToLower(buf.String())
-	return
+
+	return strings.ToLower(buf.String()), nil
 }
 
 // DeleteOIDCProvider will delete an OIDC provider.

pkg/cloud/services/iam/oidc.go

@@ -319,8 +319,8 @@ func objectMeta(name, namespace string) metav1.ObjectMeta {
 	return meta
 }
 
-// reconcileCertifcateSecret takes a secret and moves it to the workload cluster.
-func reconcileCertifcateSecret(ctx context.Context, cert *corev1.Secret, remoteClient client.Client) error {
+// reconcileCertificateSecret takes a secret and moves it to the workload cluster.
+func reconcileCertificateSecret(ctx context.Context, cert *corev1.Secret, remoteClient client.Client) error {
 	// check if the secret was created by cert-manager
 	certCheck := &corev1.Secret{}
 	if err := remoteClient.Get(ctx, types.NamespacedName{

pkg/cloud/services/iam/podidentitywebhook.go

@@ -43,7 +43,7 @@ func (s *Service) reconcilePodIdentityWebhook(ctx context.Context) error {
 	}
 
 	// switch it to kube-system and move it to the remote cluster
-	if err := reconcileCertifcateSecret(ctx, certSecret, remoteClient); err != nil {
+	if err := reconcileCertificateSecret(ctx, certSecret, remoteClient); err != nil {
 		return err
 	}
 

pkg/cloud/services/iam/reconcilers.go

@@ -49,19 +49,19 @@ type Service struct {
 var DisabledError = errors.New("s3 management disabled")
 
 func IsDisabledError(err error) bool {
-	return err == DisabledError
+	return errors.Is(err, DisabledError)
 }
 
 var EmptyBucketError = errors.New("empty bucket name")
 
 func IsEmptyBucketError(err error) bool {
-	return err == EmptyBucketError
+	return errors.Is(err, EmptyBucketError)
 }
 
 var EmptyKeyError = errors.New("empty key")
 
 func IsEmptyKeyError(err error) bool {
-	return err == EmptyKeyError
+	return errors.Is(err, EmptyKeyError)
 }
 
 // NewService returns a new service given the api clients.
@@ -123,7 +123,8 @@ func (s *Service) DeleteBucket() error {
 		return nil
 	}
 
-	aerr, ok := err.(awserr.Error)
+	var aerr awserr.Error
+	ok := errors.As(err, &aerr)
 	if !ok {
 		return errors.Wrap(err, "deleting S3 bucket")
 	}
@@ -186,10 +187,10 @@ func (s *Service) create(putInput *s3.PutObjectInput) (string, error) {
 	}
 
 	if exp := s.scope.Bucket().PresignedURLDuration; exp != nil {
-		s.scope.Info("Generating presigned URL", "bucket_name", bucket, "key", key)
+		s.scope.Info("Generating presigned URL", "bucket_name", aws.StringValue(putInput.Bucket), "key", aws.StringValue(putInput.Key))
 		req, _ := s.S3Client.GetObjectRequest(&s3.GetObjectInput{
-			Bucket: aws.String(bucket),
-			Key:    aws.String(key),
+			Bucket: putInput.Bucket,
+			Key:    putInput.Key,
 		})
 		return req.Presign(exp.Duration)
 	}
@@ -228,7 +229,8 @@ func (s *Service) Delete(key string) error {
 		return nil
 	}
 
-	aerr, ok := err.(awserr.Error)
+	var aerr awserr.Error
+	ok := errors.As(err, &aerr)
 	if !ok {
 		return errors.Wrap(err, "deleting S3 object")
 	}
@@ -255,7 +257,8 @@ func (s *Service) createBucketIfNotExist(bucketName string) error {
 		return nil
 	}
 
-	aerr, ok := err.(awserr.Error)
+	var aerr awserr.Error
+	ok := errors.As(err, &aerr)
 	if !ok {
 		return errors.Wrap(err, "creating S3 bucket")
 	}
@@ -272,11 +275,10 @@ func (s *Service) createBucketIfNotExist(bucketName string) error {
 }
 
 func (s *Service) ensureBucketAccess(bucketName string) error {
-	f := false
 	input := &s3.PutPublicAccessBlockInput{
 		Bucket: aws.String(bucketName),
 		PublicAccessBlockConfiguration: &s3.PublicAccessBlockConfiguration{
-			BlockPublicAcls: aws.Bool(f),
+			BlockPublicAcls: aws.Bool(false),
 		},
 	}
 

pkg/cloud/services/s3/s3.go

@@ -299,7 +299,6 @@ func TestReconcileBucket(t *testing.T) {
 
 			s3Mock.EXPECT().CreateBucket(gomock.Any()).Return(nil, nil).Times(1)
 			s3Mock.EXPECT().PutBucketTagging(gomock.Any()).Return(nil, nil).Times(1)
-			s3Mock.EXPECT().PutBucketPolicy(gomock.Any()).Return(nil, errors.New("error")).Times(1)
 			s3Mock.EXPECT().PutPublicAccessBlock(gomock.Any()).Return(nil, errors.New("error")).Times(1)
 
 			if err := svc.ReconcileBucket(); err == nil {