feat: Added get keys functionality #161

drkfmorton · 2022-08-31T13:37:25Z

Signed-off-by: Kyle Morton [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/go-mod-secrets/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?)
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?)

Testing Instructions

New Dependency Instructions (If applicable)

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i

Response from Vault for a LIST would be:

data:
  keys: [ secret1 secret2 subpath/ ]

We want to return:

listsecrets():
return []string{
secret1
secret2
? subpath/subkey <-- suggest don't return this (note that vault won't show subkey without another call)
? subpath/ <-- vault returns this today
? ???? <-- would consul return subpath/ too? if so, go ahead and return otherwise scrub data3/ from Vault output
}

instead of getTestSecretsKeysData() call it listTestSecretsKeysData() to show it is the LIST verb output vs GET verb output

Signed-off-by: Kyle Morton <[email protected]>

internal/pkg/vault/secrets_test.go

bnevis-i · 2022-08-31T17:14:06Z

internal/pkg/vault/secrets_test.go

+			},
+		},
+		{
+			name:              "Handle HTTP no path error",
+			path:              testPath,
+			expectedValues:    nil,
+			expectError:       true,
+			expectedErrorType: TestConnErrorPathNotFound,
+			expectedDoCallNum: 1,
+			caller: &ErrorMockCaller{
+				ReturnError: false,
+				StatusCode:  404,
+				ErrorType:   pkg.NewErrPathNotFound("Not found"),
+			},
+		},
+		{
+			name:              "Handle non-200 HTTP response",
+			path:              testPath,
+			expectedValues:    nil,
+			expectError:       true,
+			expectedErrorType: TestConnError,
+			expectedDoCallNum: 1,
+			caller: &ErrorMockCaller{
+				ReturnError: false,
+				StatusCode:  400,
+				ErrorType:   pkg.NewErrSecretStore("Error"),
+			},
+		},
+		{
+			name:              "Get Key with unknown path",
+			path:              "/nonexistentpath",
+			expectedValues:    nil,
+			expectError:       true,
+			expectedErrorType: TestConnErrorPathNotFound,
+			expectedDoCallNum: 1,
+			caller: &InMemoryMockCaller{
+				DataList: testData,
+			},
+		},
+		{
+			name:              "URL Error",
+			path:              "bad path for URL",
+			expectedValues:    nil,
+			expectError:       true,
+			expectedErrorType: errors.New(""),
+			caller: &InMemoryMockCaller{
+				DataList: testData,
+			},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cfgHTTP := types.SecretConfig{
+				Host:      "localhost",
+				Port:      8080,
+				Protocol:  "http",
+				Namespace: testNamespace,
+			}
+			ssm := Client{
+				Config:     cfgHTTP,
+				HttpCaller: test.caller,
+				lc:         logger.NewMockClient(),
+			}
+
+			actual, err := ssm.GetKeys(test.path)
+			if test.expectError {
+				require.Error(t, err)
+
+				eet := reflect.TypeOf(test.expectedErrorType)
+				aet := reflect.TypeOf(err)
+				if !aet.AssignableTo(eet) {
+					t.Errorf("Expected error of type %v, but got an error of type %v", eet, aet)
+				}
+
+				return
+			}
+
+			var mockType string
+			var callCount int
+			switch v := test.caller.(type) {
+			case *ErrorMockCaller:
+				mockType = "ErrorMockCaller"
+				callCount = v.DoCallCount
+			case *InMemoryMockCaller:
+				mockType = "InMemoryMockCaller"
+				callCount = v.DoCallCount
+			}
+
+			require.Equalf(t, test.expectedDoCallNum, callCount,
+				"Expected %d %s.Do calls, got %d", mockType, test.expectedDoCallNum, callCount)
+			fmt.Println("a ", actual)
+			fmt.Println("ae ", test.expectedValues)
+			for k, expected := range test.expectedValues {
+				if actual[k] != expected {
+					assert.Equalf(t, expected, actual[k], "Expected value '%s', but got '%s'", expected, actual[k])
+				}
+			}
+		})
+	}
+}


This whole test is wrong.

First, document the logic structure of the secret store:

.../secret1
{ one: uno, two: dos, three: tres }
.../secret2
{ one: "1", two: "2", three: "3" }
.../subpath/secret3
{ one: "i", two: "ii", three: "iii" }

Then, we should test the following cases:

GetKeys("") returns [ "secret1", "secret2", "subpath/" ]
GetKeys("secret1") returns Error for [] (verify with Vault for actual behavior of LIST of a secret instead of GET of a secret)
GetKeys("subpath") returns [ "secret3" ]
GetKeys("doesnotexist") returns Error

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i

Please add comments on what this Test..._GetKeys is testing.

internal/pkg/vault/secrets_test.go

bnevis-i · 2022-08-31T20:05:23Z

internal/pkg/vault/secrets.go

+
+	var validKeys []string
+	for _, key := range data {
+		if len(key) == strings.Index(key, "/")+1 || strings.Index(key, "/") < 1 {


Would HasSuffix() work better here?

I removed this check do to me thinking a path + subpath could be returned. This check was removing it from the list.

internal/pkg/vault/secrets.go

lenny-goodell · 2022-08-31T20:39:28Z

internal/pkg/vault/secrets_test.go

+		name              string
+		path              string
+		expectedValues    []string
+		expectError       bool


This field is redundant to expectedErrorType . Can just check expectedErrorType == nil for expected error

This was not resolved in this test like it was in other test. expectError bool still present.

internal/pkg/vault/secrets_test.go

Signed-off-by: Kyle Morton <[email protected]>

codecov-commenter · 2022-08-31T22:40:17Z

Codecov Report

Merging #161 (52c5176) into main (c185b68) will increase coverage by 0.20%.
The diff coverage is 82.85%.

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
+ Coverage   77.37%   77.57%   +0.20%     
==========================================
  Files          17       17              
  Lines         906      941      +35     
==========================================
+ Hits          701      730      +29     
- Misses        147      150       +3     
- Partials       58       61       +3

Impacted Files	Coverage Δ
internal/pkg/vault/secrets.go	`75.00% <82.85%> (+1.18%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bnevis-i

LGTM

bnevis-i

Please also change secrets/interfaces.go

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i

Please run make test and fix any failing unit tests

[2022-09-01T13:56:41.654Z] # github.com/edgexfoundry/go-mod-secrets/v2/pkg/listener [github.com/edgexfoundry/go-mod-secrets/v2/pkg/listener.test]

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:126:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:135:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:189:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:198:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:207:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:216:23: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in struct literal:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:261:32: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in argument to NewInMemoryCacheListener:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:279:32: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in argument to NewInMemoryCacheListener:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:297:32: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in argument to NewInMemoryCacheListener:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

[2022-09-01T13:56:41.654Z] pkg/listener/poll_test.go:314:32: cannot use testClient (variable of type MockSecretClient) as type secrets.SecretClient in argument to NewInMemoryCacheListener:

[2022-09-01T13:56:41.654Z] 	MockSecretClient does not implement secrets.SecretClient (missing GetKeys method)

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i

LGTM assuming unit tests pass this time.

lenny-goodell · 2022-09-01T16:17:09Z

internal/pkg/vault/secrets_test.go

+		name              string
+		path              string
+		expectedValues    []string
+		expectError       bool


This was not resolved in this test like it was in other test. expectError bool still present.

Signed-off-by: Kyle Morton <[email protected]>

lenny-goodell

LGTM

feat: Added get keys functionality

66a840f

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i requested changes Aug 31, 2022

View reviewed changes

feat: Updated get keys functionality

34a77ad

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i requested changes Aug 31, 2022

View reviewed changes

feat: Updated unit test functionality

5708ff6

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i requested changes Aug 31, 2022

View reviewed changes

internal/pkg/vault/secrets_test.go Outdated Show resolved Hide resolved

internal/pkg/vault/secrets_test.go Outdated Show resolved Hide resolved

internal/pkg/vault/secrets_test.go Outdated Show resolved Hide resolved

bnevis-i reviewed Aug 31, 2022

View reviewed changes

lenny-goodell requested changes Aug 31, 2022

View reviewed changes

feat: Updated unit test

52c5176

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i approved these changes Sep 1, 2022

View reviewed changes

bnevis-i requested changes Sep 1, 2022

View reviewed changes

feat:Added interface

e5c1b38

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i requested changes Sep 1, 2022

View reviewed changes

feat: Fixed unit test

6927774

Signed-off-by: Kyle Morton <[email protected]>

bnevis-i previously approved these changes Sep 1, 2022

View reviewed changes

lenny-goodell requested changes Sep 1, 2022

View reviewed changes

feat: Updated unit test

b8c65c9

Signed-off-by: Kyle Morton <[email protected]>

drkfmorton dismissed bnevis-i’s stale review via b8c65c9 September 1, 2022 18:44

lenny-goodell approved these changes Sep 1, 2022

View reviewed changes

lenny-goodell requested a review from bnevis-i September 1, 2022 18:59

bnevis-i approved these changes Sep 1, 2022

View reviewed changes

bnevis-i merged commit 46e806f into edgexfoundry:main Sep 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added get keys functionality #161

feat: Added get keys functionality #161

drkfmorton commented Aug 31, 2022 •

edited by bnevis-i

Loading

bnevis-i left a comment •

edited

Loading

bnevis-i Aug 31, 2022

bnevis-i left a comment

bnevis-i Aug 31, 2022

drkfmorton Aug 31, 2022

lenny-goodell Aug 31, 2022

lenny-goodell Sep 1, 2022

codecov-commenter commented Aug 31, 2022

bnevis-i left a comment

bnevis-i left a comment

bnevis-i left a comment

bnevis-i left a comment

lenny-goodell Sep 1, 2022

lenny-goodell left a comment

feat: Added get keys functionality #161

feat: Added get keys functionality #161

Conversation

drkfmorton commented Aug 31, 2022 • edited by bnevis-i Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

bnevis-i left a comment • edited Loading

Choose a reason for hiding this comment

bnevis-i Aug 31, 2022

Choose a reason for hiding this comment

bnevis-i left a comment

Choose a reason for hiding this comment

bnevis-i Aug 31, 2022

Choose a reason for hiding this comment

drkfmorton Aug 31, 2022

Choose a reason for hiding this comment

lenny-goodell Aug 31, 2022

Choose a reason for hiding this comment

lenny-goodell Sep 1, 2022

Choose a reason for hiding this comment

codecov-commenter commented Aug 31, 2022

Codecov Report

bnevis-i left a comment

Choose a reason for hiding this comment

bnevis-i left a comment

Choose a reason for hiding this comment

bnevis-i left a comment

Choose a reason for hiding this comment

bnevis-i left a comment

Choose a reason for hiding this comment

lenny-goodell Sep 1, 2022

Choose a reason for hiding this comment

lenny-goodell left a comment

Choose a reason for hiding this comment

drkfmorton commented Aug 31, 2022 •

edited by bnevis-i

Loading

bnevis-i left a comment •

edited

Loading