fix direct controller set labels

gemmahou · Sep 18, 2024 · 6afeab4 · 6afeab4
1 parent 6f0b538
commit 6afeab4
Show file tree

Hide file tree

Showing 9 changed files with 337 additions and 75 deletions.
diff --git a/mockgcp/mockcompute/globalforwardingrulesv1.go b/mockgcp/mockcompute/globalforwardingrulesv1.go
@@ -67,6 +67,8 @@ func (s *GlobalForwardingRulesV1) Insert(ctx context.Context, req *pb.InsertGlob
 	obj.CreationTimestamp = PtrTo(s.nowString())
 	obj.Id = &id
 	obj.Kind = PtrTo("compute#forwardingRule")
+	// labels will be added separately with setLabels
+	obj.Labels = nil
 	// If below values are not provided by user, it appears to default by GCP
 	if obj.LabelFingerprint == nil {
 		obj.LabelFingerprint = PtrTo(computeFingerprint(obj))

diff --git a/mockgcp/mockcompute/regionalforwardingrulev1.go b/mockgcp/mockcompute/regionalforwardingrulev1.go
@@ -68,6 +68,8 @@ func (s *RegionalForwardingRulesV1) Insert(ctx context.Context, req *pb.InsertFo
 	obj.Id = &id
 	obj.Kind = PtrTo("compute#forwardingRule")
 	obj.Region = PtrTo(fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/%s/regions/%s", name.Project.ID, name.Region))
+	// labels will be added separately with setLabels
+	obj.Labels = nil
 	// If below values are not provided by user, it appears to default by GCP
 	if obj.LabelFingerprint == nil {
 		obj.LabelFingerprint = PtrTo(computeFingerprint(obj))

diff --git a/pkg/controller/direct/compute/forwardingrule_controller.go b/pkg/controller/direct/compute/forwardingrule_controller.go
@@ -314,6 +314,7 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, createOp *directbase
 	forwardingRule.Name = direct.LazyPtr(a.id.forwardingRule)
 	forwardingRule.Labels = desired.Labels
 
+	// Create forwarding rule(labels are not set during Insert)
 	var err error
 	op := &gcp.Operation{}
 	if a.id.location == "global" {
@@ -340,6 +341,7 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, createOp *directbase
 		}
 	}
 	log.V(2).Info("successfully created ComputeForwardingRule", "name", a.fullyQualifiedName())
+
 	// Get the created resource
 	created := &computepb.ForwardingRule{}
 	if a.id.location == "global" {
@@ -360,6 +362,55 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, createOp *directbase
 		return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.fullyQualifiedName(), err)
 	}
 
+	// Set labels for the created forwarding rule
+	if forwardingRule.Labels != nil {
+		if a.id.location == "global" {
+			setLabelsReq := &computepb.SetLabelsGlobalForwardingRuleRequest{
+				Resource:                       a.id.forwardingRule,
+				GlobalSetLabelsRequestResource: &computepb.GlobalSetLabelsRequest{LabelFingerprint: created.LabelFingerprint, Labels: forwardingRule.Labels},
+				Project:                        a.id.project,
+			}
+			op, err = a.globalForwardingRulesClient.SetLabels(ctx, setLabelsReq)
+		} else {
+			setLabelsReq := &computepb.SetLabelsForwardingRuleRequest{
+				Resource:                       a.id.forwardingRule,
+				RegionSetLabelsRequestResource: &computepb.RegionSetLabelsRequest{LabelFingerprint: created.LabelFingerprint, Labels: forwardingRule.Labels},
+				Project:                        a.id.project,
+				Region:                         a.id.location,
+			}
+			op, err = a.forwardingRulesClient.SetLabels(ctx, setLabelsReq)
+		}
+		if err != nil {
+			return fmt.Errorf("adding ComputeForwardingRule labels %s: %w", a.fullyQualifiedName(), err)
+		}
+		if !op.Done() {
+			err = op.Wait(ctx)
+			if err != nil {
+				return fmt.Errorf("waiting ComputeForwardingRule %s add labels failed: %w", a.fullyQualifiedName(), err)
+			}
+		}
+		log.V(2).Info("successfully added ComputeForwardingRule labels", "name", a.fullyQualifiedName())
+
+		// Get the created resource with label added
+		if a.id.location == "global" {
+			getReq := &computepb.GetGlobalForwardingRuleRequest{
+				ForwardingRule: a.id.forwardingRule,
+				Project:        a.id.project,
+			}
+			created, err = a.globalForwardingRulesClient.Get(ctx, getReq)
+		} else {
+			getReq := &computepb.GetForwardingRuleRequest{
+				ForwardingRule: a.id.forwardingRule,
+				Region:         a.id.location,
+				Project:        a.id.project,
+			}
+			created, err = a.forwardingRulesClient.Get(ctx, getReq)
+		}
+		if err != nil {
+			return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.fullyQualifiedName(), err)
+		}
+	}
+
 	status := &krm.ComputeForwardingRuleStatus{
 		LabelFingerprint:  created.LabelFingerprint,
 		CreationTimestamp: created.CreationTimestamp,
@@ -393,52 +444,33 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, updateOp *directbase
 	var err error
 	op := &gcp.Operation{}
 	updated := &computepb.ForwardingRule{}
-	// TODO(yuhou): Checked the realGCP logs, setLabels request is being sent even when there are no updates to labels.
-	// That might because of the generated labelsFingerPrint?
-	if a.id.location == "global" {
-		setLabelsReq := &computepb.SetLabelsGlobalForwardingRuleRequest{
-			Resource:                       a.id.forwardingRule,
-			GlobalSetLabelsRequestResource: &computepb.GlobalSetLabelsRequest{LabelFingerprint: a.actual.LabelFingerprint, Labels: forwardingRule.Labels},
-			Project:                        a.id.project,
-		}
-		op, err = a.globalForwardingRulesClient.SetLabels(ctx, setLabelsReq)
-	} else {
-		setLabelsReq := &computepb.SetLabelsForwardingRuleRequest{
-			Resource:                       a.id.forwardingRule,
-			RegionSetLabelsRequestResource: &computepb.RegionSetLabelsRequest{LabelFingerprint: a.actual.LabelFingerprint, Labels: forwardingRule.Labels},
-			Project:                        a.id.project,
-			Region:                         a.id.location,
+	if !reflect.DeepEqual(forwardingRule.Labels, a.actual.Labels) {
+		if a.id.location == "global" {
+			setLabelsReq := &computepb.SetLabelsGlobalForwardingRuleRequest{
+				Resource:                       a.id.forwardingRule,
+				GlobalSetLabelsRequestResource: &computepb.GlobalSetLabelsRequest{LabelFingerprint: a.actual.LabelFingerprint, Labels: forwardingRule.Labels},
+				Project:                        a.id.project,
+			}
+			op, err = a.globalForwardingRulesClient.SetLabels(ctx, setLabelsReq)
+		} else {
+			setLabelsReq := &computepb.SetLabelsForwardingRuleRequest{
+				Resource:                       a.id.forwardingRule,
+				RegionSetLabelsRequestResource: &computepb.RegionSetLabelsRequest{LabelFingerprint: a.actual.LabelFingerprint, Labels: forwardingRule.Labels},
+				Project:                        a.id.project,
+				Region:                         a.id.location,
+			}
+			op, err = a.forwardingRulesClient.SetLabels(ctx, setLabelsReq)
 		}
-		op, err = a.forwardingRulesClient.SetLabels(ctx, setLabelsReq)
-	}
-	if err != nil {
-		return fmt.Errorf("updating ComputeForwardingRule labels %s: %w", a.fullyQualifiedName(), err)
-	}
-	if !op.Done() {
-		err = op.Wait(ctx)
 		if err != nil {
-			return fmt.Errorf("waiting ComputeForwardingRule %s update labels failed: %w", a.fullyQualifiedName(), err)
-		}
-	}
-	log.V(2).Info("successfully updated ComputeForwardingRule labels", "name", a.fullyQualifiedName())
-
-	// Get the updated resource
-	if a.id.location == "global" {
-		getReq := &computepb.GetGlobalForwardingRuleRequest{
-			ForwardingRule: a.id.forwardingRule,
-			Project:        a.id.project,
+			return fmt.Errorf("updating ComputeForwardingRule labels %s: %w", a.fullyQualifiedName(), err)
 		}
-		updated, err = a.globalForwardingRulesClient.Get(ctx, getReq)
-	} else {
-		getReq := &computepb.GetForwardingRuleRequest{
-			ForwardingRule: a.id.forwardingRule,
-			Region:         a.id.location,
-			Project:        a.id.project,
+		if !op.Done() {
+			err = op.Wait(ctx)
+			if err != nil {
+				return fmt.Errorf("waiting ComputeForwardingRule %s update labels failed: %w", a.fullyQualifiedName(), err)
+			}
 		}
-		updated, err = a.forwardingRulesClient.Get(ctx, getReq)
-	}
-	if err != nil {
-		return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.id.forwardingRule, err)
+		log.V(2).Info("successfully updated ComputeForwardingRule labels", "name", a.fullyQualifiedName())
 	}
 
 	// setTarget request is sent when there are updates to target.

diff --git a/...estdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log b/...estdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log
@@ -848,11 +848,6 @@ X-Xss-Protection: 0
   "id": "000000000000000000000",
   "kind": "compute#forwardingRule",
   "labelFingerprint": "abcdef0123A=",
-  "labels": {
-    "cnrm-test": "true",
-    "label-one": "value-one",
-    "managed-by-cnrm": "true"
-  },
   "loadBalancingScheme": "INTERNAL_SELF_MANAGED",
   "name": "computeglobalforwardingrule-${uniqueId}",
   "network": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/networks/${networkID}",

diff --git a/...ata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrulefull/_http.log b/...ata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrulefull/_http.log
@@ -863,11 +863,6 @@ X-Xss-Protection: 0
   "ipVersion": "IPV4",
   "kind": "compute#forwardingRule",
   "labelFingerprint": "abcdef0123A=",
-  "labels": {
-    "cnrm-test": "true",
-    "label-one": "value-one",
-    "managed-by-cnrm": "true"
-  },
   "loadBalancingScheme": "INTERNAL_SELF_MANAGED",
   "metadataFilters": [
     {
@@ -899,7 +894,7 @@ x-goog-request-params: project=${projectId}&resource=computeglobalforwardingrule
   "labelFingerprint": "abcdef0123A=",
   "labels": {
     "cnrm-test": "true",
-    "label-one": "value-two",
+    "label-one": "value-one",
     "managed-by-cnrm": "true"
   }
 }
@@ -961,7 +956,7 @@ X-Xss-Protection: 0
   "labelFingerprint": "abcdef0123A=",
   "labels": {
     "cnrm-test": "true",
-    "label-one": "value-two",
+    "label-one": "value-one",
     "managed-by-cnrm": "true"
   },
   "loadBalancingScheme": "INTERNAL_SELF_MANAGED",
@@ -986,6 +981,49 @@ X-Xss-Protection: 0
 
 ---
 
+POST https://compute.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/${forwardingRuleID}/setLabels
+Content-Type: application/json
+User-Agent: kcc/controller-manager
+x-goog-request-params: project=${projectId}&resource=computeglobalforwardingrule-${uniqueId}
+
+{
+  "labelFingerprint": "abcdef0123A=",
+  "labels": {
+    "cnrm-test": "true",
+    "label-one": "value-two",
+    "managed-by-cnrm": "true"
+  }
+}
+
+200 OK
+Cache-Control: private
+Content-Type: application/json; charset=UTF-8
+Server: ESF
+Vary: Origin
+Vary: X-Origin
+Vary: Referer
+X-Content-Type-Options: nosniff
+X-Frame-Options: SAMEORIGIN
+X-Xss-Protection: 0
+
+{
+  "endTime": "2024-04-01T12:34:56.123456Z",
+  "id": "000000000000000000000",
+  "insertTime": "2024-04-01T12:34:56.123456Z",
+  "kind": "compute#operation",
+  "name": "${operationID}",
+  "operationType": "setLabels",
+  "progress": 100,
+  "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}",
+  "startTime": "2024-04-01T12:34:56.123456Z",
+  "status": "DONE",
+  "targetId": "${forwardingRulesId}",
+  "targetLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/computeglobalforwardingrule-${uniqueId}",
+  "user": "[email protected]"
+}
+
+---
+
 POST https://compute.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/${forwardingRuleID}/setTarget
 Content-Type: application/json
 User-Agent: kcc/controller-manager

diff --git a/...ta/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrulehttps/_http.log b/...ta/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrulehttps/_http.log
@@ -1142,11 +1142,6 @@ X-Xss-Protection: 0
   "id": "000000000000000000000",
   "kind": "compute#forwardingRule",
   "labelFingerprint": "abcdef0123A=",
-  "labels": {
-    "cnrm-test": "true",
-    "label-one": "value-one",
-    "managed-by-cnrm": "true"
-  },
   "loadBalancingScheme": "EXTERNAL",
   "name": "computeglobalforwardingrule-${uniqueId}",
   "networkTier": "PREMIUM",
@@ -1166,7 +1161,7 @@ x-goog-request-params: project=${projectId}&resource=computeglobalforwardingrule
   "labelFingerprint": "abcdef0123A=",
   "labels": {
     "cnrm-test": "true",
-    "label-one": "value-two",
+    "label-one": "value-one",
     "managed-by-cnrm": "true"
   }
 }
@@ -1227,7 +1222,7 @@ X-Xss-Protection: 0
   "labelFingerprint": "abcdef0123A=",
   "labels": {
     "cnrm-test": "true",
-    "label-one": "value-two",
+    "label-one": "value-one",
     "managed-by-cnrm": "true"
   },
   "loadBalancingScheme": "EXTERNAL",
@@ -1240,6 +1235,49 @@ X-Xss-Protection: 0
 
 ---
 
+POST https://compute.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/${forwardingRuleID}/setLabels
+Content-Type: application/json
+User-Agent: kcc/controller-manager
+x-goog-request-params: project=${projectId}&resource=computeglobalforwardingrule-${uniqueId}
+
+{
+  "labelFingerprint": "abcdef0123A=",
+  "labels": {
+    "cnrm-test": "true",
+    "label-one": "value-two",
+    "managed-by-cnrm": "true"
+  }
+}
+
+200 OK
+Cache-Control: private
+Content-Type: application/json; charset=UTF-8
+Server: ESF
+Vary: Origin
+Vary: X-Origin
+Vary: Referer
+X-Content-Type-Options: nosniff
+X-Frame-Options: SAMEORIGIN
+X-Xss-Protection: 0
+
+{
+  "endTime": "2024-04-01T12:34:56.123456Z",
+  "id": "000000000000000000000",
+  "insertTime": "2024-04-01T12:34:56.123456Z",
+  "kind": "compute#operation",
+  "name": "${operationID}",
+  "operationType": "setLabels",
+  "progress": 100,
+  "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}",
+  "startTime": "2024-04-01T12:34:56.123456Z",
+  "status": "DONE",
+  "targetId": "${forwardingRulesId}",
+  "targetLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/computeglobalforwardingrule-${uniqueId}",
+  "user": "[email protected]"
+}
+
+---
+
 POST https://compute.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/${forwardingRuleID}/setTarget
 Content-Type: application/json
 User-Agent: kcc/controller-manager