Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: SecretManagerSecret full test coverage & manual replication field #3371

Merged
merged 7 commits into from
Dec 16, 2024
Merged

feat: SecretManagerSecret full test coverage & manual replication field #3371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94a7548
feat: add SecretManager manual replication feature
yuwenma Dec 12, 2024
afde6b9
real gcp log
yuwenma Dec 12, 2024
32a002c
mock gcp log
yuwenma Dec 12, 2024
f2edfe4
add alpha-direct annotation to secretversion test suite
yuwenma Dec 12, 2024
52f0b95
record vcr
yuwenma Dec 13, 2024
8825382
option 1 with real GCP
yuwenma Dec 16, 2024
792894d
mock gcp
yuwenma Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions apis/secretmanager/v1beta1/secret_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ type SecretManagerSecretSpec struct {
// This is always provided on output, regardless of what was sent on input.
ExpireTime *string `json:"expireTime,omitempty"`

// Input only. The TTL for the
// [Secret][google.cloud.secretmanager.v1.Secret].
// Input only. A duration in seconds with up to nine fractional digits, ending with 's'. Example: "3.5s".
Copy link
Collaborator Author

@yuwenma yuwenma Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TTL is a stale field that causes customers issues: it uses "Now()" when the resource is created that users cannot control or predict.
But since this is already in Beta, we still add the test coverage for it.

Also this change improves the user experience a little bit otherwise they don't now what value to give.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch. This doesn't feel like a spec field, because if I specify ttl: 60s what is the TTL value in 10 seconds? It seems like it should be 60s, because that is my desired state. But I suspect it will be 50s.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is exactly the problem!

TTL *string `json:"ttl,omitempty"`

// Optional. Rotation policy attached to the
Expand Down
3 changes: 2 additions & 1 deletion ...v1_customresourcedefinition_secretmanagersecrets.secretmanager.cnrm.cloud.google.com.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,8 @@ spec:
type: object
type: array
ttl:
description: Input only. The TTL for the [Secret][google.cloud.secretmanager.v1.Secret].
description: 'Input only. A duration in seconds with up to nine fractional
digits, ending with ''s''. Example: "3.5s".'
type: string
versionAliases:
additionalProperties:
Expand Down
2 changes: 1 addition & 1 deletion dev/tasks/run-e2e
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ if [[ -z "${KUBEBUILDER_ASSETS:-}" ]]; then
fi

if [[ -z "${KCC_USE_DIRECT_RECONCILERS:-}" ]]; then
KCC_USE_DIRECT_RECONCILERS=ComputeForwardingRule,GKEHubFeatureMembership,SecretManagerSecret,SecretManagerSecretVersion
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test now covers both legacy controller and direct controller (via alpha direct annotation). we no longer need this

KCC_USE_DIRECT_RECONCILERS=ComputeForwardingRule,GKEHubFeatureMembership
fi
echo "Using direct controllers: $KCC_USE_DIRECT_RECONCILERS"
export KCC_USE_DIRECT_RECONCILERS
Expand Down
11 changes: 11 additions & 0 deletions mockgcp/mocksecretmanager/secrets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ func (s *SecretsV1) populateDefaultsForSecret(ctx context.Context, obj *pb.Secre
return fmt.Errorf("Aliases cannot be assigned to versions that don't exist")
}
}
// TTL and ExpireTime are OneOf, but the GCP service always converts TTL to expireTime before storing the object.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable behaviour. Maybe we should deprecate spec.ttl (and replace with spec.initialTTL - although if we are going to do that, we can just change the comment on the field)

if obj.GetTtl() != nil {
expirateTime := timestamppb.Now().AsTime().Add(obj.GetTtl().AsDuration())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: might as well name it the same as the field you're setting, i.e. expireTime

obj.Expiration = &pb.Secret_ExpireTime{
ExpireTime: timestamppb.New(expirateTime),
}
}
return nil
}

Expand Down Expand Up @@ -154,6 +161,10 @@ func (s *SecretsV1) UpdateSecret(ctx context.Context, req *pb.UpdateSecretReques
updated.Expiration = &pb.Secret_ExpireTime{
ExpireTime: req.Secret.GetExpireTime(),
}
case "ttl":
updated.Expiration = &pb.Secret_Ttl{
Ttl: req.Secret.GetTtl(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: really? I would have assumed it would set Expiration.ExpireTime, just like in create. Still if this is what the logs tell us....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't know if we have test coverage of this in this PR (?). It's not a big deal, because it's a mock (we can fix it when we need it), just that I would have guessed the other way!

In general I think the behaviour of this ttl field is very tricky. Should we reconcile it (meaning secrets never expire)? Should we only apply on create? But if we do that, what happens when the user changes the TTL field in KRM? What happens if they change a different field?

Copy link
Collaborator Author

@yuwenma yuwenma Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... here's what the SecertManager server tells us https://screenshot.googleplex.com/9giJXnHPt3nMSwm.png For the log, the respond is always expire time, no ttl whatever the request says. I uploaded the real log in the git-commit

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. We set ttl, and then in populateDefaultsForSecret we convert it. I guess that works, because then we can use the shared fieldmask update code at some point in the future!

}
case "expiration":
updated.Expiration = req.Secret.GetExpiration()
case "rotation.nextRotationTime":
Expand Down
17 changes: 16 additions & 1 deletion pkg/controller/direct/secretmanager/secret_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,18 @@ func normalizeExternal(ctx context.Context, reader client.Reader, src client.Obj
secret.Spec.Replication.LegacyAutomatic.CustomerManagedEncryption.KmsKeyRef = kmsKeyRef
}
}
if secret.Spec.Replication.UserManaged != nil {
for _, r := range secret.Spec.Replication.UserManaged.Replicas {
if r.CustomerManagedEncryption != nil {
kmsKeyRef := r.CustomerManagedEncryption.KmsKeyRef
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I previously missed this field because I thought it is another field spec.customerManagedEncryption, which is not supported yet in the current released beta API.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a fuzz test :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR on the way.

kmsKeyRef, err := refs.ResolveKMSCryptoKeyRef(ctx, reader, src, kmsKeyRef)
if err != nil {
return err
}
r.CustomerManagedEncryption.KmsKeyRef = kmsKeyRef
}
}
}
}
if len(secret.Spec.TopicRefs) != 0 {
for _, topicRef := range secret.Spec.TopicRefs {
Expand Down Expand Up @@ -246,7 +258,10 @@ func (a *Adapter) Update(ctx context.Context, op *directbase.UpdateOperation) er
if err != nil {
return err
}

if paths.Has("ttl") {
paths = paths.Delete("ttl")
resource.Expiration = a.actual.Expiration
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there should be a comment here, because I don't think this does anything unless expiration is set in paths

}
if len(paths) == 0 {
log.V(2).Info("no field needs update", "name", a.id)
return nil
Expand Down
3 changes: 3 additions & 0 deletions pkg/controller/direct/secretmanager/secret_mapping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ func SecretManagerSecretSpec_ToProto(mapCtx *direct.MapContext, in *krm.SecretMa
if oneof := direct.StringTimestamp_ToProto(mapCtx, in.ExpireTime); oneof != nil {
out.Expiration = &pb.Secret_ExpireTime{ExpireTime: oneof}
}
if oneof := direct.Duration_ToProto(mapCtx, in.TTL); oneof != nil {
out.Expiration = &pb.Secret_Ttl{Ttl: oneof}
}
// MISSING: Etag
out.Rotation = Rotation_ToProto(mapCtx, in.Rotation)
out.VersionAliases = MapStringString_ToMapStringInt64(mapCtx, in.VersionAliases)
Expand Down
1 change: 1 addition & 0 deletions ...r/v1beta1/basicsecretmanagersecret/_generated_object_basicsecretmanagersecret.golden.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ apiVersion: secretmanager.cnrm.cloud.google.com/v1beta1
kind: SecretManagerSecret
metadata:
annotations:
alpha.cnrm.cloud.google.com/reconciler: direct
cnrm.cloud.google.com/management-conflict-prevention-policy: none
cnrm.cloud.google.com/project-id: ${projectId}
finalizers:
Expand Down
1 change: 1 addition & 0 deletions ...resourcefixture/testdata/basic/secretmanager/v1beta1/basicsecretmanagersecret/create.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ kind: SecretManagerSecret
metadata:
annotations:
cnrm.cloud.google.com/project-id: ${projectId}
alpha.cnrm.cloud.google.com/reconciler: "direct"
labels:
label-one: value-one
name: secretmanagersecret-${uniqueId}
Expand Down
0 ...ted_export_fullsecretmanagersecret.golden → ...ullsecretmanagersecret-auto-direct.golden
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions ...bject_fullsecretmanagersecret.golden.yaml → ...cretmanagersecret-auto-direct.golden.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ apiVersion: secretmanager.cnrm.cloud.google.com/v1beta1
kind: SecretManagerSecret
metadata:
annotations:
alpha.cnrm.cloud.google.com/reconciler: direct
cnrm.cloud.google.com/management-conflict-prevention-policy: none
cnrm.cloud.google.com/project-id: ${projectId}
finalizers:
Expand Down
14 changes: 7 additions & 7 deletions ...v1beta1/fullsecretmanagersecret/_http.log → ...secretmanagersecret-auto-direct/_http.log
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ x-goog-request-params: parent=projects%2F${projectId}
"annotations": {
"foo": "secretmanagersecret"
},
"expireTime": "2025-10-02T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -733,7 +733,7 @@ X-Xss-Protection: 0
},
"createTime": "2024-04-01T12:34:56.123456Z",
"etag": "abcdef0123A=",
"expireTime": "2025-10-02T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -782,7 +782,7 @@ X-Xss-Protection: 0
},
"createTime": "2024-04-01T12:34:56.123456Z",
"etag": "abcdef0123A=",
"expireTime": "2025-10-02T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -820,7 +820,7 @@ x-goog-request-params: secret.name=projects%2F${projectId}%2Fsecrets%2Fsecretman
"foo": "secretmanagersecret"
},
"etag": "abcdef0123A=",
"expireTime": "2025-10-03T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -864,7 +864,7 @@ X-Xss-Protection: 0
},
"createTime": "2024-04-01T12:34:56.123456Z",
"etag": "abcdef0123A=",
"expireTime": "2025-10-03T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -914,7 +914,7 @@ X-Xss-Protection: 0
},
"createTime": "2024-04-01T12:34:56.123456Z",
"etag": "abcdef0123A=",
"expireTime": "2025-10-03T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down Expand Up @@ -965,7 +965,7 @@ X-Xss-Protection: 0
},
"createTime": "2024-04-01T12:34:56.123456Z",
"etag": "abcdef0123A=",
"expireTime": "2025-10-03T15:01:23Z",
"expireTime": "2024-04-01T12:34:56.123456Z",
"labels": {
"cnrm-test": "true",
"label-one": "value-one",
Expand Down
1 change: 1 addition & 0 deletions ...beta1/fullsecretmanagersecret/create.yaml → ...cretmanagersecret-auto-direct/create.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ kind: SecretManagerSecret
metadata:
annotations:
cnrm.cloud.google.com/project-id: ${projectId}
alpha.cnrm.cloud.google.com/reconciler: "direct"
labels:
label-one: value-one
name: secretmanagersecret-${uniqueId}
Expand Down
0 ...fullsecretmanagersecret/dependencies.yaml → ...nagersecret-auto-direct/dependencies.yaml
View file
Open in desktop
File renamed without changes.
1 change: 1 addition & 0 deletions ...beta1/fullsecretmanagersecret/update.yaml → ...cretmanagersecret-auto-direct/update.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ kind: SecretManagerSecret
metadata:
annotations:
cnrm.cloud.google.com/project-id: ${projectId}
alpha.cnrm.cloud.google.com/reconciler: "direct"
labels:
label-one: value-one
label-two: value-two
Expand Down
23 changes: 23 additions & 0 deletions ...retmanagersecret-auto-legacy/_generated_export_fullsecretmanagersecret-auto-legacy.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
apiVersion: secretmanager.cnrm.cloud.google.com/v1beta1
kind: SecretManagerSecret
metadata:
annotations:
cnrm.cloud.google.com/project-id: ${projectId}
labels:
cnrm-test: "true"
label-one: value-one
label-two: value-two
managed-by-cnrm: "true"
name: secretmanagersecret-${uniqueId}
spec:
annotations:
bar: secretmanagersecret-bar
foo: secretmanagersecret
expireTime: "2025-10-03T15:01:23Z"
resourceID: secretmanagersecret-${uniqueId}
rotation:
nextRotationTime: "2025-10-03T15:01:23Z"
rotationPeriod: 3600s
topics:
- topicRef:
external: projects/${projectId}/topics/topic-2-${uniqueId}
40 changes: 40 additions & 0 deletions ...nagersecret-auto-legacy/_generated_object_fullsecretmanagersecret-auto-legacy.golden.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
apiVersion: secretmanager.cnrm.cloud.google.com/v1beta1
kind: SecretManagerSecret
metadata:
annotations:
cnrm.cloud.google.com/management-conflict-prevention-policy: none
cnrm.cloud.google.com/project-id: ${projectId}
cnrm.cloud.google.com/state-into-spec: absent
finalizers:
- cnrm.cloud.google.com/finalizer
- cnrm.cloud.google.com/deletion-defender
generation: 3
labels:
cnrm-test: "true"
label-one: value-one
label-two: value-two
name: secretmanagersecret-${uniqueId}
namespace: ${uniqueId}
spec:
annotations:
bar: secretmanagersecret-bar
foo: secretmanagersecret
expireTime: "2025-10-03T15:01:23Z"
replication:
automatic: true
resourceID: secretmanagersecret-${uniqueId}
rotation:
nextRotationTime: "2025-10-03T15:01:23Z"
rotationPeriod: 3600s
topics:
- topicRef:
name: topic-2-${uniqueId}
status:
conditions:
- lastTransitionTime: "1970-01-01T00:00:00Z"
message: The resource is up to date
reason: UpToDate
status: "True"
type: Ready
name: projects/${projectNumber}/secrets/secretmanagersecret-${uniqueId}
observedGeneration: 3
Loading
Loading