(Part 5 Do not merge)Fix diffs in compute forwarding rule: collect realGCP logs to verify

mockgcp/mockcompute/globalforwardingrulesv1.go

@@ -42,6 +42,9 @@ func (s *GlobalForwardingRulesV1) Get(ctx context.Context, req *pb.GetGlobalForw
 
 	obj := &pb.ForwardingRule{}
 	if err := s.storage.Get(ctx, fqn, obj); err != nil {
+		if status.Code(err) == codes.NotFound {
+			return nil, status.Errorf(codes.NotFound, "The resource '%s' was not found", fqn)
+		}
 		return nil, err
 	}
 
@@ -64,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))
@@ -160,10 +165,13 @@ func (s *GlobalForwardingRulesV1) SetLabels(ctx context.Context, req *pb.SetLabe
 	op := &pb.Operation{
 		TargetId:      obj.Id,
 		TargetLink:    obj.SelfLink,
-		OperationType: PtrTo("SetLabels"),
+		OperationType: PtrTo("setLabels"),
 		User:          PtrTo("[email protected]"),
 		// SetLabels operation has EndTime in response
 		EndTime: PtrTo("2024-04-01T12:34:56.123456Z"),
+		// SetLabels operation finished super fast
+		Progress: PtrTo(int32(100)),
+		Status:   PtrTo(pb.Operation_DONE),
 	}
 	return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) {
 		return obj, nil

mockgcp/mockcompute/operations.go

@@ -91,13 +91,17 @@ func (s *computeOperations) startLRO0(ctx context.Context, op *pb.Operation, fqn
 	op.InsertTime = PtrTo(formatTime(now))
 	op.Id = PtrTo(uint64(nanos))
 
-	op.Progress = PtrTo(int32(0))
+	if op.Progress == nil {
+		op.Progress = PtrTo(int32(0))
+	}
+
+	if op.Status == nil {
+		op.Status = PtrTo(pb.Operation_RUNNING)
+	}
 
 	op.Kind = PtrTo("compute#operation")
 	op.SelfLink = PtrTo("https://www.googleapis.com/compute/v1/" + fqn)
 
-	op.Status = PtrTo(pb.Operation_RUNNING)
-
 	log.Info("storing operation", "fqn", fqn)
 	if err := s.storage.Create(ctx, fqn, op); err != nil {
 		return nil, err

mockgcp/mockcompute/regionalforwardingrulev1.go

@@ -42,6 +42,9 @@ func (s *RegionalForwardingRulesV1) Get(ctx context.Context, req *pb.GetForwardi
 
 	obj := &pb.ForwardingRule{}
 	if err := s.storage.Get(ctx, fqn, obj); err != nil {
+		if status.Code(err) == codes.NotFound {
+			return nil, status.Errorf(codes.NotFound, "The resource '%s' was not found", fqn)
+		}
 		return nil, err
 	}
 
@@ -64,6 +67,9 @@ func (s *RegionalForwardingRulesV1) Insert(ctx context.Context, req *pb.InsertFo
 	obj.CreationTimestamp = PtrTo(s.nowString())
 	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))
@@ -161,10 +167,13 @@ func (s *RegionalForwardingRulesV1) SetLabels(ctx context.Context, req *pb.SetLa
 	op := &pb.Operation{
 		TargetId:      obj.Id,
 		TargetLink:    obj.SelfLink,
-		OperationType: PtrTo("SetLabels"),
+		OperationType: PtrTo("setLabels"),
 		User:          PtrTo("[email protected]"),
 		// SetLabels operation has EndTime in response
 		EndTime: PtrTo("2024-04-01T12:34:56.123456Z"),
+		// SetLabels operation finished super fast
+		Progress: PtrTo(int32(100)),
+		Status:   PtrTo(pb.Operation_DONE),
 	}
 	return s.startRegionalLRO(ctx, name.Project.ID, name.Region, op, func() (proto.Message, error) {
 		return obj, nil

pkg/controller/direct/compute/forwardingrule_controller.go

@@ -314,9 +314,15 @@ 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" {
+		// todo(yuhou): TF does not support networkTier field for global forwarding rule
+		// It will always use GCP's default value, which is "PREMIUM." Any value set by the user will be ignored and not sent to GCP.
+		// To align with the TF controller, I remove this field.
+		// Ideally, direct controller should support this field and validate that the value.
+		forwardingRule.NetworkTier = nil
 		req := &computepb.InsertGlobalForwardingRuleRequest{
 			ForwardingRuleResource: forwardingRule,
 			Project:                a.id.project,
@@ -333,11 +339,14 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, createOp *directbase
 	if err != nil {
 		return fmt.Errorf("creating ComputeForwardingRule %s: %w", a.fullyQualifiedName(), err)
 	}
-	err = op.Wait(ctx)
-	if err != nil {
-		return fmt.Errorf("waiting ComputeForwardingRule %s create failed: %w", a.fullyQualifiedName(), err)
+	if !op.Done() {
+		err = op.Wait(ctx)
+		if err != nil {
+			return fmt.Errorf("waiting ComputeForwardingRule %s create failed: %w", a.fullyQualifiedName(), err)
+		}
 	}
 	log.V(2).Info("successfully created ComputeForwardingRule", "name", a.fullyQualifiedName())
+
 	// Get the created resource
 	created := &computepb.ForwardingRule{}
 	if a.id.location == "global" {
@@ -358,6 +367,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,
@@ -391,49 +449,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)
-	}
-	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,
+		if err != nil {
+			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.
@@ -457,9 +499,11 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, updateOp *directbase
 		if err != nil {
 			return fmt.Errorf("updating ComputeForwardingRule target %s: %w", a.fullyQualifiedName(), err)
 		}
-		err = op.Wait(ctx)
-		if err != nil {
-			return fmt.Errorf("waiting ComputeForwardingRule %s update target failed: %w", a.fullyQualifiedName(), err)
+		if !op.Done() {
+			err = op.Wait(ctx)
+			if err != nil {
+				return fmt.Errorf("waiting ComputeForwardingRule %s update target failed: %w", a.fullyQualifiedName(), err)
+			}
 		}
 		log.V(2).Info("successfully updated ComputeForwardingRule target", "name", a.fullyQualifiedName())
 	}
@@ -544,9 +588,11 @@ func (a *forwardingRuleAdapter) Delete(ctx context.Context, deleteOp *directbase
 	if err != nil {
 		return false, fmt.Errorf("deleting ComputeForwardingRule %s: %w", a.id.forwardingRule, err)
 	}
-	err = op.Wait(ctx)
-	if err != nil {
-		return false, fmt.Errorf("waiting ComputeForwardingRule %s delete failed: %w", a.id.forwardingRule, err)
+	if !op.Done() {
+		err = op.Wait(ctx)
+		if err != nil {
+			return false, fmt.Errorf("waiting ComputeForwardingRule %s delete failed: %w", a.id.forwardingRule, err)
+		}
 	}
 	log.V(2).Info("successfully deleted ComputeForwardingRule", "name", a.id.forwardingRule)
 	return true, nil

pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_generated_object_globalcomputeforwardingrule.golden.yaml

@@ -4,10 +4,11 @@ metadata:
   annotations:
     cnrm.cloud.google.com/management-conflict-prevention-policy: none
     cnrm.cloud.google.com/project-id: ${projectId}
+    cnrm.cloud.google.com/state-into-spec: absent
   finalizers:
   - cnrm.cloud.google.com/finalizer
   - cnrm.cloud.google.com/deletion-defender
-  generation: 2
+  generation: 3
   labels:
     cnrm-test: "true"
     label-one: value-one
@@ -22,6 +23,7 @@ spec:
   networkRef:
     name: default
   portRange: "80"
+  resourceID: computeglobalforwardingrule-${uniqueId}
   target:
     targetHTTPProxyRef:
       name: computetargethttpproxy-2-${uniqueId}
@@ -32,8 +34,6 @@ status:
     reason: UpToDate
     status: "True"
     type: Ready
-  creationTimestamp: "1970-01-01T00:00:00Z"
-  externalRef: //compute.googleapis.com/projects/${projectId}/global/forwardingrules/computeglobalforwardingrule-${uniqueId}
   labelFingerprint: abcdef0123A=
-  observedGeneration: 2
+  observedGeneration: 3
   selfLink: https://www.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/computeglobalforwardingrule-${uniqueId}