chore: improve secretmanager on edge case handling #3274

yuwenma · 2024-12-02T20:16:10Z

Change description

Fixes #

Tests you have done

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

yuwenma · 2024-12-03T03:19:38Z

...ager/v1beta1/basicsecretmanagersecret/_generated_object_basicsecretmanagersecret.golden.yaml

@@ -19,14 +20,13 @@ spec:
    userManaged:
      replicas:
      - location: us-central1
+  resourceID: secretmanagersecret-${uniqueId}


TF always add the spec.resourceID even if state-into-spec: absent

yuwenma · 2024-12-03T03:21:29Z

...test/resourcefixture/testdata/basic/secretmanager/v1beta1/basicsecretmanagersecret/_http.log

-    "label-one": "value-one",
+    "cnrm-test": "true",


(regression?) TF-based uses metadata.labels + managed-by-cnrm - system labels , the direct uses spec.labels + managed-by-cnrm.

I think that's a behavioural change. It agree with the direction, but I think it needs a design as it's a big topic.

yuwenma · 2024-12-03T03:24:03Z

.../v1beta1/secretmanagersecretversion/_generated_object_secretmanagersecretversion.golden.yaml

@@ -3,17 +3,20 @@ kind: SecretManagerSecretVersion
 metadata:
  annotations:
    cnrm.cloud.google.com/management-conflict-prevention-policy: none
+    cnrm.cloud.google.com/observed-secret-versions: (removed)


This is a legacy annotation. The TF-based approach builds a mechanism around this annotation to determine if a sensitive data is changed.

yuwenma · 2024-12-03T03:24:44Z

.../v1beta1/secretmanagersecretversion/_generated_object_secretmanagersecretversion.golden.yaml

  labels:
    cnrm-test: "true"
  name: secretmanagersecretversion-${uniqueId}
  namespace: ${uniqueId}
 spec:
  enabled: false
+  resourceID: "1"


secret version is a service-genereated-ID resource, that TF_based always write back to spec.resourceID. In the direct approach, we use status.externalRef instead.

I think that's technically a regression, but I think it's a minor one. So I think if we add this to the release notes (which you can do in this PR if we have a release note file on the go) then I think that's reasonable.

yuwenma · 2024-12-03T03:26:46Z

...ager/v1beta1/basicsecretmanagersecret/_generated_object_basicsecretmanagersecret.golden.yaml

Besides the basic test suite, I also added full test suite, where the TF-based controller cannot reconcile correctly because it assume a mutable field is immutable.
Here's the error, and here's the tracking issue with more details.

justinsb · 2024-12-02T21:41:33Z

apis/secretmanager/v1beta1/secret_reference.go

+		actualExternalRef, _, err2 = unstructured.NestedString(u.Object, "status", "name")
+		if err2 != nil {
+			err2 = fmt.Errorf("SecretManagerSecret `status.name` not configured: %w", err2)
+			return "", fmt.Errorf("%w\n%w", err1, err2)


Nit: I think errors.Join is good here

Good call! Thanks!

justinsb · 2024-12-02T21:42:28Z

apis/secretmanager/v1beta1/secret_types.go

+	//
+	//  Label keys must be between 1 and 63 characters long, have a UTF-8 encoding
+	//  of maximum 128 bytes, and must conform to the following PCRE regular
+	//  expression: `[\p{Ll}\p{Lo}][\p{Ll}\p{Lo}\p{N}_-]{0,62}`


Is this regex from our proto? Can we express it in human?

justinsb · 2024-12-03T12:51:19Z

...test/resourcefixture/testdata/basic/secretmanager/v1beta1/basicsecretmanagersecret/_http.log

-    "label-one": "value-one",
+    "cnrm-test": "true",


I think that's a behavioural change. It agree with the direction, but I think it needs a design as it's a big topic.

justinsb · 2024-12-03T12:52:16Z

.../v1beta1/secretmanagersecretversion/_generated_object_secretmanagersecretversion.golden.yaml

  labels:
    cnrm-test: "true"
  name: secretmanagersecretversion-${uniqueId}
  namespace: ${uniqueId}
 spec:
  enabled: false
+  resourceID: "1"


I think that's technically a regression, but I think it's a minor one. So I think if we add this to the release notes (which you can do in this PR if we have a release note file on the go) then I think that's reasonable.

justinsb · 2024-12-03T12:53:57Z

...io_v1_customresourcedefinition_secretmanagersecrets.secretmanager.cnrm.cloud.google.com.yaml

@@ -84,6 +84,22 @@ spec:
                  is scheduled to expire. This is always provided on output, regardless
                  of what was sent on input.
                type: string
+              labels:


I think this is what needs a separate design, I'm afraid.

justinsb · 2024-12-03T12:55:45Z

pkg/controller/direct/secretmanager/secret_controller.go

 	resource := SecretManagerSecretSpec_ToProto(mapCtx, &desired.Spec)
 	if mapCtx.Err() != nil {
 		return mapCtx.Err()
 	}
-
-	resource.Annotations = ComputeAnnotations(desired)


Are we also dropping support for annotations? I think that should be mentioned in the labels design, but I don't think it is as tricky because it's not as widely used.

I'm not even sure if terraform maps metadata.annotations -> GCP annotations - does it?

justinsb · 2024-12-03T12:56:31Z

pkg/controller/direct/secretmanager/secret_controller.go

@@ -262,6 +239,10 @@ func (a *Adapter) Update(ctx context.Context, op *directbase.UpdateOperation) er
 	if mapCtx.Err() != nil {
 		return mapCtx.Err()
 	}
+	// Set externalRef. This field is empty after migration from the TF-based controller to the direct.


Can we still go back to the terraform controller after creating resources with the direction controller? i.e. does it matter if spec.resourceID is not set? (Is it always == metadata.name?)

Secret is not service generated ID resource (SecretVersion does). So it can switch back to use the TF-based controller. The real problem on using the TF-based controller is that the Secret spec.rotation.rotationPeriod is broken. If user configures that fields in Direct and switched back to TF-based, it no longer works.

justinsb · 2024-12-03T13:01:44Z

pkg/controller/direct/secretmanager/secretversion_controller.go

@@ -164,6 +164,19 @@ func (a *SecretVersionAdapter) Create(ctx context.Context, createOp *directbase.
 	}
 	log.V(2).Info("successfully created SecretVersion", "name", a.id)

+	// The default status for newly created resource is "enabled".


This is a bummer, but I verified that this is indeed the behaviour!

google-oss-prow · 2024-12-03T23:47:42Z

[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

yuwenma force-pushed the secret-version branch from 5af3051 to 374ea58 Compare December 3, 2024 02:53

yuwenma commented Dec 3, 2024

View reviewed changes

yuwenma force-pushed the secret-version branch from 91a7af6 to c871e6a Compare December 3, 2024 07:05

justinsb reviewed Dec 3, 2024

View reviewed changes

yuwenma added 5 commits December 3, 2024 20:35

chore: improve secretmanager on edge case handling

b87e607

improve annotations

31e6b50

tf-based result

310e635

direct real gcp

1305055

revert label and annotation changes

bbb6602

yuwenma force-pushed the secret-version branch from ab3567b to bbb6602 Compare December 3, 2024 20:36

justinsb approved these changes Dec 3, 2024

View reviewed changes

google-oss-prow bot assigned justinsb Dec 3, 2024

google-oss-prow bot added the lgtm label Dec 3, 2024

google-oss-prow bot added the approved label Dec 3, 2024

google-oss-prow bot merged commit caaedb4 into GoogleCloudPlatform:master Dec 3, 2024
18 checks passed

yuwenma added this to the 1.126 milestone Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve secretmanager on edge case handling #3274

chore: improve secretmanager on edge case handling #3274

yuwenma commented Dec 2, 2024

yuwenma Dec 3, 2024

yuwenma Dec 3, 2024

justinsb Dec 3, 2024

yuwenma Dec 3, 2024

yuwenma Dec 3, 2024 •

edited

Loading

justinsb Dec 3, 2024

yuwenma Dec 3, 2024

justinsb Dec 2, 2024

yuwenma Dec 3, 2024

justinsb Dec 2, 2024

justinsb Dec 3, 2024

justinsb Dec 3, 2024

justinsb Dec 3, 2024

justinsb Dec 3, 2024

justinsb Dec 3, 2024

yuwenma Dec 3, 2024

justinsb Dec 3, 2024

google-oss-prow bot commented Dec 3, 2024

chore: improve secretmanager on edge case handling #3274

chore: improve secretmanager on edge case handling #3274

Conversation

yuwenma commented Dec 2, 2024

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

Choose a reason for hiding this comment

yuwenma Dec 3, 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

google-oss-prow bot commented Dec 3, 2024

yuwenma Dec 3, 2024 •

edited

Loading