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.

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: Add tag-on-create #516

Merged
merged 1 commit into from
Jun 11, 2024
Merged

feat: Add tag-on-create #516

merged 1 commit into from
Jun 11, 2024

Conversation

bhavanki
Copy link
Contributor

Store implementations may support tagging secrets immediately after they
are created, as a convenience and a basis for future enforcement of
tagging. As usual, only the SSM store implementation supports the new
feature.

The README is updated with documentation about both this new feature and
the recently added tag commands.

Store implementations may support tagging secrets immediately after they
are created, as a convenience and a basis for future enforcement of
tagging. As usual, only the SSM store implementation supports the new
feature.

The README is updated with documentation about both this new feature and
the recently added tag commands.
@bhavanki bhavanki requested a review from a team as a code owner June 10, 2024 15:16
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 35.99%. Comparing base (37a70e0) to head (4f3dcb8).

Files Patch % Lines
cmd/write.go 20.00% 4 Missing ⚠️
store/nullstore.go 0.00% 2 Missing ⚠️
store/s3store.go 0.00% 2 Missing ⚠️
store/s3storeKMS.go 0.00% 2 Missing ⚠️
store/secretsmanagerstore.go 0.00% 2 Missing ⚠️
store/ssmstore.go 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   35.97%   35.99%   +0.01%     
==========================================
  Files          29       29              
  Lines        2524     2545      +21     
==========================================
+ Hits          908      916       +8     
- Misses       1538     1550      +12     
- Partials       78       79       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Sabrina0614 Sabrina0614 left a comment

Choose a reason for hiding this comment

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

Looks great! Just some questions. Thank you for adding it and also update the README

@@ -99,6 +107,10 @@ func (s *SSMStore) Write(ctx context.Context, id SecretId, value string) error {
version = current.Meta.Version + 1
}

if len(tags) > 0 && version != 1 {
return errors.New("tags on write only supported for new secrets")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Curious if there is a particular reason why tagging is only support for new secrets? Sorry if I am missing it somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You didn't miss anything! (I thought for a second that I had documented why, but it's just in my notes.) I was thinking of what chamber should do when the secret already exists and it already has tags.

  • If you include --tags, what should happen to the other tags that you don't list? Should they be deleted or left alone? Admittedly, we could add the --delete-other-tags option from chamber tag write to let you pick.
  • If you do not include --tags, what should happen to the existing tags? Should they be all dropped out or left alone? We could maybe add another option, --delete-existing-tags or something, to let you pick.

It's because of this complexity that, at least for this pass, I opted to restrict tags to just new secrets.

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the context! I did not know that it would override the existing tag prior.

@@ -114,6 +126,12 @@ func (s *SSMStore) Write(ctx context.Context, id SecretId, value string) error {
return err
}

if len(tags) > 0 {
if err := s.WriteTags(ctx, id, tags, false); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ (More of the WriteTags method) - It looks like the WriteTags also take in deleteOtherTags, curious what would happen for the following case:

  1. If there already exist a tag for a secret, let's say it has tagapp = backstage and another tag requiredChamber = true
  2. I then now call write tag for this secret with the app = backstage and with deleteOtherTags is set to true, it will remove the required = chamber, but would it error out if we are trying to add the tag that already exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand your scenario correctly, it would be fine, because you can use chamber tag write to replace the value of an existing tag, and it's fine if the new value is the same as the old value.

In the implementation of WriteTags, first all of the tags which are not being written are deleted; because the app tag is being written, it isn't deleted. Then, the app tag is updated using the SSM add-tags-to-resource API, which either adds or overwrites tags.

Let me know if I misunderstood your scenario!

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense! thank you for walking me through it!

@bhavanki bhavanki merged commit 0e67861 into master Jun 11, 2024
17 checks passed
@bhavanki bhavanki deleted the tag-on-create branch June 11, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants