Merge pull request #3735 from luthermonson/oidc-config

normalize oidc configs to string values for comparison

Author: k8s-ci-robot

pkg/cloud/converters/eks.go

@@ -148,16 +148,19 @@ func TaintEffectFromSDK(effect string) (expinfrav1.TaintEffect, error) {
 
 func ConvertSDKToIdentityProvider(in *ekscontrolplanev1.OIDCIdentityProviderConfig) *identityprovider.OidcIdentityProviderConfig {
 	if in != nil {
+		if in.RequiredClaims == nil {
+			in.RequiredClaims = make(map[string]string)
+		}
 		return &identityprovider.OidcIdentityProviderConfig{
 			ClientID:                   in.ClientID,
-			GroupsClaim:                in.GroupsClaim,
-			GroupsPrefix:               in.GroupsPrefix,
+			GroupsClaim:                aws.StringValue(in.GroupsClaim),
+			GroupsPrefix:               aws.StringValue(in.GroupsPrefix),
 			IdentityProviderConfigName: in.IdentityProviderConfigName,
 			IssuerURL:                  in.IssuerURL,
-			RequiredClaims:             aws.StringMap(in.RequiredClaims),
+			RequiredClaims:             in.RequiredClaims,
 			Tags:                       in.Tags,
-			UsernameClaim:              in.UsernameClaim,
-			UsernamePrefix:             in.UsernamePrefix,
+			UsernameClaim:              aws.StringValue(in.UsernameClaim),
+			UsernamePrefix:             aws.StringValue(in.UsernamePrefix),
 		}
 	}
 

pkg/cloud/services/eks/identity_provider.go

@@ -49,14 +49,20 @@ func (s *Service) reconcileIdentityProvider(ctx context.Context) error {
 	}
 
 	s.scope.V(2).Info("creating oidc provider plan", "desired", desired, "current", current)
+	procedures, err := identityprovider.
+		NewPlan(clusterName, current, desired, s.EKSClient, s.scope.Logger).
+		Create(ctx)
 
-	identityProviderPlan := identityprovider.NewPlan(clusterName, current, desired, s.EKSClient, s.scope.Logger)
-
-	procedures, err := identityProviderPlan.Create(ctx)
 	if err != nil {
 		s.scope.Error(err, "failed creating eks identity provider plan")
 		return fmt.Errorf("creating eks identity provider plan: %w", err)
 	}
+
+	// nothing will be done, we can leave
+	if len(procedures) == 0 {
+		return nil
+	}
+
 	s.scope.V(2).Info("computed EKS identity provider plan", "numprocs", len(procedures))
 
 	// Perform required operations
@@ -73,16 +79,24 @@ func (s *Service) reconcileIdentityProvider(ctx context.Context) error {
 		return errors.Wrap(err, "getting associated identity provider")
 	}
 
-	if latest != nil {
-		s.scope.ControlPlane.Status.IdentityProviderStatus = ekscontrolplanev1.IdentityProviderStatus{
-			ARN:    aws.StringValue(latest.IdentityProviderConfigArn),
-			Status: aws.StringValue(latest.Status),
-		}
+	if latest == nil {
+		return nil
+	}
 
-		err := s.scope.PatchObject()
-		if err != nil {
-			return errors.Wrap(err, "updating identity provider status")
-		}
+	// don't patch if arn/status is the same
+	if latest.IdentityProviderConfigArn == s.scope.ControlPlane.Status.IdentityProviderStatus.ARN &&
+		latest.Status == s.scope.ControlPlane.Status.IdentityProviderStatus.Status {
+		return nil
+	}
+
+	// idp status has changed, patch the control plane
+	s.scope.ControlPlane.Status.IdentityProviderStatus = ekscontrolplanev1.IdentityProviderStatus{
+		ARN:    latest.IdentityProviderConfigArn,
+		Status: latest.Status,
+	}
+
+	if err := s.scope.PatchObject(); err != nil {
+		return errors.Wrap(err, "updating identity provider status")
 	}
 
 	return nil
@@ -114,16 +128,16 @@ func (s *Service) getAssociatedIdentityProvider(ctx context.Context, clusterName
 	config := providerconfig.IdentityProviderConfig.Oidc
 
 	return &identityprovider.OidcIdentityProviderConfig{
-		ClientID:                   *config.ClientId,
-		GroupsClaim:                config.GroupsClaim,
-		GroupsPrefix:               config.GroupsPrefix,
-		IdentityProviderConfigArn:  config.IdentityProviderConfigArn,
-		IdentityProviderConfigName: *config.IdentityProviderConfigName,
-		IssuerURL:                  *config.IssuerUrl,
-		RequiredClaims:             config.RequiredClaims,
-		Status:                     config.Status,
+		ClientID:                   aws.StringValue(config.ClientId),
+		GroupsClaim:                aws.StringValue(config.GroupsClaim),
+		GroupsPrefix:               aws.StringValue(config.GroupsPrefix),
+		IdentityProviderConfigArn:  aws.StringValue(config.IdentityProviderConfigArn),
+		IdentityProviderConfigName: aws.StringValue(config.IdentityProviderConfigName),
+		IssuerURL:                  aws.StringValue(config.IssuerUrl),
+		RequiredClaims:             aws.StringValueMap(config.RequiredClaims),
+		Status:                     aws.StringValue(config.Status),
 		Tags:                       converters.MapPtrToMap(config.Tags),
-		UsernameClaim:              config.UsernameClaim,
-		UsernamePrefix:             config.UsernamePrefix,
+		UsernameClaim:              aws.StringValue(config.UsernameClaim),
+		UsernamePrefix:             aws.StringValue(config.UsernamePrefix),
 	}, nil
 }

pkg/eks/identityprovider/plan.go

@@ -19,7 +19,6 @@ package identityprovider
 import (
 	"context"
 
-	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/eks"
 	"github.com/aws/aws-sdk-go/service/eks/eksiface"
 	"github.com/go-logr/logr"
@@ -58,7 +57,7 @@ func (p *plan) Create(ctx context.Context) ([]planner.Procedure, error) {
 	if p.desiredIdentityProvider == nil {
 		// disassociation will also also trigger deletion hence
 		// we do nothing in case of ConfigStatusDeleting as it will happen eventually
-		if aws.StringValue(p.currentIdentityProvider.Status) == eks.ConfigStatusActive {
+		if p.currentIdentityProvider.Status == eks.ConfigStatusActive {
 			procedures = append(procedures, &DisassociateIdentityProviderConfig{plan: p})
 		}
 
@@ -80,7 +79,7 @@ func (p *plan) Create(ctx context.Context) ([]planner.Procedure, error) {
 		if len(p.desiredIdentityProvider.Tags) == 0 && len(p.currentIdentityProvider.Tags) != 0 {
 			procedures = append(procedures, &RemoveIdentityProviderTagsProcedure{plan: p})
 		}
-		switch aws.StringValue(p.currentIdentityProvider.Status) {
+		switch p.currentIdentityProvider.Status {
 		case eks.ConfigStatusActive:
 			// config active no work to be done
 			return procedures, nil

pkg/eks/identityprovider/plan_test.go

@@ -227,8 +227,8 @@ func createDesiredIdentityProvider(name string, tags infrav1.Tags) *OidcIdentity
 
 func createCurrentIdentityProvider(name string, arn, status string, tags infrav1.Tags) *OidcIdentityProviderConfig {
 	config := createDesiredIdentityProvider(name, tags)
-	config.IdentityProviderConfigArn = aws.String(arn)
-	config.Status = aws.String(status)
+	config.IdentityProviderConfigArn = arn
+	config.Status = status
 
 	return config
 }
@@ -243,6 +243,11 @@ func createDesiredIdentityProviderRequest(name *string) *eks.OidcIdentityProvide
 		ClientId:                   aws.String("clientId"),
 		IdentityProviderConfigName: name,
 		IssuerUrl:                  aws.String("http://IssuerURL.com"),
+		RequiredClaims:             make(map[string]*string),
+		GroupsClaim:                aws.String(""),
+		GroupsPrefix:               aws.String(""),
+		UsernameClaim:              aws.String(""),
+		UsernamePrefix:             aws.String(""),
 	}
 }
 

pkg/eks/identityprovider/procedures.go

@@ -106,13 +106,13 @@ func (a *AssociateIdentityProviderProcedure) Do(ctx context.Context) error {
 		ClusterName: aws.String(a.plan.clusterName),
 		Oidc: &eks.OidcIdentityProviderConfigRequest{
 			ClientId:                   aws.String(oidc.ClientID),
-			GroupsClaim:                oidc.GroupsClaim,
-			GroupsPrefix:               oidc.GroupsPrefix,
+			GroupsClaim:                aws.String(oidc.GroupsClaim),
+			GroupsPrefix:               aws.String(oidc.GroupsPrefix),
 			IdentityProviderConfigName: aws.String(oidc.IdentityProviderConfigName),
 			IssuerUrl:                  aws.String(oidc.IssuerURL),
-			RequiredClaims:             oidc.RequiredClaims,
-			UsernameClaim:              oidc.UsernameClaim,
-			UsernamePrefix:             oidc.UsernamePrefix,
+			RequiredClaims:             aws.StringMap(oidc.RequiredClaims),
+			UsernameClaim:              aws.String(oidc.UsernameClaim),
+			UsernamePrefix:             aws.String(oidc.UsernamePrefix),
 		},
 	}
 
@@ -139,7 +139,7 @@ func (u *UpdatedIdentityProviderTagsProcedure) Name() string {
 func (u *UpdatedIdentityProviderTagsProcedure) Do(ctx context.Context) error {
 	arn := u.plan.currentIdentityProvider.IdentityProviderConfigArn
 	_, err := u.plan.eksClient.TagResource(&eks.TagResourceInput{
-		ResourceArn: arn,
+		ResourceArn: &arn,
 		Tags:        aws.StringMap(u.plan.desiredIdentityProvider.Tags),
 	})
 
@@ -164,8 +164,10 @@ func (r *RemoveIdentityProviderTagsProcedure) Do(ctx context.Context) error {
 	for key := range r.plan.currentIdentityProvider.Tags {
 		keys = append(keys, aws.String(key))
 	}
+
+	arn := r.plan.currentIdentityProvider.IdentityProviderConfigArn
 	_, err := r.plan.eksClient.UntagResource(&eks.UntagResourceInput{
-		ResourceArn: r.plan.currentIdentityProvider.IdentityProviderConfigArn,
+		ResourceArn: &arn,
 		TagKeys:     keys,
 	})
 

pkg/eks/identityprovider/types.go

@@ -17,61 +17,62 @@ limitations under the License.
 package identityprovider
 
 import (
-	"github.com/google/go-cmp/cmp"
+	"reflect"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta2"
 )
 
-// OidcIdentityProviderConfig represents the configuration for an OpenID Connect (OIDC)
-// identity provider.
+// OidcIdentityProviderConfig represents a normalized version of the configuration for an OpenID Connect (OIDC)
+// identity provider configuration. To reconcile the config we are going to get the version from EKS and
+// AWSManagedControlPlane and will need to have one consistent version of string values from each API.
 type OidcIdentityProviderConfig struct {
 	ClientID                   string
-	GroupsClaim                *string
-	GroupsPrefix               *string
-	IdentityProviderConfigArn  *string
+	GroupsClaim                string
+	GroupsPrefix               string
+	IdentityProviderConfigArn  string
 	IdentityProviderConfigName string
 	IssuerURL                  string
-	RequiredClaims             map[string]*string
-	Status                     *string
+	RequiredClaims             map[string]string
+	Status                     string
 	Tags                       infrav1.Tags
-	UsernameClaim              *string
-	UsernamePrefix             *string
+	UsernameClaim              string
+	UsernamePrefix             string
 }
 
 func (o *OidcIdentityProviderConfig) IsEqual(other *OidcIdentityProviderConfig) bool {
 	if o == other {
 		return true
 	}
 
-	if !cmp.Equal(o.ClientID, other.ClientID) {
+	if o.ClientID != other.ClientID {
 		return false
 	}
 
-	if !cmp.Equal(o.GroupsClaim, other.GroupsClaim) {
+	if o.GroupsClaim != other.GroupsClaim {
 		return false
 	}
 
-	if !cmp.Equal(o.GroupsPrefix, other.GroupsPrefix) {
+	if o.GroupsPrefix != other.GroupsPrefix {
 		return false
 	}
 
-	if !cmp.Equal(o.IdentityProviderConfigName, other.IdentityProviderConfigName) {
+	if o.IdentityProviderConfigName != other.IdentityProviderConfigName {
 		return false
 	}
 
-	if !cmp.Equal(o.IssuerURL, other.IssuerURL) {
+	if o.IssuerURL != other.IssuerURL {
 		return false
 	}
 
-	if !cmp.Equal(o.RequiredClaims, other.RequiredClaims) {
+	if !reflect.DeepEqual(o.RequiredClaims, other.RequiredClaims) {
 		return false
 	}
 
-	if !cmp.Equal(o.UsernameClaim, other.UsernameClaim) {
+	if o.UsernameClaim != other.UsernameClaim {
 		return false
 	}
 
-	if !cmp.Equal(o.UsernamePrefix, other.UsernamePrefix) {
+	if o.UsernamePrefix != other.UsernamePrefix {
 		return false
 	}
 

pkg/eks/identityprovider/types_test.go

@@ -19,7 +19,6 @@ package identityprovider
 import (
 	"testing"
 
-	"github.com/aws/aws-sdk-go/aws"
 	"github.com/onsi/gomega"
 )
 
@@ -39,7 +38,7 @@ func TestIdentityProviderEqual(t *testing.T) {
 				ClientID:                   "a",
 				IdentityProviderConfigName: "b",
 				IssuerURL:                  "c",
-				Status:                     aws.String("e"),
+				Status:                     "e",
 			},
 		},
 	}