Implemented option for custom SM locations #165
Conversation
|
Hi @MartijnNPO - thank you for the PR. My concern with doing this is that it has no corresponding impact when the secrets storage is Cloud Storage instead of Secret Manager. I think that would be confusing to people. |
main.go
Outdated
| @@ -449,6 +450,8 @@ func main() { | |||
| rootCmd.AddCommand(createCmd) | |||
| createCmd.Flags().StringVar(&key, "key", "", | |||
| "KMS key to use for encryption") | |||
| createCmd.Flags().StringSliceVar(&smLocations, "locations", nil, | |||
| "Canonical IDs (e.g. 'us-east1') to replicate secrets to, seperated by comma's") | |||
There was a problem hiding this comment.
| "Canonical IDs (e.g. 'us-east1') to replicate secrets to, seperated by comma's") | |
| "Comma-separated canonical IDs in which to replicate secrets (e.g. 'us-east1,us-west-1')") |
pkg/berglas/berglas.go
Outdated
|
|
||
| // Locations is the list of custom locations a Secret Manager secret has | ||
| // been replicated to. Just set to nil if the secret is automatically | ||
| // replicated. |
There was a problem hiding this comment.
Add a note that this only applies when the storage is Secret Manager.
pkg/berglas/create.go
Outdated
| Automatic: &secretspb.Replication_Automatic{}, | ||
| }, | ||
| } | ||
| } else if len(i.Locations) > 0 { |
There was a problem hiding this comment.
It feels weird to treat nil and []string separately. Why not just treat len(i.Locations) == 0 as the default automatic replication case?
There was a problem hiding this comment.
I've modified the code to treat both cases identically. I was just a tad worried it might hide some issues when someone incorrectly generates this list on-the-fly. Although I can't think of any reasonable use case where I would be doing so.
pkg/berglas/create_test.go
Outdated
| @@ -47,6 +47,43 @@ func TestClient_Create_secretManager(t *testing.T) { | |||
| t.Fatal(err) | |||
| } | |||
|
|
|||
| if createResp.Locations != nil { | |||
| t.Errorf("expected the locations to be set to `nil`, got %+v", createResp.Locations) | |||
There was a problem hiding this comment.
Please follow the existing pattern of expected %#v to be %#v
| replication := versionResp.ReplicationStatus.GetUserManaged() | ||
| if replication != nil { | ||
| locations = make([]string, len(replication.Replicas)) | ||
| for i, r := range replication.Replicas { |
There was a problem hiding this comment.
These should probably be sorted for consistency.
|
Thank you for your feedback @sethvargo. It should now be fixed according to your specifications. With regards to the concern you mention: I get where you're coming from. It would be possible to also implement the Another alternative - if you prefer - is to just drop the locations from the |
|
Would you mind expanding on that a bit? I can't seem to find any place in the code where a |
|
Sorry - it's in main.go. There's a if/else block based on the type. If |
|
Ah, my mistake. I apparantly overlooked this possibility. This commit should fix that. |
|
This pull request has been automatically locked since there has not |
This PR contains backwards-compatible code which allows users to specify which locations they want a newly-created secret replicated to. It also passes this information along when reading a secret. The newly-created Locations parameter is set to (or kept at)
nilwhen automatic replication is chosen instead.This change is necessary for organisations where the rules only allow limited replication. Or where the secrets might contain PII and the GDPR places limits on the areas to store them at. The syntax is kept simple (and optional) without any complex options to keep Berglas as an easy-to-use wrapper.