feat: add secretManager version alias #3282
Conversation
yuwenma
changed the title
yuwenma
force-pushed
the
sm-version-alias
branch
2 times, most recently
from
bec47e4 to
e21f5bc
Compare
yuwenma
force-pushed
the
sm-version-alias
branch
from
e21f5bc to
6fafcf3
Compare
yuwenma
force-pushed
the
sm-version-alias
branch
from
6fafcf3 to
f3dfb2f
Compare
| return nil | ||
| } | ||
| out := &krm.SecretManagerSecretObservedState{} | ||
| out.VersionAliases = MapStringInt64_ToMapStringString(mapCtx, in.VersionAliases) |
There was a problem hiding this comment.
Nit: might be worth adding this to the autogenerator. Though there is something to be said for "flagging" this by refusing to generate it!
There was a problem hiding this comment.
I was thinking of the same thing. But this use here is not typical enough and IMO more like a unclear SecretManager API decision. I'd prefer adding it later if we encounter another case.
| @@ -96,8 +103,7 @@ func SecretManagerSecretSpec_ToProto(mapCtx *direct.MapContext, in *krm.SecretMa | |||
| } | |||
| // MISSING: Etag | |||
| out.Rotation = Rotation_ToProto(mapCtx, in.Rotation) | |||
| // MISSING: VersionAliases | |||
| // out.VersionAliases = in.VersionAliases | |||
| out.VersionAliases = MapStringString_ToMapStringInt64(mapCtx, in.VersionAliases) | |||
There was a problem hiding this comment.
Oh ... we should test with really big values (bigger than max json values)
We have had bugs in the past where int64 values went through json numbers and were rounder. IIRC it is numbers bigger than 10^53. It doesn't always happen, but some methods do "reparse" the values and truncate them.
This is status, and it doesn't really matter, except that we might have to make the KRM type map[string]string (which would be a shame, because map[string]int64 can work)
There was a problem hiding this comment.
Very interesting case! I can do a follow up PR to improve this. This is less likely to happen for SecretManager because the int64 is used as a version count starting from 1.
| @@ -79,6 +79,43 @@ X-Xss-Protection: 0 | |||
|
|
|||
| --- | |||
|
|
|||
| GET https://secretmanager.googleapis.com/v1/projects/${projectId}/secrets/secretmanagersecret-${uniqueId}?alt=json | |||
There was a problem hiding this comment.
The extra get is because we're now exporting? If so 👍
| observedState: {} | ||
| observedState: | ||
| versionAliases: | ||
| string: string |
There was a problem hiding this comment.
status.versionAliases is a new field I added in this PR. otherwise, it is very unclear whether the versionAliases succeeds or not.
The spec.versionAliases are already there.
There was a problem hiding this comment.
Yeah, I was confused by github's presentation of the diff. map[string]string makes sense. It would be nice if it was map[string]int64 in KRM and proto, but it may encounter some bugs with big values.
| return "" | ||
| switch gvk.Kind { | ||
| case "SecretManagerSecretVersion": | ||
| // Skip run export.Execute because the SecretVersion servicemappings has a broken marker |
There was a problem hiding this comment.
Can we only export if using direct? Or just move to direct export everywhere?
There was a problem hiding this comment.
I guess in practice it's not very useful to export the SecretManagerSecretVersion, and potentially risky (exposing secret material)
There was a problem hiding this comment.
Agree. And agree. I hit an error around the scenario, and explored on what happened in the code. Lots of improvements are needed for Export.
yuwenma
force-pushed
the
sm-version-alias
branch
from
262636c to
d75b3db
Compare
| @@ -203,7 +204,14 @@ func (a *Adapter) Create(ctx context.Context, op *directbase.CreateOperation) er | |||
| external := a.id.String() | |||
| status.ExternalRef = &external | |||
| status.Name = created.Name | |||
| return op.UpdateStatus(ctx, status, nil) | |||
| if err = op.UpdateStatus(ctx, status, nil); err != nil { | |||
There was a problem hiding this comment.
| if err = op.UpdateStatus(ctx, status, nil); err != nil { | |
| if err := op.UpdateStatus(ctx, status, nil); err != nil { |
There was a problem hiding this comment.
Let me fix that in follow up PRs. have some other comments and nits want to resolve as well.
| observedState: {} | ||
| observedState: | ||
| versionAliases: | ||
| string: string |
There was a problem hiding this comment.
Yeah, I was confused by github's presentation of the diff. map[string]string makes sense. It would be nice if it was map[string]int64 in KRM and proto, but it may encounter some bugs with big values.
|
LGTM, will hold so you can decide when you want to merge /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 |
|
/hold cancel |
google-oss-prow
bot
merged commit 2c92427
into
GoogleCloudPlatform:master
Change description
Fixes #
Tests you have done
make ready-prto ensure this PR is ready for review.