diff --git a/apis/cloudbuild/v1alpha1/workerpool_types.go b/apis/cloudbuild/v1alpha1/workerpool_types.go index b98f8842a70..e2142346515 100644 --- a/apis/cloudbuild/v1alpha1/workerpool_types.go +++ b/apis/cloudbuild/v1alpha1/workerpool_types.go @@ -68,7 +68,10 @@ type CloudBuildWorkerPoolStatus struct { // +optional ObservedGeneration *int64 `json:"observedGeneration,omitempty"` - ResourceID *string `json:"resourceID,omitempty"` + /* The GCP URL to the cloudbuild workerpool resource. + The cloudbuild workerpool does not have selfLink. */ + // +optional + ExternalRef *string `json:"externalRef,omitempty"` /* ObservedState is the state of the resource as most recently observed in GCP. */ // +optional @@ -85,6 +88,7 @@ type CloudBuildWorkerPoolObservedState struct { // +optional // +kubebuilder:validation:Format=date-time UpdateTime *string `json:"updateTime,omitempty"` + // +required WorkerConfig *WorkerConfig `json:"workerConfig,omitempty"` NetworkConfig *NetworkConfigState `json:"networkConfig,omitempty"` diff --git a/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go b/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go index 4c688371a1e..8d095bc6ec0 100644 --- a/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go +++ b/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go @@ -147,8 +147,8 @@ func (in *CloudBuildWorkerPoolStatus) DeepCopyInto(out *CloudBuildWorkerPoolStat *out = new(int64) **out = **in } - if in.ResourceID != nil { - in, out := &in.ResourceID, &out.ResourceID + if in.ExternalRef != nil { + in, out := &in.ExternalRef, &out.ExternalRef *out = new(string) **out = **in } diff --git a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml index 2fc2ff23ded..1ed1d8676c2 100644 --- a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml +++ b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml @@ -164,6 +164,10 @@ spec: type: string type: object type: array + externalRef: + description: The GCP URL to the cloudbuild workerpool resource. The + cloudbuild workerpool does not have selfLink. + type: string observedGeneration: description: ObservedGeneration is the generation of the resource that was most recently observed by the Config Connector controller. @@ -204,8 +208,6 @@ spec: required: - workerConfig type: object - resourceID: - type: string type: object type: object served: true diff --git a/pkg/clients/generated/apis/cloudbuild/v1alpha1/cloudbuildworkerpool_types.go b/pkg/clients/generated/apis/cloudbuild/v1alpha1/cloudbuildworkerpool_types.go index 19a5a299241..5c8f4adfee0 100644 --- a/pkg/clients/generated/apis/cloudbuild/v1alpha1/cloudbuildworkerpool_types.go +++ b/pkg/clients/generated/apis/cloudbuild/v1alpha1/cloudbuildworkerpool_types.go @@ -116,6 +116,10 @@ type CloudBuildWorkerPoolStatus struct { /* Conditions represent the latest available observations of the CloudBuildWorkerPool's current state. */ Conditions []v1alpha1.Condition `json:"conditions,omitempty"` + /* The GCP URL to the cloudbuild workerpool resource. The cloudbuild workerpool does not have selfLink. */ + // +optional + ExternalRef *string `json:"externalRef,omitempty"` + /* ObservedGeneration is the generation of the resource that was most recently observed by the Config Connector controller. If this is equal to metadata.generation, then that means that the current reported status reflects the most recent desired state of the resource. */ // +optional ObservedGeneration *int64 `json:"observedGeneration,omitempty"` @@ -123,9 +127,6 @@ type CloudBuildWorkerPoolStatus struct { /* ObservedState is the state of the resource as most recently observed in GCP. */ // +optional ObservedState *WorkerpoolObservedStateStatus `json:"observedState,omitempty"` - - // +optional - ResourceID *string `json:"resourceID,omitempty"` } // +genclient diff --git a/pkg/clients/generated/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go b/pkg/clients/generated/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go index ba89cb99dca..1f02d6f5d5e 100644 --- a/pkg/clients/generated/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/clients/generated/apis/cloudbuild/v1alpha1/zz_generated.deepcopy.go @@ -131,6 +131,11 @@ func (in *CloudBuildWorkerPoolStatus) DeepCopyInto(out *CloudBuildWorkerPoolStat *out = make([]k8sv1alpha1.Condition, len(*in)) copy(*out, *in) } + if in.ExternalRef != nil { + in, out := &in.ExternalRef, &out.ExternalRef + *out = new(string) + **out = **in + } if in.ObservedGeneration != nil { in, out := &in.ObservedGeneration, &out.ObservedGeneration *out = new(int64) @@ -141,11 +146,6 @@ func (in *CloudBuildWorkerPoolStatus) DeepCopyInto(out *CloudBuildWorkerPoolStat *out = new(WorkerpoolObservedStateStatus) (*in).DeepCopyInto(*out) } - if in.ResourceID != nil { - in, out := &in.ResourceID, &out.ResourceID - *out = new(string) - **out = **in - } return } diff --git a/pkg/controller/direct/cloudbuild/externalresource.go b/pkg/controller/direct/cloudbuild/externalresource.go new file mode 100644 index 00000000000..149a7e0ddb8 --- /dev/null +++ b/pkg/controller/direct/cloudbuild/externalresource.go @@ -0,0 +1,51 @@ +/* +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 cloudbuild + +import ( + "strings" + + cloudbuildpb "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb" + "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/externalresource" +) + +func NewResourceRef(gcpObj *cloudbuildpb.WorkerPool) *ResourceRef { + return &ResourceRef{ + externalresource.New("https://cloudbuild.googleapis.com/v1/", gcpObj), + } +} + +type ResourceRef struct { + *externalresource.ExternalResourceReference +} + +func (e *ResourceRef) GetResourceID() string { + segments := strings.Split(*e.Get(), "/workerPools/") + return segments[1] +} + +func (e *ResourceRef) GetLocation() string { + segments := strings.Split(*e.Get(), "/locations/") + segment := strings.Split(segments[1], "/workerPools/") + return segment[0] +} + +func (e *ResourceRef) GetProject() string { + segments := strings.Split(*e.Get(), "/projects/") + segment := strings.Split(segments[1], "/locations/") + return segment[0] +} diff --git a/pkg/controller/direct/cloudbuild/workerpool_controller.go b/pkg/controller/direct/cloudbuild/workerpool_controller.go index fce6fee12fc..29b2c6dcb52 100644 --- a/pkg/controller/direct/cloudbuild/workerpool_controller.go +++ b/pkg/controller/direct/cloudbuild/workerpool_controller.go @@ -197,10 +197,14 @@ func (a *Adapter) Create(ctx context.Context, u *unstructured.Unstructured) erro } status.ObservedState.CreateTime = ToOpenAPIDateTime(created.GetCreateTime()) status.ObservedState.UpdateTime = ToOpenAPIDateTime(created.GetUpdateTime()) + status.ExternalRef = NewResourceRef(created).Get() return setStatus(u, status) } func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) error { + if err := a.ValidateExternalResource(); err != nil { + return err + } updateMask := &fieldmaskpb.FieldMask{} @@ -283,6 +287,8 @@ func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) erro } status.ObservedState.CreateTime = ToOpenAPIDateTime(updated.GetCreateTime()) status.ObservedState.UpdateTime = ToOpenAPIDateTime(updated.GetUpdateTime()) + // This value should not be updated. Just in case. + status.ExternalRef = NewResourceRef(updated).Get() return setStatus(u, status) } @@ -292,9 +298,10 @@ func (a *Adapter) Export(ctx context.Context) (*unstructured.Unstructured, error // Delete implements the Adapter interface. func (a *Adapter) Delete(ctx context.Context) (bool, error) { - if a.resourceID == "" { - return false, nil + if err := a.ValidateExternalResource(); err != nil { + return false, err } + req := &cloudbuildpb.DeleteWorkerPoolRequest{Name: a.fullyQualifiedName(), AllowMissing: true} op, err := a.gcpClient.DeleteWorkerPool(ctx, req) if err != nil { @@ -313,6 +320,38 @@ func (a *Adapter) Delete(ctx context.Context) (bool, error) { return true, nil } +// ValidateExternalResource compares the `status.externalRef` with the `spec` Project, Location and +// (external) resourceID to make sure those fields are immutable and matches the previous deployed value. +func (a *Adapter) ValidateExternalResource() error { + actualExternalRef := NewResourceRef(a.actual) + + desiredExternalRef := "https://cloudbuild.googleapis.com/v1/" + a.fullyQualifiedName() + if *actualExternalRef.Get() == desiredExternalRef { + return nil + } + + // Give user guidance on how to fix the CloudBuildWorkerPool spec. + if a.desired.Spec.ResourceID != nil && *a.desired.Spec.ResourceID != actualExternalRef.GetResourceID() { + return fmt.Errorf("`spec.resourceID` is immutable field, expect %s, got %s", + actualExternalRef.GetResourceID(), *a.desired.Spec.ResourceID) + } + if a.desired.Spec.Location != actualExternalRef.GetLocation() { + return fmt.Errorf("`spec.location` is immutable field, expect %s, got %s", + actualExternalRef.GetLocation(), a.desired.Spec.Location) + } + // TODO: Some Selflink may change the project from projectID to projectNum. + /* + if a.desired.Spec.ProjectRef.Name != "" { + return fmt.Errorf("`spec.projectRef.name` is immutable field, expect project %s", + actualExternalRef.GetProject()) + } + if a.desired.Spec.ProjectRef.External != "" { + return fmt.Errorf("`spec.projectRef.external` is immutable field, expect project %s", + actualExternalRef.GetProject()) + }*/ + return nil +} + func (a *Adapter) fullyQualifiedName() string { return fmt.Sprintf("projects/%s/locations/%s/workerPools/%s", a.projectID, a.location, a.resourceID) } diff --git a/pkg/controller/direct/directbase/directbase_controller.go b/pkg/controller/direct/directbase/directbase_controller.go index f0884b4feff..938320754ae 100644 --- a/pkg/controller/direct/directbase/directbase_controller.go +++ b/pkg/controller/direct/directbase/directbase_controller.go @@ -225,6 +225,18 @@ func (r *reconcileContext) doReconcile(ctx context.Context, u *unstructured.Unst return false, r.handleUpdateFailed(ctx, u, err) } + // To create, update or delete the GCP object, we need to get the GCP object first. + // Because the object contains the cloud service information like `selfLink` `ID` required to validate + // the resource uniqueness before updating/deleting. + existsAlready, err := adapter.Find(ctx) + if err != nil { + if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok { + logger.Info(unwrappedErr.Error(), "resource", k8s.GetNamespacedName(u)) + return r.handleUnresolvableDeps(ctx, u, unwrappedErr) + } + return false, r.handleUpdateFailed(ctx, u, err) + } + defer execution.RecoverWithInternalError(&err) if !u.GetDeletionTimestamp().IsZero() { if !k8s.HasFinalizer(u, k8s.ControllerFinalizerName) { @@ -255,14 +267,6 @@ func (r *reconcileContext) doReconcile(ctx context.Context, u *unstructured.Unst return false, r.handleDeleted(ctx, u) } - existsAlready, err := adapter.Find(ctx) - if err != nil { - if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok { - logger.Info(unwrappedErr.Error(), "resource", k8s.GetNamespacedName(u)) - return r.handleUnresolvableDeps(ctx, u, unwrappedErr) - } - return false, r.handleUpdateFailed(ctx, u, err) - } k8s.EnsureFinalizers(u, k8s.ControllerFinalizerName, k8s.DeletionDefenderFinalizerName) // set the etag to an empty string, since IAMPolicy is the authoritative intent, KCC wants to overwrite the underlying policy regardless diff --git a/pkg/controller/direct/externalresource/resource.go b/pkg/controller/direct/externalresource/resource.go new file mode 100644 index 00000000000..f9280674f72 --- /dev/null +++ b/pkg/controller/direct/externalresource/resource.go @@ -0,0 +1,64 @@ +/* +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 externalresource + +import ( + "reflect" +) + +type ExternalResourceReference struct { + hasSelfLink bool + externalRef string +} + +type GCPResource interface { + GetName() string +} + +// New builds the External Reference for CloudBuildWorkerPool when the object already exists on the GCP server. +// Ideally, it expects to use the GCP object's selfLink value as the external reference. However, if +// the resource does not have `selfLink` (like CloudBuildWorkerPool), it builds the URL from the GCP object. +func New(host string, gcpObj GCPResource) *ExternalResourceReference { + baseExternalRef := &ExternalResourceReference{ + hasSelfLink: false, + externalRef: host + gcpObj.GetName(), + } + // Ignore the actual GCP proto, get the `selfLink` in general. + s := reflect.ValueOf(gcpObj).Elem() + if s.Kind() != reflect.Struct { + return baseExternalRef + } + + selfLink := s.FieldByName("SelfLink") + + if !selfLink.IsValid() || selfLink.IsZero() { + return baseExternalRef + } + switch selfLink.Kind() { + case reflect.String: + baseExternalRef.hasSelfLink = true + baseExternalRef.externalRef = selfLink.String() + case reflect.Pointer: + baseExternalRef.hasSelfLink = true + baseExternalRef.externalRef = selfLink.Elem().String() + } + return baseExternalRef +} + +func (e *ExternalResourceReference) Get() *string { + return &e.externalRef +} diff --git a/pkg/controller/direct/externalresource/resource_test.go b/pkg/controller/direct/externalresource/resource_test.go new file mode 100644 index 00000000000..07466f70b15 --- /dev/null +++ b/pkg/controller/direct/externalresource/resource_test.go @@ -0,0 +1,61 @@ +/* +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 externalresource + +import ( + "testing" + + "cloud.google.com/go/cloudbuild/apiv1/v2/cloudbuildpb" + "cloud.google.com/go/compute/apiv1/computepb" +) + +var ( + networkSelfLink = "https://www.googleapis.com/mockservice/v1/projects/yuwenma-gke-dev/global/networks/computenetwork-52ldcmpbgxfmizhrk74q" +) + +func TestNew(t *testing.T) { + tests := []struct { + name string + gcpResource GCPResource + expectedHasSelfLink bool + expectedExternalRef string + }{ + { + name: "cloudbuild workerpool, no selfLink", + gcpResource: &cloudbuildpb.WorkerPool{Name: "projects/mock-project/locations/us-central1/workerPools/mock-pool"}, + expectedHasSelfLink: false, + expectedExternalRef: "https://www.googleapis.com/mockservice/v1/projects/mock-project/locations/us-central1/workerPools/mock-pool", + }, + { + name: "compute network, has seflLink as string pointer", + gcpResource: &computepb.Network{SelfLink: &networkSelfLink}, + expectedHasSelfLink: true, + expectedExternalRef: networkSelfLink, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gcpObj := test.gcpResource + actualRef := New("https://www.googleapis.com/mockservice/v1/", gcpObj) + if actualRef.hasSelfLink != test.expectedHasSelfLink { + t.Errorf("expected %v, got %v", test.expectedHasSelfLink, actualRef.hasSelfLink) + } + if actualRef.externalRef != test.expectedExternalRef { + t.Errorf("expected %v, got %v", test.expectedExternalRef, actualRef.externalRef) + } + }) + } +} diff --git a/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_generated_object_cloudbuildworkerpool.golden.yaml b/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_generated_object_cloudbuildworkerpool.golden.yaml index 843b50127f1..9619a1eefec 100644 --- a/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_generated_object_cloudbuildworkerpool.golden.yaml +++ b/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_generated_object_cloudbuildworkerpool.golden.yaml @@ -30,6 +30,7 @@ status: reason: UpToDate status: "True" type: Ready + externalRef: https://cloudbuild.googleapis.com/v1/projects/${projectId}/locations/us-central1/workerPools/cloudbuildworkerpool-${uniqueId} observedGeneration: 2 observedState: createTime: "1970-01-01T00:00:00Z" diff --git a/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_http.log b/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_http.log index d38bcce2cff..269e5d2a883 100644 --- a/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_http.log +++ b/pkg/test/resourcefixture/testdata/basic/cloudbuild/v1alpha1/cloudbuildworkerpool/_http.log @@ -734,6 +734,40 @@ X-Xss-Protection: 0 --- +GET https://cloudbuild.googleapis.com/v1/projects/${projectId}/locations/us-central1/workerPools/cloudbuildworkerpool-${uniqueId}?%24alt=json%3Benum-encoding%3Dint +Content-Type: application/json +x-goog-request-params: location=us-central1 + +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 + +{ + "displayName": "New CloudBuild WorkerPool", + "name": "projects/${projectId}/locations/us-central1/workerPools/cloudbuildworkerpool-${uniqueId}", + "privatePoolV1Config": { + "networkConfig": { + "egressOption": "NO_PUBLIC_EGRESS", + "peeredNetwork": "projects/${projectId}/global/networks/computenetwork-${uniqueId}", + "peeredNetworkIpRange": "/29" + }, + "workerConfig": { + "diskSizeGb": "100", + "machineType": "e2-medium" + } + }, + "updateTime": "2024-04-01T12:34:56.123456Z" +} + +--- + DELETE https://cloudbuild.googleapis.com/v1/projects/${projectId}/locations/us-central1/workerPools/cloudbuildworkerpool-${uniqueId}?%24alt=json%3Benum-encoding%3Dint&allowMissing=true Content-Type: application/json x-goog-request-params: location=us-central1