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
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 36 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,42 @@ Secret keys are normalized automatically. The `-` will be `_` and the letters wi
be converted to upper case (for example a secret with key `secret_key` and
`secret-key` will become `SECRET_KEY`).

#### Tagging on Write

```bash
$ chamber write <service> <key> <value> --tags key1=value1,key2=value2
```

This operation will write a secret into the secret store with the specified tags.
Tagging on write is only available for new secrets.

### Tagging Secrets

```bash
$ chamber tag write <service> <key> tag1=value1 tag2=value2
Key Value
tag1 value1
tag2 value2
$ chamber tag read <service> <key>
Key Value
tag1 value1
tag2 value2
$ chamber tag delete <service> <key> tag1
$ chamber tag read <service> <key>
Key Value
tag2 value2
```

Writing tags normally leaves other tags intact. If you want to replace all tags
with the new ones, use `--delete-other-tags` flag. _Note: The option may change
before the next major release._

```bash
$ chamber tag write --delete-other-tags <service> <key> tag1=value1
Key Value
tag1 value1
```

### Listing Secrets

```bash
Expand Down
8 changes: 7 additions & 1 deletion cmd/write.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
var (
singleline bool
skipUnchanged bool
tags map[string]string

// writeCmd represents the write command
writeCmd = &cobra.Command{
Expand All @@ -29,6 +30,7 @@
func init() {
writeCmd.Flags().BoolVarP(&singleline, "singleline", "s", false, "Insert single line parameter (end with \\n)")
writeCmd.Flags().BoolVarP(&skipUnchanged, "skip-unchanged", "", false, "Skip writing secret if value is unchanged")
writeCmd.Flags().StringToStringVarP(&tags, "tags", "t", map[string]string{}, "Add tags to the secret; new secrets only")
RootCmd.AddCommand(writeCmd)
}

Expand Down Expand Up @@ -92,5 +94,9 @@
}
}

return secretStore.Write(cmd.Context(), secretId, value)
if len(tags) > 0 {
return secretStore.WriteWithTags(cmd.Context(), secretId, value, tags)
} else {
return secretStore.Write(cmd.Context(), secretId, value)

Check warning on line 100 in cmd/write.go

View check run for this annotation

Codecov / codecov/patch

cmd/write.go#L97-L100

Added lines #L97 - L100 were not covered by tests
}
}
4 changes: 4 additions & 0 deletions store/nullstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
return errors.New("Write is not implemented for Null Store")
}

func (s *NullStore) WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error {
return errors.New("WriteWithTags is not implemented for Null Store")

Check warning on line 21 in store/nullstore.go

View check run for this annotation

Codecov / codecov/patch

store/nullstore.go#L20-L21

Added lines #L20 - L21 were not covered by tests
}

func (s *NullStore) Read(ctx context.Context, id SecretId, version int) (Secret, error) {
return Secret{}, errors.New("Not implemented for Null Store")
}
Expand Down
4 changes: 4 additions & 0 deletions store/s3store.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@
return s.writeLatest(ctx, id.Service, index)
}

func (s *S3Store) WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error {
return errors.New("Not implemented for S3 Store")

Check warning on line 135 in store/s3store.go

View check run for this annotation

Codecov / codecov/patch

store/s3store.go#L134-L135

Added lines #L134 - L135 were not covered by tests
}

func (s *S3Store) Read(ctx context.Context, id SecretId, version int) (Secret, error) {
obj, ok, err := s.readObjectById(ctx, id)
if err != nil {
Expand Down
4 changes: 4 additions & 0 deletions store/s3storeKMS.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@
return s.writeLatest(ctx, id.Service, index)
}

func (s *S3KMSStore) WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error {
return errors.New("Not implemented for S3 KMS Store")

Check warning on line 142 in store/s3storeKMS.go

View check run for this annotation

Codecov / codecov/patch

store/s3storeKMS.go#L141-L142

Added lines #L141 - L142 were not covered by tests
}

func (s *S3KMSStore) ListServices(ctx context.Context, service string, includeSecretName bool) ([]string, error) {
return nil, fmt.Errorf("S3KMS Backend is experimental and does not implement this command")
}
Expand Down
4 changes: 4 additions & 0 deletions store/secretsmanagerstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,10 @@
return nil
}

func (s *SecretsManagerStore) WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error {
return errors.New("tags on write not implemented for Secrets Manager Store")

Check warning on line 219 in store/secretsmanagerstore.go

View check run for this annotation

Codecov / codecov/patch

store/secretsmanagerstore.go#L218-L219

Added lines #L218 - L219 were not covered by tests
}

// Read reads a secret at a specific version.
// To grab the latest version, use -1 as the version number.
func (s *SecretsManagerStore) Read(ctx context.Context, id SecretId, version int) (Secret, error) {
Expand Down
18 changes: 18 additions & 0 deletions store/ssmstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@
// Write writes a given value to a secret identified by id. If the secret
// already exists, then write a new version.
func (s *SSMStore) Write(ctx context.Context, id SecretId, value string) error {
return s.write(ctx, id, value, nil)
}

func (s *SSMStore) WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error {
return s.write(ctx, id, value, tags)
}

func (s *SSMStore) write(ctx context.Context, id SecretId, value string, tags map[string]string) error {
version := 1
// first read to get the current version
current, err := s.Read(ctx, id, -1)
Expand All @@ -99,6 +107,10 @@
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.


putParameterInput := &ssm.PutParameterInput{
KeyId: aws.String(s.KMSKey()),
Name: aws.String(s.idToName(id)),
Expand All @@ -114,6 +126,12 @@
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!

return fmt.Errorf("failed to write tags on successfully created secret: %w", err)

Check warning on line 131 in store/ssmstore.go

View check run for this annotation

Codecov / codecov/patch

store/ssmstore.go#L131

Added line #L131 was not covered by tests
}
}

return nil
}

Expand Down
33 changes: 33 additions & 0 deletions store/ssmstore_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,39 @@ func TestWritePaths(t *testing.T) {
})
}

func TestWriteWithTags(t *testing.T) {
ctx := context.Background()
parameters := map[string]mockParameter{}
store := NewTestSSMStore(parameters)

t.Run("Setting a new key with tags should work", func(t *testing.T) {
secretId := SecretId{Service: "test", Key: "mykey"}
tags := map[string]string{
"tag1": "value1",
"tag2": "value2",
}
err := store.WriteWithTags(ctx, secretId, "value", tags)
assert.Nil(t, err)
assert.Contains(t, parameters, store.idToName(secretId))

paramTags := parameters[store.idToName(secretId)].tags
assert.Equal(t, "value1", paramTags["tag1"])
assert.Equal(t, "value2", paramTags["tag2"])
})

t.Run("Setting a existing key with tags should fail", func(t *testing.T) {
secretId := SecretId{Service: "test", Key: "mykey"}
tags := map[string]string{
"tag3": "value3",
}
err := store.WriteWithTags(ctx, secretId, "newvalue", tags)
assert.Error(t, err)

assert.Contains(t, parameters, store.idToName(secretId))
assert.Equal(t, "value", *parameters[store.idToName(secretId)].currentParam.Value) // unchanged
})
}

func TestReadPaths(t *testing.T) {
ctx := context.Background()
parameters := map[string]mockParameter{}
Expand Down
1 change: 1 addition & 0 deletions store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type ChangeEvent struct {

type Store interface {
Write(ctx context.Context, id SecretId, value string) error
WriteWithTags(ctx context.Context, id SecretId, value string, tags map[string]string) error
Read(ctx context.Context, id SecretId, version int) (Secret, error)
WriteTags(ctx context.Context, id SecretId, tags map[string]string, deleteOtherTags bool) error
ReadTags(ctx context.Context, id SecretId) (map[string]string, error)
Expand Down
Loading