From 7fa9f2d2f612ed5546671a1861492f1f3a3c4326 Mon Sep 17 00:00:00 2001 From: Gemma Hou Date: Thu, 10 Oct 2024 17:15:11 +0000 Subject: [PATCH] Use direct controller in tests --- .../v1beta1/firewallpolicyrule_types.go | 4 +- dev/tasks/run-e2e | 2 +- .../compute/firewallpolicyrule/client.go | 88 ------------------- .../firewallpolicyrule_controller.go | 69 ++++++--------- .../firewallpolicyrule_externalresource.go | 26 ------ .../direct/compute/firewallpolicyrule/refs.go | 1 + pkg/controller/direct/maputils.go | 4 +- 7 files changed, 34 insertions(+), 160 deletions(-) delete mode 100644 pkg/controller/direct/compute/firewallpolicyrule/client.go delete mode 100644 pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_externalresource.go diff --git a/apis/compute/v1beta1/firewallpolicyrule_types.go b/apis/compute/v1beta1/firewallpolicyrule_types.go index da8d6eac6e..b64b86b02e 100644 --- a/apis/compute/v1beta1/firewallpolicyrule_types.go +++ b/apis/compute/v1beta1/firewallpolicyrule_types.go @@ -25,8 +25,8 @@ import ( var ( ComputeFirewallPolicyRuleGVK = schema.GroupVersionKind{ - Group: SchemeGroupVersion.Group, - Version: SchemeGroupVersion.Version, + Group: GroupVersion.Group, + Version: GroupVersion.Version, Kind: "ComputeFirewallPolicyRule", } ) diff --git a/dev/tasks/run-e2e b/dev/tasks/run-e2e index c649780576..99462cbe78 100755 --- a/dev/tasks/run-e2e +++ b/dev/tasks/run-e2e @@ -26,7 +26,7 @@ if [[ -z "${KUBEBUILDER_ASSETS:-}" ]]; then fi if [[ -z "${KCC_USE_DIRECT_RECONCILERS:-}" ]]; then - KCC_USE_DIRECT_RECONCILERS=ComputeForwardingRule,GKEHubFeatureMembership,SecretManagerSecret + KCC_USE_DIRECT_RECONCILERS=ComputeFirewallPolicyRule,ComputeForwardingRule,GKEHubFeatureMembership,SecretManagerSecret fi echo "Using direct controllers: $KCC_USE_DIRECT_RECONCILERS" export KCC_USE_DIRECT_RECONCILERS diff --git a/pkg/controller/direct/compute/firewallpolicyrule/client.go b/pkg/controller/direct/compute/firewallpolicyrule/client.go deleted file mode 100644 index cbcc0207ae..0000000000 --- a/pkg/controller/direct/compute/firewallpolicyrule/client.go +++ /dev/null @@ -1,88 +0,0 @@ -// Copyright 2024 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package firewallpolicyrule - -import ( - "context" - "fmt" - "net/http" - - api "cloud.google.com/go/compute/apiv1" - "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/config" - "google.golang.org/api/option" -) - -type gcpClient struct { - config config.ControllerConfig -} - -func newGCPClient(ctx context.Context, config *config.ControllerConfig) (*gcpClient, error) { - gcpClient := &gcpClient{ - config: *config, - } - return gcpClient, nil -} - -func (m *gcpClient) options() ([]option.ClientOption, error) { - var opts []option.ClientOption - if m.config.UserAgent != "" { - opts = append(opts, option.WithUserAgent(m.config.UserAgent)) - } - if m.config.HTTPClient != nil { - // TODO: Set UserAgent in this scenario (error is: WithHTTPClient is incompatible with gRPC dial options) - - httpClient := &http.Client{} - *httpClient = *m.config.HTTPClient - httpClient.Transport = &optionsRoundTripper{ - config: m.config, - inner: m.config.HTTPClient.Transport, - } - opts = append(opts, option.WithHTTPClient(httpClient)) - } - if m.config.UserProjectOverride && m.config.BillingProject != "" { - opts = append(opts, option.WithQuotaProject(m.config.BillingProject)) - } - - // TODO: support endpoints? - // if m.config.Endpoint != "" { - // opts = append(opts, option.WithEndpoint(m.config.Endpoint)) - // } - - return opts, nil -} - -type optionsRoundTripper struct { - config config.ControllerConfig - inner http.RoundTripper -} - -func (m *optionsRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - if m.config.UserAgent != "" { - req.Header.Set("User-Agent", m.config.UserAgent) - } - return m.inner.RoundTrip(req) -} - -func (m *gcpClient) firewallPoliciesClient(ctx context.Context) (*api.FirewallPoliciesClient, error) { - opts, err := m.options() - if err != nil { - return nil, err - } - client, err := api.NewFirewallPoliciesRESTClient(ctx, opts...) - if err != nil { - return nil, fmt.Errorf("building FirewallPolicy client: %w", err) - } - return client, err -} diff --git a/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_controller.go b/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_controller.go index b811338249..d8efc515e9 100644 --- a/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_controller.go +++ b/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_controller.go @@ -18,6 +18,8 @@ import ( "context" "fmt" + "google.golang.org/api/option" + gcp "cloud.google.com/go/compute/apiv1" computepb "cloud.google.com/go/compute/apiv1/computepb" krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/compute/v1beta1" @@ -59,6 +61,19 @@ type firewallPolicyRuleAdapter struct { var _ directbase.Adapter = &firewallPolicyRuleAdapter{} +func (m *firewallPolicyRuleModel) client(ctx context.Context) (*gcp.FirewallPoliciesClient, error) { + var opts []option.ClientOption + opts, err := m.config.RESTClientOptions() + if err != nil { + return nil, err + } + gcpClient, err := gcp.NewFirewallPoliciesRESTClient(ctx, opts...) + if err != nil { + return nil, fmt.Errorf("building FirewallPolicy client: %w", err) + } + return gcpClient, err +} + func (m *firewallPolicyRuleModel) AdapterForObject(ctx context.Context, reader client.Reader, u *unstructured.Unstructured) (directbase.Adapter, error) { obj := &krm.ComputeFirewallPolicyRule{} if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &obj); err != nil { @@ -88,16 +103,11 @@ func (m *firewallPolicyRuleModel) AdapterForObject(ctx context.Context, reader c } // Get GCP client - gcpClient, err := newGCPClient(ctx, m.config) + gcpClient, err := m.client(ctx) if err != nil { return nil, fmt.Errorf("building gcp client: %w", err) } - - firewallPoliciesClient, err := gcpClient.firewallPoliciesClient(ctx) - if err != nil { - return nil, err - } - firewallPolicyRuleAdapter.firewallPoliciesClient = firewallPoliciesClient + firewallPolicyRuleAdapter.firewallPoliciesClient = gcpClient return firewallPolicyRuleAdapter, nil } @@ -113,10 +123,10 @@ func (a *firewallPolicyRuleAdapter) Find(ctx context.Context) (bool, error) { firewallPolicyRule, err := a.get(ctx) if err != nil { - // When a certain rule does not exist, the error has code 400(invalid) instead of 404(not found) + // When a certain rule does not exist, the error has code 400(bad request) instead of 404(not found) // example error message: // "Invalid value for field 'priority': '9000'. The firewall policy does not contain a rule at priority 9000.", - if direct.IsInvalidValue(err) { + if direct.IsBadRequest(err) { return false, nil } return false, fmt.Errorf("getting ComputeFirewallPolicyRule %d: %w", a.priority, err) @@ -126,7 +136,6 @@ func (a *firewallPolicyRuleAdapter) Find(ctx context.Context) (bool, error) { } func (a *firewallPolicyRuleAdapter) Create(ctx context.Context, createOp *directbase.CreateOperation) error { - u := createOp.GetUnstructured() var err error err = resolveDependencies(ctx, a.reader, a.desired) @@ -169,15 +178,12 @@ func (a *firewallPolicyRuleAdapter) Create(ctx context.Context, createOp *direct return fmt.Errorf("getting ComputeFirewallPolicyRule %d: %w", a.priority, err) } - status := &krm.ComputeFirewallPolicyRuleStatus{ - RuleTupleCount: direct.PtrTo(int64(*created.RuleTupleCount)), - Kind: direct.PtrTo("compute#firewallPolicyRule"), - } - return setStatus(u, status) + status := &krm.ComputeFirewallPolicyRuleStatus{} + status = ComputeFirewallPolicyRuleStatus_FromProto(mapCtx, created) + return createOp.UpdateStatus(ctx, status, nil) } func (a *firewallPolicyRuleAdapter) Update(ctx context.Context, updateOp *directbase.UpdateOperation) error { - u := updateOp.GetUnstructured() var err error err = resolveDependencies(ctx, a.reader, a.desired) @@ -194,9 +200,10 @@ func (a *firewallPolicyRuleAdapter) Update(ctx context.Context, updateOp *direct if mapCtx.Err() != nil { return mapCtx.Err() } + // The field priority should be removed from the patch request body and included as a query parameter. + // See API doc: https://cloud.google.com/compute/docs/reference/rest/v1/firewallPolicies/patchRule#query-parameters firewallPolicyRule.Priority = nil - op := &gcp.Operation{} updated := &computepb.FirewallPolicyRule{} updateReq := &computepb.PatchRuleFirewallPolicyRequest{ @@ -204,7 +211,7 @@ func (a *firewallPolicyRuleAdapter) Update(ctx context.Context, updateOp *direct FirewallPolicy: a.firewallPolicy, Priority: direct.PtrTo(int32(a.priority)), } - op, err = a.firewallPoliciesClient.PatchRule(ctx, updateReq) + op, err := a.firewallPoliciesClient.PatchRule(ctx, updateReq) if err != nil { return fmt.Errorf("updating ComputeFirewallPolicyRule %d: %w", a.priority, err) } @@ -222,11 +229,9 @@ func (a *firewallPolicyRuleAdapter) Update(ctx context.Context, updateOp *direct return fmt.Errorf("getting ComputeFirewallPolicyRule %d: %w", a.priority, err) } - status := &krm.ComputeFirewallPolicyRuleStatus{ - RuleTupleCount: direct.PtrTo(int64(*updated.RuleTupleCount)), - Kind: direct.PtrTo("compute#firewallPolicyRule"), - } - return setStatus(u, status) + status := &krm.ComputeFirewallPolicyRuleStatus{} + status = ComputeFirewallPolicyRuleStatus_FromProto(mapCtx, updated) + return updateOp.UpdateStatus(ctx, status, nil) } func (a *firewallPolicyRuleAdapter) Export(ctx context.Context) (*unstructured.Unstructured, error) { @@ -290,21 +295,3 @@ func (a *firewallPolicyRuleAdapter) get(ctx context.Context) (*computepb.Firewal } return a.firewallPoliciesClient.GetRule(ctx, getReq) } - -func setStatus(u *unstructured.Unstructured, typedStatus any) error { - status, err := runtime.DefaultUnstructuredConverter.ToUnstructured(typedStatus) - if err != nil { - return fmt.Errorf("error converting status to unstructured: %w", err) - } - - old, _, _ := unstructured.NestedMap(u.Object, "status") - if old != nil { - status["conditions"] = old["conditions"] - status["observedGeneration"] = old["observedGeneration"] - status["externalRef"] = old["externalRef"] - } - - u.Object["status"] = status - - return nil -} diff --git a/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_externalresource.go b/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_externalresource.go deleted file mode 100644 index fc8edc4366..0000000000 --- a/pkg/controller/direct/compute/firewallpolicyrule/firewallpolicyrule_externalresource.go +++ /dev/null @@ -1,26 +0,0 @@ -/* -Copyright 2024. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package firewallpolicyrule - -const ( - serviceDomain = "//compute.googleapis.com" -) - -type FirewallPolicyRuleIdentity struct { - firewallPolicy string - priority int64 -} diff --git a/pkg/controller/direct/compute/firewallpolicyrule/refs.go b/pkg/controller/direct/compute/firewallpolicyrule/refs.go index cdb3e61ab5..6b19ad6a0c 100644 --- a/pkg/controller/direct/compute/firewallpolicyrule/refs.go +++ b/pkg/controller/direct/compute/firewallpolicyrule/refs.go @@ -17,6 +17,7 @@ package firewallpolicyrule import ( "context" "fmt" + krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/compute/v1beta1" refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1" diff --git a/pkg/controller/direct/maputils.go b/pkg/controller/direct/maputils.go index 43796127dd..34ba5a03f5 100644 --- a/pkg/controller/direct/maputils.go +++ b/pkg/controller/direct/maputils.go @@ -228,8 +228,8 @@ func IsNotFound(err error) bool { return HasHTTPCode(err, 404) } -// IsInvalidValue returns true if the given error is an HTTP 400. -func IsInvalidValue(err error) bool { +// IsBadRequest returns true if the given error is an HTTP 400. +func IsBadRequest(err error) bool { return HasHTTPCode(err, 400) }