From bf6bc4ee8be045497f68f668ce84cf6c9f2afdbd Mon Sep 17 00:00:00 2001 From: Gemma Hou Date: Tue, 27 Aug 2024 01:46:39 +0000 Subject: [PATCH] Address comments & update mocks --- .../mockcompute/regionalforwardingrulev1.go | 8 +- pkg/controller/direct/compute/client.go | 1 - .../compute/forwardingrule_controller.go | 37 +++--- .../globalcomputeforwardingrule/_http.log | 34 ++++++ .../regionalcomputeforwardingrule/_http.log | 110 +++++++++++++++--- 5 files changed, 147 insertions(+), 43 deletions(-) diff --git a/mockgcp/mockcompute/regionalforwardingrulev1.go b/mockgcp/mockcompute/regionalforwardingrulev1.go index 14d7ef5a8a..6f64832373 100644 --- a/mockgcp/mockcompute/regionalforwardingrulev1.go +++ b/mockgcp/mockcompute/regionalforwardingrulev1.go @@ -109,7 +109,7 @@ func (s *RegionalForwardingRulesV1) Insert(ctx context.Context, req *pb.InsertFo OperationType: PtrTo("insert"), User: PtrTo("user@example.com"), } - return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) { + return s.startRegionalLRO(ctx, name.Project.ID, name.Region, op, func() (proto.Message, error) { return obj, nil }) } @@ -134,7 +134,7 @@ func (s *RegionalForwardingRulesV1) Delete(ctx context.Context, req *pb.DeleteFo OperationType: PtrTo("delete"), User: PtrTo("user@example.com"), } - return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) { + return s.startRegionalLRO(ctx, name.Project.ID, name.Region, op, func() (proto.Message, error) { return deleted, nil }) } @@ -166,7 +166,7 @@ func (s *RegionalForwardingRulesV1) SetLabels(ctx context.Context, req *pb.SetLa // SetLabels operation has EndTime in response EndTime: PtrTo("2024-04-01T12:34:56.123456Z"), } - return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) { + return s.startRegionalLRO(ctx, name.Project.ID, name.Region, op, func() (proto.Message, error) { return obj, nil }) } @@ -196,7 +196,7 @@ func (s *RegionalForwardingRulesV1) SetTarget(ctx context.Context, req *pb.SetTa OperationType: PtrTo("SetTarget"), User: PtrTo("user@example.com"), } - return s.startGlobalLRO(ctx, name.Project.ID, op, func() (proto.Message, error) { + return s.startRegionalLRO(ctx, name.Project.ID, name.Region, op, func() (proto.Message, error) { return obj, nil }) } diff --git a/pkg/controller/direct/compute/client.go b/pkg/controller/direct/compute/client.go index 51eae5e28b..a65fbe4e08 100644 --- a/pkg/controller/direct/compute/client.go +++ b/pkg/controller/direct/compute/client.go @@ -71,7 +71,6 @@ type optionsRoundTripper struct { 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") } return m.inner.RoundTrip(req) } diff --git a/pkg/controller/direct/compute/forwardingrule_controller.go b/pkg/controller/direct/compute/forwardingrule_controller.go index b34a89302b..361b9d60f7 100644 --- a/pkg/controller/direct/compute/forwardingrule_controller.go +++ b/pkg/controller/direct/compute/forwardingrule_controller.go @@ -208,7 +208,7 @@ func (a *forwardingRuleAdapter) Find(ctx context.Context) (bool, error) { if direct.IsNotFound(err) { return false, nil } - return false, fmt.Errorf("getting ComputeForwardingRule %q failed: %w", a.fullyQualifiedName(), err) + return false, fmt.Errorf("getting ComputeForwardingRule %q: %w", a.fullyQualifiedName(), err) } } else { req := &computepb.GetForwardingRuleRequest{ @@ -221,7 +221,7 @@ func (a *forwardingRuleAdapter) Find(ctx context.Context) (bool, error) { if direct.IsNotFound(err) { return false, nil } - return false, fmt.Errorf("getting ComputeForwardingRule %q failed: %w", a.fullyQualifiedName(), err) + return false, fmt.Errorf("getting ComputeForwardingRule %q: %w", a.fullyQualifiedName(), err) } } a.actual = forwardingRule @@ -266,7 +266,11 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, u *unstructured.Unst op, err = a.forwardingRulesClient.Insert(ctx, req) } if err != nil { - return fmt.Errorf("creating ComputeForwardingRule %s failed: %w", a.fullyQualifiedName(), err) + 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) } log.V(2).Info("successfully created ComputeForwardingRule", "name", a.fullyQualifiedName()) // Get the created resource @@ -286,12 +290,12 @@ func (a *forwardingRuleAdapter) Create(ctx context.Context, u *unstructured.Unst created, err = a.forwardingRulesClient.Get(ctx, getReq) } if err != nil { - return fmt.Errorf("getting ComputeForwardingRule %q failed: %w", a.fullyQualifiedName(), err) + return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.fullyQualifiedName(), err) } status := &krm.ComputeForwardingRuleStatus{ LabelFingerprint: created.LabelFingerprint, - CreationTimestamp: op.Proto().InsertTime, + CreationTimestamp: created.CreationTimestamp, SelfLink: created.SelfLink, } status.ExternalRef = a.id.AsExternalRef() @@ -339,7 +343,7 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, u *unstructured.Unst op, err = a.forwardingRulesClient.SetLabels(ctx, setLabelsReq) } if err != nil { - return fmt.Errorf("updating ComputeForwardingRule labels %s failed: %w", a.fullyQualifiedName(), err) + return fmt.Errorf("updating ComputeForwardingRule labels %s: %w", a.fullyQualifiedName(), err) } err = op.Wait(ctx) if err != nil { @@ -362,7 +366,7 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, u *unstructured.Unst updated, err = a.forwardingRulesClient.Get(ctx, getReq) } if err != nil { - return fmt.Errorf("getting ComputeForwardingRule %q failed: %w", a.id.forwardingRule, err) + return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.id.forwardingRule, err) } // setTarget request is sent when there are updates to target. @@ -384,7 +388,7 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, u *unstructured.Unst op, err = a.forwardingRulesClient.SetTarget(ctx, setTargetReq) } if err != nil { - return fmt.Errorf("updating ComputeForwardingRule target %s failed: %w", a.fullyQualifiedName(), err) + return fmt.Errorf("updating ComputeForwardingRule target %s: %w", a.fullyQualifiedName(), err) } err = op.Wait(ctx) if err != nil { @@ -392,7 +396,6 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, u *unstructured.Unst } log.V(2).Info("successfully updated ComputeForwardingRule target", "name", a.fullyQualifiedName()) } - // Get the updated resource if a.id.location == "global" { getReq := &computepb.GetGlobalForwardingRuleRequest{ @@ -409,12 +412,12 @@ func (a *forwardingRuleAdapter) Update(ctx context.Context, u *unstructured.Unst updated, err = a.forwardingRulesClient.Get(ctx, getReq) } if err != nil { - return fmt.Errorf("getting ComputeForwardingRule %q failed: %w", a.id.forwardingRule, err) + return fmt.Errorf("getting ComputeForwardingRule %q: %w", a.id.forwardingRule, err) } status := &krm.ComputeForwardingRuleStatus{ LabelFingerprint: updated.LabelFingerprint, - CreationTimestamp: op.Proto().InsertTime, + CreationTimestamp: updated.CreationTimestamp, SelfLink: updated.SelfLink, } return setStatus(u, status) @@ -434,15 +437,7 @@ func (a *forwardingRuleAdapter) Delete(ctx context.Context) (bool, error) { log := klog.FromContext(ctx).WithName(ctrlName) log.V(2).Info("deleting ComputeForwardingRule", "name", a.id.forwardingRule) - exist, err := a.Find(ctx) - if err != nil { - return false, err - } - if !exist { - // return (false, nil) if the object was not found but should be presumed deleted. - return false, nil - } - + var err error op := &gcp.Operation{} if a.id.location == "global" { req := &computepb.DeleteGlobalForwardingRuleRequest{ @@ -459,7 +454,7 @@ func (a *forwardingRuleAdapter) Delete(ctx context.Context) (bool, error) { op, err = a.forwardingRulesClient.Delete(ctx, req) } if err != nil { - return false, fmt.Errorf("deleting ComputeForwardingRule %s failed: %w", a.id.forwardingRule, err) + return false, fmt.Errorf("deleting ComputeForwardingRule %s: %w", a.id.forwardingRule, err) } err = op.Wait(ctx) if err != nil { diff --git a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log index cc47a8cf09..398e273a35 100644 --- a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log +++ b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/globalcomputeforwardingrule/_http.log @@ -789,6 +789,40 @@ X-Xss-Protection: 0 --- +GET https://compute.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID} +Content-Type: application/json +User-Agent: kcc/controller-manager +x-goog-request-params: project=${projectId}&operation=${operationID} + +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": "insert", + "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": "user@example.com" +} + +--- + GET https://compute.googleapis.com/compute/v1/projects/${projectId}/global/forwardingRules/${forwardingRuleID} Content-Type: application/json User-Agent: kcc/controller-manager diff --git a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log index 0ba822593e..cade0a3ade 100644 --- a/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log +++ b/pkg/test/resourcefixture/testdata/basic/compute/v1beta1/computeforwardingrule/regionalcomputeforwardingrule/_http.log @@ -551,7 +551,8 @@ X-Xss-Protection: 0 "name": "${operationID}", "operationType": "insert", "progress": 0, - "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}", + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", "startTime": "2024-04-01T12:34:56.123456Z", "status": "RUNNING", "targetId": "${forwardingRulesId}", @@ -561,6 +562,41 @@ X-Xss-Protection: 0 --- +GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID} +Content-Type: application/json +User-Agent: kcc/controller-manager +x-goog-request-params: project=${projectId}®ion=us-central1&operation=${operationID} + +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": "insert", + "progress": 100, + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", + "startTime": "2024-04-01T12:34:56.123456Z", + "status": "DONE", + "targetId": "${forwardingRulesId}", + "targetLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/computeregionalforwardingrule-${uniqueId}", + "user": "user@example.com" +} + +--- + GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/${forwardingRuleID} Content-Type: application/json User-Agent: kcc/controller-manager @@ -633,7 +669,8 @@ X-Xss-Protection: 0 "name": "${operationID}", "operationType": "SetLabels", "progress": 0, - "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}", + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", "startTime": "2024-04-01T12:34:56.123456Z", "status": "RUNNING", "targetId": "${forwardingRulesId}", @@ -643,6 +680,41 @@ X-Xss-Protection: 0 --- +GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID} +Content-Type: application/json +User-Agent: kcc/controller-manager +x-goog-request-params: project=${projectId}®ion=us-central1&operation=${operationID} + +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, + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", + "startTime": "2024-04-01T12:34:56.123456Z", + "status": "DONE", + "targetId": "${forwardingRulesId}", + "targetLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/computeregionalforwardingrule-${uniqueId}", + "user": "user@example.com" +} + +--- + GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/${forwardingRuleID} Content-Type: application/json User-Agent: kcc/controller-manager @@ -705,7 +777,8 @@ X-Xss-Protection: 0 "name": "${operationID}", "operationType": "delete", "progress": 0, - "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/global/operations/${operationID}", + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", "startTime": "2024-04-01T12:34:56.123456Z", "status": "RUNNING", "targetId": "${forwardingRulesId}", @@ -715,12 +788,12 @@ X-Xss-Protection: 0 --- -GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/${forwardingRuleID} +GET https://compute.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID} Content-Type: application/json User-Agent: kcc/controller-manager -x-goog-request-params: project=${projectId}®ion=us-central1&forwarding_rule=computeregionalforwardingrule-${uniqueId} +x-goog-request-params: project=${projectId}®ion=us-central1&operation=${operationID} -404 Not Found +200 OK Cache-Control: private Content-Type: application/json; charset=UTF-8 Server: ESF @@ -732,17 +805,20 @@ X-Frame-Options: SAMEORIGIN X-Xss-Protection: 0 { - "error": { - "code": 404, - "errors": [ - { - "domain": "global", - "message": "forwardingRule \"projects/${projectId}/regions/us-central1/forwardingRules/computeregionalforwardingrule-${uniqueId}\" not found", - "reason": "notFound" - } - ], - "message": "forwardingRule \"projects/${projectId}/regions/us-central1/forwardingRules/computeregionalforwardingrule-${uniqueId}\" not found" - } + "endTime": "2024-04-01T12:34:56.123456Z", + "id": "000000000000000000000", + "insertTime": "2024-04-01T12:34:56.123456Z", + "kind": "compute#operation", + "name": "${operationID}", + "operationType": "delete", + "progress": 100, + "region": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1", + "selfLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/operations/${operationID}", + "startTime": "2024-04-01T12:34:56.123456Z", + "status": "DONE", + "targetId": "${forwardingRulesId}", + "targetLink": "https://www.googleapis.com/compute/v1/projects/${projectId}/regions/us-central1/forwardingRules/computeregionalforwardingrule-${uniqueId}", + "user": "user@example.com" } ---