feat: add ExternalReference (selfLink) with GCP update/deletion uniqueness guardrail #2020
Conversation
| ) | ||
|
|
||
| func NewResourceRef(gcpObj *cloudbuildpb.WorkerPool) *ResourceRef { | ||
| return &ResourceRef{ |
There was a problem hiding this comment.
This CBWP ResourceRef customize the common ResourceRef, mostly to provide dedicated user information.
| return setStatus(u, status) | ||
| } | ||
|
|
||
| func (a *Adapter) Update(ctx context.Context, u *unstructured.Unstructured) error { | ||
| if err := a.ValidateExternalResource(); err != nil { |
There was a problem hiding this comment.
We do not allow user to update the spec.resourceID spec.projectRef or spec.location to a different value (which means a different GCP object).
Ideally we can move this to the directbase_controller. (or we already have this check somewhere?)
...s.io_v1_customresourcedefinition_cloudbuildworkerpools.cloudbuild.cnrm.cloud.google.com.yaml
Outdated
Show resolved
Hide resolved
|
|
||
| func NewResourceRef(gcpObj *cloudbuildpb.WorkerPool) *ResourceRef { | ||
| return &ResourceRef{ | ||
| externalresource.New("https://cloudbuild.googleapis.com/v1/", gcpObj), |
There was a problem hiding this comment.
Nit: I don't know if we want v1. Although I guess it depends on whether in OnePlatform a v1 and v2 with the same name would be the same resource.
There was a problem hiding this comment.
I see what you mean. I think we need to specify a version in the URL anyways to make it valid. Maybe I'm wrong, but does the v1/v2 bundles in the godoc.org/cloud.google.com/go?
| } | ||
|
|
||
| func (e *ResourceRef) GetResourceID() string { | ||
| segments := strings.Split(*e.Get(), "/workerPools/") |
There was a problem hiding this comment.
Two nits:
- Dereferencing pointers is always scary (because it panics on nil), better to use a helper like ValueOf
- I think you should also check that segments actually has len != 0.
It might be the fluent way to do this is to move the parsing to NewResourceRef (which can then return an error)
There was a problem hiding this comment.
Thanks! The code looks much nicer now.
| @@ -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() | |||
There was a problem hiding this comment.
You may want to write a quick design-doc for "status.externalRef" including "why is it not status.selfLink", how it fixes resource deletion etc
| @@ -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. | |||
There was a problem hiding this comment.
Oooh cool - another advantage!
| 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. |
There was a problem hiding this comment.
We also won't be able to check e.g. references like containerRef on a NodePool.
This method seems cool, but it might be overkill - at least for now?
| existsAlready, err := adapter.Find(ctx) | ||
| if err != nil { | ||
| if unwrappedErr, ok := lifecyclehandler.CausedByUnresolvableDeps(err); ok { | ||
| logger.Info(unwrappedErr.Error(), "resource", k8s.GetNamespacedName(u)) |
There was a problem hiding this comment.
One gotcha is that if we're deleting we likely don't need the spec refs, we just need status.externalRef.
I guess we can tackle this later (as part of cleaning up resource cleanup)?
There was a problem hiding this comment.
oh, right! Yes I like that.
I added a todo and would like to do that in a follow up PR, since that requires some refactoring of the fullyQualifiedName method.
| @@ -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} | |||
There was a problem hiding this comment.
Seeing this here, I like this!
|
You have some conflicts, but... /approve |
…eletion uniqueness guardrail
|
Thanks - looks great! /approve |
|
[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:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit aa7eb04
into
GoogleCloudPlatform:master
Follow up PR to resolve comment in #1920 (comment)