feat: Enforce required tags in SSM store

[bhavanki]

The behavior is as follows:

• When writing a new secret, all required tags must be provided.
• When writing tags to a secret, and other tags are being deleted, then all required tags that are already present on the secret must be provided.
• When deleting tags from a secret, required tags may not be provided.

The second item is a bit subtle. If other tags aren't being deleted, i.e., tags not provided are being left alone, then required tags don't have to be provided (they may be, but don't have to be). Also, if other tags are being deleted, then the only required tags that have to be provided are those that are already set. These behaviors are designed so that after a store's configuration is updated to require tags, you don't have to then immediately set them all on every existing secret. Once you do set them though, you can't get rid of them.

[codecov]

Codecov Report

Attention: Patch coverage is 85.45455% with 8 lines in your changes missing coverage. Please review.

Project coverage is 37.33%. Comparing base (c2d3399) to head (7b2348b).
Report is 5 commits behind head on master.

Files	Patch %	Lines
store/ssmstore.go	88.00%	3 Missing and 3 partials ⚠️
store/store.go	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
+ Coverage   36.34%   37.33%   +0.99%     
==========================================
  Files          29       29              
  Lines        2592     2646      +54     
==========================================
+ Hits          942      988      +46     
- Misses       1567     1571       +4     
- Partials       83       87       +4     

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

[Sabrina0614]

Some questions but looks good overall! Thanks for the PR, Bill

[Sabrina0614]

Would you mind elaborate a bit more on this for loop, especially on the if statement? I thought that if a required tag is already present, and not being updated, we should allow it given that the tags already exist.

[bhavanki]

You're right, unless the option is selected to "delete other tags". When that happens, then any required tags which are already present must be explicitly provided again ... or else they would be deleted! This function checkForPresentRequiredTags is only called when "delete other tags" is true, so that's why this condition is OK.

This explanation isn't obvious from the code, though, so I'll add some comments to clarify.

[Sabrina0614]

Thanks for clarifying it, Bill! I am wondering if we can not ask/require the user to provide the existing required tag when it already exists?
Or if this is only for terraform, i.e the tags will always exists when applying the changes?

[bhavanki]

I don't think I understand your question. Let me lay out the way it works to see if I can answer it. Suppose that tag A is required and is already set, and someone runs chamber tag write <svc> <key> B=valueb.

• If they do not include --delete-other-tags, then the write should succeed. Tag A stays with its current value, and tag B is set to valueb.
• If they do include --delete-other-tags, then the write should fail. Chamber will say that tag A is required, so you have to provide a value for it (rewrite it).
• If they do include --delete-other-tags and also provide some value for tag A as well (A=valueA B=valueB), then the write should succeed. Tags A and B will both be set. So I think that covers requiring the user to provide the existing required tag.

Yeah, I guess Terraform is a good use case for --delete-other-tags because then chamber ensures that only exactly the provided tags exist, and all others would be deleted.

[Sabrina0614]

❓ I see that we are creating tags as a map/set in this case. Is it because the tagKeys could be duplicated and thus, we are creating a map/set in this case? Curious if it is possible that it could be duplicated

[bhavanki]

The user running chamber tag delete could list a tag more than once. But, the reason for the set here is just to make it easier to check if a required tag was provided - instead of having to iterate through the tagKeys slice for every required tag, it's just a set lookup.

	var foundTags []string
	for _, requiredTag := range requiredTags {
		if _, ok := tags[requiredTag]; ok {
			foundTags = append(foundTags, requiredTag)
		}
	}

The code could instead make a set for requiredKeys and then iterate through the original tagKeys slice (is this one required? is this next one required?) ... although then it could report a required key more than once, if the user listed it more than once to delete.

Just to check, I ran chamber tag delete on a secret and listed a tag twice, and it was removed without error.