feat(secrets): support for csi directories #565

Tediferous · 2022-09-14T17:52:17Z

Closes #

💸 TL;DR

Allow baseplate go to read secrets from a vault csi directory

Jira

🧪 Testing Steps / Validation

✅ Checks

CI tests (if present) are passing
Adheres to code style for repo
Contributor License Agreement (CLA) completed if not a Reddit employee

fishy · 2022-09-14T18:01:34Z

secrets/secrets.go

+				return nil
+			}
+			// parse file
+			file, err := os.Open(path)


you actually need to use dir.Open instead of os.Open (the path is relative inside the fs)

also you did not close the file :)

fishy

can you please also add some unit tests for the directory case?

fishy · 2022-09-15T16:45:51Z

secrets/secrets.go

+				return nil
+			}
+			// parse file
+			file, err := fs.ReadFile(dir, path)


read the file fully into memory then use bytes.NewReader is very inefficient. just use d.Open, defer close after error check, would be much better.

fishy · 2022-09-15T16:47:38Z

secrets/store.go

@@ -41,13 +43,22 @@ func NewStore(ctx context.Context, path string, logger log.Wrapper, middlewares
 	store := &Store{
 		secretHandlerFunc: nopSecretHandlerFunc,
 	}
+	parser := store.parser


nit: move this to right before line 53 instead.

ghirsch-reddit

Agreed with Yuxuan's comment about the fs.ReadFile call, but otherwise lgtm

fishy · 2022-09-16T16:16:41Z

secrets/store_test.go

+	}
+	for _, tt := range tests {
+		t.Run(
+			tt.name,


this will run the subtests with the same name as from line 132. which will make the output very confusing.

either construct the name to add -file/-dir suffix (e.g. make line 132 tt.name + "-file" and here tt.name + "-dir", or run another level of subtests with names file/dir.

I would strongly prefer to add another level of subtests. you can also make it another level of for loop to reuse the majority of the code:

for _, tt := range tests { t.Run( tt.name, func(t *testing.T) { for label, path := range map[string]string{ "file": tmpPath, "dir": dirCSI, } { t.Run(label, func(t *testing.T) { store, err := secrets.NewStore(context.Background(), path, log.TestWrapper(t)) if err != nil { t.Fatal(err) } t.Cleanup(func() { store.Close() }) secret, err := store.GetSimpleSecret(tt.key) if tt.expectedError == nil && err != nil { t.Fatal(err) } if tt.expectedError != nil && err.Error() != tt.expectedError.Error() { t.Fatalf("expected error %v, actual: %v", tt.expectedError, err) } if !reflect.DeepEqual(secret, tt.expected) { t.Fatalf("expected %+v, actual: %+v", tt.expected, secret) } }) } }, ) }

you are correct

mterwill

A few style/readability nits, otherwise LGTM.

secrets/secrets.go

mterwill · 2022-09-16T18:01:19Z

secrets/secrets.go

+// NewDirSecrets parses a directory and returns its secrets
+func NewDirSecrets(dir fs.FS) (*Secrets, error) {


This stutters – secrets.NewDirSecrets. Perhaps secrets.FromDir?

secrets/secrets.go

mterwill · 2022-09-16T18:13:27Z

secrets/secrets.go

+			var secretFile CSIFile
+			err = json.NewDecoder(file).Decode(&secretFile)
+			if err != nil {
+				return err


🔕 bare returns like this can make errors difficult to trace back if they don't contain sufficient context. I'm not going to audit everything in here, but something to keep in mind for the future. Instead:

a) if the error message contains sufficient context, add a comment to let code reviewers and future readers know that you thought about this:

return err // contains context

or, b) wrap the error with a helpful message:

return fmt.Errorf("decoding %q: %s", path, err)

agreed but we should use %w for err.

mterwill · 2022-09-16T18:18:12Z

secrets/secrets.go

+			return nil
+		},
+	)
+	return secretsDocument, err


Unless you explicitly need the return value in the error case, it's idiomatic to return nil, err in the multi-return error case. The error and the return value are generally considered to be mutually exclusive and this pattern makes the intended use explicit and lowers overall cognitive overhead. (In this case you'd also have to adjust the signature to return a *Document)

mterwill · 2022-09-16T18:19:56Z

secrets/store_test.go

+  "renewable": false,
+  "data": {
+    "type": "simple",
+    "value": "Y2RvVXhNMVdsTXJma3BDaHRGZ0dPYkVGSg==",


Sanity check: this isn't a real secret, right? Looks more random than the rest.

I dont know, I copied this from the other example secret that this test was already using

I think it's a valid secret that's only used in unit tests. @pacejackson might know more of the origin of it 😅

I'm 99% sure it's not something we used in prod.

mterwill · 2022-09-16T18:22:09Z

secrets/secrets.go

@@ -298,3 +321,35 @@ func NewSecrets(r io.Reader) (*Secrets, error) {
 	}
 	return secrets, nil
 }
+
+func csiPathParser(dir fs.FS) (Document, error) {


🔕

Suggested change

func csiPathParser(dir fs.FS) (Document, error) {

// walkCSIDirectory ...

func walkCSIDirectory(dir fs.FS) (*Document, error) {

Co-authored-by: Matt Terwilliger <matt@terwilligers.com>

fishy · 2022-10-20T17:19:48Z

Oh sorry I didn't realize that you already got approvals from all reviewers. gonna merge this.

* Upgrade thrift to 0.17.0 This allows us to remove tDuplicateToProtocol and getClientError implementation as they are in thrift v0.17.0 now. There's no breaking changes between 0.16.0 and 0.17.0 regarding go library/compiler. * breakerbp: Fix prometheus gauge In the current implementation, the gauge value is only set when the state of the breaker changes. Which means if a breaker never got tripped, we would not report the gauge value at all. Set an initial value of 1 in constructor. * Update gRPC metrics to match v2 and spec (#572) * feat(secrets): support for csi directories (#565) Co-authored-by: Ted Dorfeuille <ted.dorfeuille@reddit.com> Co-authored-by: Matt Terwilliger <matt@terwilligers.com> Co-authored-by: Yuxuan 'fishy' Wang <yuxuan.wang@reddit.com> Co-authored-by: Kyle Lemons <kyle.lemons@reddit.com> Co-authored-by: Ted <Tediferous@users.noreply.github.com> Co-authored-by: Ted Dorfeuille <ted.dorfeuille@reddit.com> Co-authored-by: Matt Terwilliger <matt@terwilligers.com>

feat(secrets): support for csi directories

00a21df

Tediferous mentioned this pull request Sep 14, 2022

feat(filewatcher,secrets): Support for parsing Vault CSI Directories #472

Closed

fishy reviewed Sep 14, 2022

View reviewed changes

change file reader logic

2a54c5b

Tediferous marked this pull request as ready for review September 15, 2022 13:40

Tediferous requested a review from a team as a code owner September 15, 2022 13:40

Tediferous requested review from konradreiche, mterwill, ghirsch-reddit and fishy and removed request for a team September 15, 2022 13:40

fishy requested changes Sep 15, 2022

View reviewed changes

nit

5088f34

ghirsch-reddit approved these changes Sep 15, 2022

View reviewed changes

open logic

5917497

konradreiche approved these changes Sep 15, 2022

View reviewed changes

Ted Dorfeuille added 2 commits September 15, 2022 16:55

test

8fa3237

fix test

67779a2

Tediferous requested a review from fishy September 16, 2022 16:12

fishy requested changes Sep 16, 2022

View reviewed changes

fix test

2b3c346

Tediferous requested a review from fishy September 16, 2022 16:50

fishy approved these changes Sep 16, 2022

View reviewed changes

mterwill approved these changes Sep 16, 2022

View reviewed changes

Tediferous and others added 2 commits September 16, 2022 14:44

Apply suggestions from code review

62c8730

Co-authored-by: Matt Terwilliger <matt@terwilligers.com>

review tweaks

12f2c35

fishy merged commit 155114b into reddit:master Oct 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secrets): support for csi directories #565

feat(secrets): support for csi directories #565

Tediferous commented Sep 14, 2022 •

edited

Loading

fishy Sep 14, 2022 •

edited

Loading

fishy Sep 14, 2022

fishy left a comment

fishy Sep 15, 2022

fishy Sep 15, 2022

ghirsch-reddit left a comment •

edited

Loading

fishy Sep 16, 2022

Tediferous Sep 16, 2022

mterwill left a comment

mterwill Sep 16, 2022

mterwill Sep 16, 2022

fishy Sep 16, 2022

mterwill Sep 16, 2022

mterwill Sep 16, 2022

Tediferous Sep 16, 2022

fishy Sep 16, 2022

mterwill Sep 16, 2022

fishy commented Oct 20, 2022

		// NewDirSecrets parses a directory and returns its secrets
		func NewDirSecrets(dir fs.FS) (*Secrets, error) {

	func csiPathParser(dir fs.FS) (Document, error) {
	// walkCSIDirectory ...
	func walkCSIDirectory(dir fs.FS) (*Document, error) {

feat(secrets): support for csi directories #565

feat(secrets): support for csi directories #565

Conversation

Tediferous commented Sep 14, 2022 • edited Loading

💸 TL;DR

🧪 Testing Steps / Validation

✅ Checks

fishy Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghirsch-reddit left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mterwill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy commented Oct 20, 2022

Tediferous commented Sep 14, 2022 •

edited

Loading

fishy Sep 14, 2022 •

edited

Loading

ghirsch-reddit left a comment •

edited

Loading