Ignore default tags during reconciliation (#131)

Author: shyamradhakrishnan

cloud/scope/cluster.go

@@ -19,22 +19,20 @@ package scope
 import (
 	"context"
 	"fmt"
-	"reflect"
-	"sigs.k8s.io/cluster-api/util/conditions"
 	"strconv"
 
-	"github.com/oracle/cluster-api-provider-oci/cloud/services/vcn"
-
 	"github.com/go-logr/logr"
 	infrastructurev1beta1 "github.com/oracle/cluster-api-provider-oci/api/v1beta1"
 	"github.com/oracle/cluster-api-provider-oci/cloud/ociutil"
 	identityClent "github.com/oracle/cluster-api-provider-oci/cloud/services/identity"
 	nlb "github.com/oracle/cluster-api-provider-oci/cloud/services/networkloadbalancer"
+	"github.com/oracle/cluster-api-provider-oci/cloud/services/vcn"
 	"github.com/oracle/oci-go-sdk/v63/common"
 	"github.com/oracle/oci-go-sdk/v63/identity"
 	"github.com/pkg/errors"
 	"k8s.io/klog/v2/klogr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/cluster-api/util/conditions"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -223,13 +221,6 @@ func (s *ClusterScope) setAvailabiltyDomainStatus(ctx context.Context, ads []ide
 	return nil
 }
 
-func (s *ClusterScope) IsTagsEqual(freeFromTags map[string]string, definedTags map[string]map[string]interface{}) bool {
-	if reflect.DeepEqual(freeFromTags, s.GetFreeFormTags()) && reflect.DeepEqual(definedTags, s.GetDefinedTags()) {
-		return true
-	}
-	return false
-}
-
 // GetRegionCodeFromRegion pulls all OCI regions available and returns the passed in region's code if contained in
 // the list.
 //
@@ -280,14 +271,15 @@ func (s *ClusterScope) APIServerPort() int32 {
 // GetFreeFormTags returns a map of FreeformTags defined in the OCICluster's spec
 func (s *ClusterScope) GetFreeFormTags() map[string]string {
 	tags := s.OCICluster.Spec.FreeformTags
-	if tags == nil {
-		tags = make(map[string]string)
+	completeTags := make(map[string]string)
+	for k, v := range tags {
+		completeTags[k] = v
 	}
-	tagsAddedByClusterAPI := ociutil.BuildClusterTags(string(s.OCICluster.GetOCIResourceIdentifier()))
+	tagsAddedByClusterAPI := ociutil.BuildClusterTags(s.OCICluster.GetOCIResourceIdentifier())
 	for k, v := range tagsAddedByClusterAPI {
-		tags[k] = v
+		completeTags[k] = v
 	}
-	return tags
+	return completeTags
 }
 
 func (s *ClusterScope) GetOCICluster() *infrastructurev1beta1.OCICluster {

cloud/scope/drg_reconciler.go

@@ -46,12 +46,6 @@ func (s *ClusterScope) ReconcileDRG(ctx context.Context) error {
 	}
 	if drg != nil {
 		s.getDRG().ID = drg.Id
-		if !s.IsTagsEqual(drg.FreeformTags, drg.DefinedTags) {
-			_, err := s.updateDRG(ctx)
-			if err != nil {
-				return err
-			}
-		}
 		s.Logger.Info("No Reconciliation Required for DRG", "drg", drg.Id)
 		return nil
 	}
@@ -131,20 +125,6 @@ func (s *ClusterScope) createDRG(ctx context.Context) (*core.Drg, error) {
 	return &response.Drg, nil
 }
 
-func (s *ClusterScope) updateDRG(ctx context.Context) (*core.Drg, error) {
-	response, err := s.VCNClient.UpdateDrg(ctx, core.UpdateDrgRequest{
-		DrgId: s.getDRG().ID,
-		UpdateDrgDetails: core.UpdateDrgDetails{
-			FreeformTags: s.GetFreeFormTags(),
-			DefinedTags:  s.GetDefinedTags(),
-		},
-	})
-	if err != nil {
-		return nil, err
-	}
-	return &response.Drg, nil
-}
-
 func (s *ClusterScope) GetDRGName() string {
 	if s.getDRG().Name != "" {
 		return s.getDRG().Name

cloud/scope/drg_reconciler_test.go

@@ -160,38 +160,6 @@ func TestDRGReconciliation(t *testing.T) {
 					}, nil)
 			},
 		},
-		{
-			name:          "drg update",
-			errorExpected: false,
-			testSpecificSetup: func(clusterScope *ClusterScope, vcnClient *mock_vcn.MockClient) {
-				vcnPeering.DRG = &infrastructurev1beta1.DRG{}
-				vcnPeering.DRG.Manage = true
-				vcnPeering.DRG.ID = common.String("drg-id")
-				existingTags := make(map[string]string)
-				existingTags[ociutil.CreatedBy] = ociutil.OCIClusterAPIProvider
-				existingTags[ociutil.ClusterResourceIdentifier] = "resource_uid"
-				existingTags["test"] = "test"
-				clusterScope.OCICluster.Spec.NetworkSpec.VCNPeering = &vcnPeering
-				vcnClient.EXPECT().GetDrg(gomock.Any(), gomock.Eq(core.GetDrgRequest{
-					DrgId: common.String("drg-id"),
-				})).
-					Return(core.GetDrgResponse{
-						Drg: core.Drg{
-							Id:           common.String("drg-id"),
-							FreeformTags: existingTags,
-							DefinedTags:  make(map[string]map[string]interface{}),
-						},
-					}, nil)
-				vcnClient.EXPECT().UpdateDrg(gomock.Any(), gomock.Eq(core.UpdateDrgRequest{
-					DrgId: common.String("drg-id"),
-					UpdateDrgDetails: core.UpdateDrgDetails{
-						FreeformTags: tags,
-						DefinedTags:  make(map[string]map[string]interface{}),
-					},
-				})).
-					Return(core.UpdateDrgResponse{}, nil)
-			},
-		},
 		{
 			name:          "drg create",
 			errorExpected: false,

cloud/scope/drg_rpc_attachment_reconciler.go

@@ -55,13 +55,6 @@ func (s *ClusterScope) ReconcileDRGRPCAttachment(ctx context.Context) error {
 		if localRpc != nil {
 			rpcSpec.RPCConnectionId = localRpc.Id
 			s.Logger.Info("Local RPC exists", "rpcId", localRpc.Id)
-			if !s.IsTagsEqual(localRpc.FreeformTags, localRpc.DefinedTags) {
-				_, err := s.updateDRGRpc(ctx, localRpc.Id, s.VCNClient)
-				if err != nil {
-					return err
-				}
-				s.Logger.Info("Local RPC has been updated", "rpcId", localRpc.Id)
-			}
 		} else {
 			localRpc, err = s.createRPC(ctx, s.getDrgID(), s.OCICluster.Name, s.VCNClient)
 			if err != nil {
@@ -91,13 +84,6 @@ func (s *ClusterScope) ReconcileDRGRPCAttachment(ctx context.Context) error {
 				s.Logger.Info("Remote RPC exists", "rpcId", localRpc.Id)
 				s.Logger.Info("Connection status of 2 peered RPCs", "status", localRpc.PeeringStatus)
 				rpcSpec.PeerRPCConnectionId = remoteRpc.Id
-				if !s.IsTagsEqual(remoteRpc.FreeformTags, remoteRpc.DefinedTags) {
-					_, err := s.updateDRGRpc(ctx, remoteRpc.Id, clientProvider.VCNClient)
-					if err != nil {
-						return err
-					}
-					s.Logger.Info("Remote RPC has been updated", "rpcId", remoteRpc.Id)
-				}
 			} else {
 				remoteRpc, err = s.createRPC(ctx, rpcSpec.PeerDRGId, s.OCICluster.Name, clientProvider.VCNClient)
 				if err != nil {

cloud/scope/drg_vcn_attachment_reconciler.go

@@ -43,12 +43,6 @@ func (s *ClusterScope) ReconcileDRGVCNAttachment(ctx context.Context) error {
 		if err != nil {
 			return err
 		}
-		if !s.IsTagsEqual(attachment.FreeformTags, attachment.DefinedTags) {
-			_, err := s.UpdateDRGAttachment(ctx)
-			if err != nil {
-				return err
-			}
-		}
 		return nil
 	}
 

cloud/scope/drg_vcn_attachment_reconciler_test.go

@@ -130,39 +130,6 @@ func TestDRGVCNAttachmentReconciliation(t *testing.T) {
 					}, nil)
 			},
 		},
-		{
-			name:          "drg attachment update",
-			errorExpected: false,
-			testSpecificSetup: func(clusterScope *ClusterScope, vcnClient *mock_vcn.MockClient) {
-				vcnPeering.DRG = &infrastructurev1beta1.DRG{}
-				vcnPeering.DRG.Manage = true
-				vcnPeering.DRG.ID = common.String("drg-id")
-				vcnPeering.DRG.VcnAttachmentId = common.String("attachment-id")
-				clusterScope.OCICluster.Spec.NetworkSpec.VCNPeering = &vcnPeering
-				existingTags := make(map[string]string)
-				existingTags[ociutil.CreatedBy] = ociutil.OCIClusterAPIProvider
-				existingTags[ociutil.ClusterResourceIdentifier] = "resource_uid"
-				existingTags["test"] = "test"
-				vcnClient.EXPECT().GetDrgAttachment(gomock.Any(), gomock.Eq(core.GetDrgAttachmentRequest{
-					DrgAttachmentId: common.String("attachment-id"),
-				})).
-					Return(core.GetDrgAttachmentResponse{
-						DrgAttachment: core.DrgAttachment{
-							Id:           common.String("attachment-id"),
-							FreeformTags: existingTags,
-							DefinedTags:  make(map[string]map[string]interface{}),
-						},
-					}, nil)
-				vcnClient.EXPECT().UpdateDrgAttachment(gomock.Any(), gomock.Eq(core.UpdateDrgAttachmentRequest{
-					DrgAttachmentId: common.String("attachment-id"),
-					UpdateDrgAttachmentDetails: core.UpdateDrgAttachmentDetails{
-						FreeformTags: tags,
-						DefinedTags:  make(map[string]map[string]interface{}),
-					},
-				})).
-					Return(core.UpdateDrgAttachmentResponse{}, nil)
-			},
-		},
 		{
 			name:          "drg attachment create",
 			errorExpected: false,

cloud/scope/internet_gateway_reconciler.go

@@ -38,9 +38,6 @@ func (s *ClusterScope) ReconcileInternetGateway(ctx context.Context) error {
 	}
 	if igw != nil {
 		s.OCICluster.Spec.NetworkSpec.Vcn.InternetGatewayId = igw.Id
-		if !s.IsTagsEqual(igw.FreeformTags, igw.DefinedTags) {
-			return s.UpdateInternetGateway(ctx)
-		}
 		s.Logger.Info("No Reconciliation Required for Internet Gateway", "internet_gateway", igw.Id)
 		return nil
 	}
@@ -90,24 +87,6 @@ func (s *ClusterScope) GetInternetGateway(ctx context.Context) (*core.InternetGa
 	return nil, nil
 }
 
-// UpdateInternetGateway updates the FreeFormTags and DefinedTags
-func (s *ClusterScope) UpdateInternetGateway(ctx context.Context) error {
-	updateIGWDetails := core.UpdateInternetGatewayDetails{
-		FreeformTags: s.GetFreeFormTags(),
-		DefinedTags:  s.GetDefinedTags(),
-	}
-	igwResponse, err := s.VCNClient.UpdateInternetGateway(ctx, core.UpdateInternetGatewayRequest{
-		IgId:                         s.OCICluster.Spec.NetworkSpec.Vcn.InternetGatewayId,
-		UpdateInternetGatewayDetails: updateIGWDetails,
-	})
-	if err != nil {
-		s.Logger.Error(err, "failed to reconcile the internet gateway, failed to update")
-		return errors.Wrap(err, "failed to reconcile the internet gateway, failed to update")
-	}
-	s.Logger.Info("successfully updated the internet gateway", "internet_gateway", *igwResponse.Id)
-	return nil
-}
-
 // CreateInternetGateway creates the Internet Gateway for the cluster based on the ClusterScope
 func (s *ClusterScope) CreateInternetGateway(ctx context.Context) (*string, error) {
 	igwDetails := core.CreateInternetGatewayDetails{

cloud/scope/internet_gateway_reconciler_test.go

@@ -70,34 +70,13 @@ func TestClusterScope_ReconcileInternetGateway(t *testing.T) {
 				FreeformTags: tags,
 				DefinedTags:  definedTagsInterface,
 			},
-		}, nil).Times(2)
+		}, nil)
 
 	updatedTags := make(map[string]string)
 	for k, v := range tags {
 		updatedTags[k] = v
 	}
 	updatedTags["foo"] = "bar"
-	vcnClient.EXPECT().UpdateInternetGateway(gomock.Any(), gomock.Eq(core.UpdateInternetGatewayRequest{
-		IgId: common.String("foo"),
-		UpdateInternetGatewayDetails: core.UpdateInternetGatewayDetails{
-			FreeformTags: updatedTags,
-			DefinedTags:  definedTagsInterface,
-		},
-	})).
-		Return(core.UpdateInternetGatewayResponse{
-			InternetGateway: core.InternetGateway{
-				Id:           common.String("foo"),
-				FreeformTags: tags,
-			},
-		}, nil)
-	vcnClient.EXPECT().UpdateInternetGateway(gomock.Any(), gomock.Eq(core.UpdateInternetGatewayRequest{
-		IgId: common.String("igw_id"),
-		UpdateInternetGatewayDetails: core.UpdateInternetGatewayDetails{
-			FreeformTags: updatedTags,
-			DefinedTags:  definedTagsInterface,
-		},
-	})).
-		Return(core.UpdateInternetGatewayResponse{}, errors.New("some error"))
 	vcnClient.EXPECT().ListInternetGateways(gomock.Any(), gomock.Eq(core.ListInternetGatewaysRequest{
 		CompartmentId: common.String("foo"),
 		DisplayName:   common.String("internet-gateway"),
@@ -190,22 +169,7 @@ func TestClusterScope_ReconcileInternetGateway(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "update needed",
-			spec: infrastructurev1beta1.OCIClusterSpec{
-				FreeformTags: map[string]string{
-					"foo": "bar",
-				},
-				DefinedTags: definedTags,
-				NetworkSpec: infrastructurev1beta1.NetworkSpec{
-					Vcn: infrastructurev1beta1.VCN{
-						InternetGatewayId: common.String("foo"),
-					},
-				},
-			},
-			wantErr: false,
-		},
-		{
-			name: "id not present in spec but found by name and update needed but error out",
+			name: "id not present in spec but found by name and no update needed",
 			spec: infrastructurev1beta1.OCIClusterSpec{
 				CompartmentId: "foo",
 				FreeformTags: map[string]string{
@@ -218,8 +182,7 @@ func TestClusterScope_ReconcileInternetGateway(t *testing.T) {
 					},
 				},
 			},
-			wantErr:       true,
-			expectedError: "failed to reconcile the internet gateway, failed to update: some error",
+			wantErr: false,
 		},
 		{
 			name: "creation needed",

cloud/scope/load_balancer_reconciler.go

@@ -112,9 +112,7 @@ func (s *ClusterScope) GetControlPlaneLoadBalancerName() string {
 func (s *ClusterScope) UpdateLB(ctx context.Context, lb infrastructurev1beta1.LoadBalancer) error {
 	lbId := s.OCICluster.Spec.NetworkSpec.APIServerLB.LoadBalancerId
 	updateLBDetails := networkloadbalancer.UpdateNetworkLoadBalancerDetails{
-		DisplayName:  common.String(lb.Name),
-		FreeformTags: s.GetFreeFormTags(),
-		DefinedTags:  s.GetDefinedTags(),
+		DisplayName: common.String(lb.Name),
 	}
 	lbResponse, err := s.LoadBalancerClient.UpdateNetworkLoadBalancer(ctx, networkloadbalancer.UpdateNetworkLoadBalancerRequest{
 		UpdateNetworkLoadBalancerDetails: updateLBDetails,
@@ -244,12 +242,12 @@ func (s *ClusterScope) getLoadbalancerIp(nlb networkloadbalancer.NetworkLoadBala
 }
 
 // IsLBEqual determines if the actual networkloadbalancer.NetworkLoadBalancer is equal to the desired.
-// Equality is determined by DisplayName, FreeformTags and DefinedTags matching.
+// Equality is determined by DisplayName
 func (s *ClusterScope) IsLBEqual(actual *networkloadbalancer.NetworkLoadBalancer, desired infrastructurev1beta1.LoadBalancer) bool {
 	if desired.Name != *actual.DisplayName {
 		return false
 	}
-	return s.IsTagsEqual(actual.FreeformTags, actual.DefinedTags)
+	return true
 }
 
 // GetLoadBalancer retrieves the Cluster's networkloadbalancer.NetworkLoadBalancer using the one of the following methods