feat: Direct controller for ComputeForwardingRule #2364

gemmahou · 2024-07-26T00:21:23Z

Change description

Tests you have done

Run make ready-pr to ensure this PR is ready for review.
Perform necessary E2E testing for changed resources.

gemmahou · 2024-08-03T00:14:09Z

...testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log

  },
  "loadBalancingScheme": "EXTERNAL",
  "name": "computeregionalforwardingrule-${uniqueId}",
-  "region": "projects/${projectId}/global/regions/us-central1",


This region looks problematic. I will investigate where the value comes from.

It may not be an issue because it's in the URL

...ngrule/globalcomputeforwardingrule/_generated_object_globalcomputeforwardingrule.golden.yaml

...le/regionalcomputeforwardingrule/_generated_object_regionalcomputeforwardingrule.golden.yaml

apis/compute/v1beta1/computeforwardingrule_types.go

apis/refs/v1beta1/computerefs.go

...e/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log

...le/regionalcomputeforwardingrule/_generated_object_regionalcomputeforwardingrule.golden.yaml

pkg/controller/direct/cloudbuild/workerpool_controller.go

yuwenma · 2024-08-07T04:54:09Z

pkg/controller/direct/compute/forwardingrule_controller.go

+	}
+	log.V(2).Info("successfully created ComputeForwardingRule", "name", a.fullyQualifiedName())
+	// Get the created resource
+	created := &computepb.ForwardingRule{}


The Insert returns a computepb.Operation, which op.Status could be either OpDone or OpPending Code. I think we shall probably only proceed the following GCP Get when we get a OpDone.

For OpPending, we probably want to return quietly and update the KCC .status in the next Reconcile, maybe Find/Update? @justinsb I'll vote for Update, do you have a preference?

I won't worry about this because we call op.Wait()(source code) a few lines above, and it makes sure that the operation is Done.

pkg/controller/direct/compute/forwardingrule_controller.go

justinsb · 2024-08-26T17:22:45Z

pkg/controller/direct/compute/client.go

+func (m *optionsRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	if m.config.UserAgent != "" {
+		req.Header.Set("User-Agent", m.config.UserAgent)
+		req.Header.Del("x-goog-request-params")


Do we know what this header is?

hmmm I'm actually unsure why this added... and seems it's not deleted by Del. I also noticed that other direct resources(monitoringdashboard, cloudbuildworkerpool) have this header as well, I would guess it comes from the underlying GCP?

justinsb · 2024-08-26T17:24:34Z

pkg/controller/direct/compute/forwardingrule_controller.go

+			return nil, err
+
+		}
+		obj.Spec.NetworkRef.External = networkRef.External


Nit: you may want to clear the other fields in the NetworkRef. But not a big deal, and we may be able to move all reference-resolution into a shared function.

Do you mean cleaning up other fields like Name in obj.Spec.NetworkRef? I won't worry about other fields because we get value from External only, see the mapping function: code

Right, it might be clearer/safer if you did clear all those other fields, it's a nit because I agree it shouldn't change functionality.

justinsb · 2024-08-26T17:27:44Z

pkg/controller/direct/compute/forwardingrule_controller.go

+			return nil, err
+		}
+
+		if id.project != projectID {


Is this equivalent to BuildID(...) != asID(externalRef) ?

Yes, but this way, we have a better idea (via separate error messages) of which specific field has been changed.

pkg/controller/direct/compute/forwardingrule_controller.go

justinsb · 2024-08-26T17:41:48Z

pkg/controller/direct/compute/forwardingrule_controller.go

+	}
+	log.V(2).Info("successfully updated ComputeForwardingRule labels", "name", a.fullyQualifiedName())
+	// Get the updated resource
+	if a.id.location == "global" {


Do we have to get it here, we also get it a few lines down....

Call Get twice, once after updating labels and once after updating targets, to match the realGCP http log.

pkg/controller/direct/compute/forwardingrule_controller.go

justinsb · 2024-08-26T19:01:06Z

...ngrule/globalcomputeforwardingrule/_generated_object_globalcomputeforwardingrule.golden.yaml

@@ -37,7 +22,6 @@ spec:
  networkRef:
    name: default
  portRange: "80"
-  resourceID: computeglobalforwardingrule-${uniqueId}


Hmmm... maybe we don't then :-) I was worried about compatibility, but as it's not server-generated I don't think it will trip anyone up!

justinsb · 2024-08-26T19:02:30Z

...testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log

  },
  "loadBalancingScheme": "EXTERNAL",
  "name": "computeregionalforwardingrule-${uniqueId}",
-  "region": "projects/${projectId}/global/regions/us-central1",


It may not be an issue because it's in the URL

justinsb · 2024-08-26T19:03:19Z

...testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log


 {
  "labelFingerprint": "abcdef0123A=",
  "labels": {
    "cnrm-test": "true",
-    "label-one": "value-one",
+    "label-one": "value-two",


This is the "always calls setLabels" behaviour you were talking about with the old controller?

justinsb · 2024-08-26T19:04:20Z

...testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log

@@ -776,10 +715,12 @@ X-Xss-Protection: 0

 ---

-GET https://compute.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}?alt=json&prettyPrint=false


I had thought this looked like we weren't checking the LRO on delete, but ... we are.

Updated the log.
I think that's because I made a mistake in mockgcp/mockcompute/regionalforwardingrulev1.go: Should use return s.startRegionalLRO not return s.startGlobalLRO. I updated the mock as well.

justinsb · 2024-08-26T19:06:22Z

Looks good, a few questions but nothing blocking I don't think! Will LGTM but add hold, feel free to address questions here or in a separate PR as you see fit.

/approve
/lgtm
/hold

justinsb · 2024-08-28T19:17:22Z

pkg/controller/direct/compute/forwardingrule_controller.go

+			return nil, err
+
+		}
+		obj.Spec.NetworkRef.External = networkRef.External


Right, it might be clearer/safer if you did clear all those other fields, it's a nit because I agree it shouldn't change functionality.

google-oss-prow · 2024-08-28T19:24:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow bot added the do-not-merge/work-in-progress label Jul 26, 2024

gemmahou force-pushed the direct-controller branch 8 times, most recently from 64c01cf to 8be07af Compare August 3, 2024 00:12

gemmahou commented Aug 3, 2024

View reviewed changes

yuwenma reviewed Aug 5, 2024

View reviewed changes

...ngrule/globalcomputeforwardingrule/_generated_object_globalcomputeforwardingrule.golden.yaml Show resolved Hide resolved

yuwenma reviewed Aug 5, 2024

View reviewed changes

...le/regionalcomputeforwardingrule/_generated_object_regionalcomputeforwardingrule.golden.yaml Show resolved Hide resolved

yuwenma reviewed Aug 5, 2024

View reviewed changes

...le/regionalcomputeforwardingrule/_generated_object_regionalcomputeforwardingrule.golden.yaml Show resolved Hide resolved

justinsb reviewed Aug 5, 2024

View reviewed changes

gemmahou force-pushed the direct-controller branch from 8be07af to 9fdbedc Compare August 5, 2024 17:50

yuwenma reviewed Aug 5, 2024

View reviewed changes

pkg/controller/direct/cloudbuild/workerpool_controller.go Outdated Show resolved Hide resolved

gemmahou force-pushed the direct-controller branch from 9fdbedc to eea1f1c Compare August 5, 2024 22:01

gemmahou marked this pull request as ready for review August 5, 2024 22:02

gemmahou force-pushed the direct-controller branch 3 times, most recently from 9925f9e to 6ee76cb Compare August 6, 2024 23:55

yuwenma reviewed Aug 7, 2024

View reviewed changes

pkg/controller/direct/compute/forwardingrule_controller.go Show resolved Hide resolved

yuwenma reviewed Aug 7, 2024

View reviewed changes

pkg/controller/direct/compute/forwardingrule_controller.go Outdated Show resolved Hide resolved

pkg/controller/direct/compute/forwardingrule_controller.go Outdated Show resolved Hide resolved

gemmahou added the service:compute label Aug 13, 2024

gemmahou mentioned this pull request Aug 20, 2024

direct: ComputeForwardingRule refs #2545

Merged

2 tasks

gemmahou force-pushed the direct-controller branch 4 times, most recently from c51505c to 8c304c6 Compare August 22, 2024 22:59

gemmahou force-pushed the direct-controller branch from 8c304c6 to 6c82d9c Compare August 22, 2024 23:03

direct controller for ComputeForwardingRule

1252cb0

gemmahou force-pushed the direct-controller branch 2 times, most recently from 924dd21 to 4588327 Compare August 23, 2024 21:55

Support status.ExternalRef

20b8fe4

gemmahou force-pushed the direct-controller branch from 4588327 to 20b8fe4 Compare August 26, 2024 17:13

gemmahou changed the title ~~WIP: feat: Direct controller for ComputeForwardingRule~~ feat: Direct controller for ComputeForwardingRule Aug 26, 2024

google-oss-prow bot removed the do-not-merge/work-in-progress label Aug 26, 2024

justinsb reviewed Aug 26, 2024

View reviewed changes

google-oss-prow bot added the do-not-merge/hold label Aug 26, 2024

google-oss-prow bot assigned justinsb Aug 26, 2024

google-oss-prow bot added lgtm approved and removed lgtm labels Aug 26, 2024

Address comments & update mocks

bf6bc4e

gemmahou force-pushed the direct-controller branch from a6f7a2e to bf6bc4e Compare August 28, 2024 19:19

justinsb approved these changes Aug 28, 2024

View reviewed changes

google-oss-prow bot added the lgtm label Aug 28, 2024

gemmahou removed the do-not-merge/hold label Aug 28, 2024

google-oss-prow bot merged commit 240941d into GoogleCloudPlatform:master Aug 28, 2024
13 checks passed

gemmahou deleted the direct-controller branch August 28, 2024 23:41

yuwenma added this to the 1.122 milestone Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Direct controller for ComputeForwardingRule #2364

feat: Direct controller for ComputeForwardingRule #2364

gemmahou commented Jul 26, 2024 •

edited

Loading

gemmahou Aug 3, 2024

justinsb Aug 26, 2024

yuwenma Aug 7, 2024

gemmahou Aug 28, 2024 •

edited

Loading

justinsb Aug 26, 2024

gemmahou Aug 27, 2024

gemmahou Aug 28, 2024

justinsb Aug 26, 2024

gemmahou Aug 27, 2024

justinsb Aug 28, 2024

justinsb Aug 26, 2024

gemmahou Aug 27, 2024

justinsb Aug 26, 2024

gemmahou Aug 27, 2024

justinsb Aug 26, 2024

justinsb Aug 26, 2024

justinsb Aug 26, 2024

justinsb Aug 26, 2024

gemmahou Aug 27, 2024

justinsb commented Aug 26, 2024

justinsb Aug 28, 2024

google-oss-prow bot commented Aug 28, 2024

		@@ -776,10 +715,12 @@ X-Xss-Protection: 0

		---

		GET https://compute.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}?alt=json&prettyPrint=false

feat: Direct controller for ComputeForwardingRule #2364

feat: Direct controller for ComputeForwardingRule #2364

Conversation

gemmahou commented Jul 26, 2024 • edited Loading

Change description

Tests you have done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gemmahou Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Aug 26, 2024

Choose a reason for hiding this comment

google-oss-prow bot commented Aug 28, 2024

gemmahou commented Jul 26, 2024 •

edited

Loading

gemmahou Aug 28, 2024 •

edited

Loading