Skip to content

Conversation

@weichou1229
Copy link
Member

Close: #3333

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

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

What is the current behavior?

Issue Number: #3333

What is the new behavior?

Add secret creation API for storing EdgeX service exclusive secret to the Secret Store.

Does this PR introduce a breaking change?

  • Yes
  • No

New Imports

  • Yes
  • No

Specific Instructions

Are there any specific instructions or things that should be known prior to reviewing?

Other information

@weichou1229
Copy link
Member Author

This PR should hold until edgexfoundry/go-mod-core-contracts#624 merged.

@codecov-commenter
Copy link

Codecov Report

Merging #3510 (7646f62) into master (8e84e87) will increase coverage by 0.03%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3510      +/-   ##
==========================================
+ Coverage   40.41%   40.44%   +0.03%     
==========================================
  Files         204      206       +2     
  Lines       16790    16846      +56     
==========================================
+ Hits         6786     6814      +28     
- Misses       9527     9552      +25     
- Partials      477      480       +3     
Impacted Files Coverage Δ
internal/pkg/v2/controller/http/secret.go 78.57% <78.57%> (ø)
internal/pkg/v2/controller/http/common.go 40.47% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e84e87...7646f62. Read the comment docs.

Comment on lines 42 to 48
// add '/' in the full URL path if it's not already at the end of the base path or sub path
if !strings.HasSuffix(secretStoreInfo.Path, "/") && !strings.HasPrefix(path, "/") {
path = "/" + path
} else if strings.HasSuffix(secretStoreInfo.Path, "/") && strings.HasPrefix(path, "/") {
// remove extra '/' in the full URL path because secret store's (Vault) APIs don't handle extra '/'.
path = path[1:]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very recent change for the value of secretStoreInfo.Path. It is no longer the full base base. Just the sub path, i.e. "core-data/" . This code is no longer needed. Lower level code handles it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, I verified this code can be removed using App Service version and have removed it in this PR
edgexfoundry/app-functions-sdk-go#870
I will submit same clean-up for Device SDK.

Copy link
Member Author

@weichou1229 weichou1229 May 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

  1. But I got a 404 error when the path contains the prefix /
curl http://0.0.0.0:59860/api/v2/secret -X POST -H "Content-Type: application/json" \
  -d '{"apiVersion":"v2", "path": "/mqtt",  "secretData":[{"key":"username","value":"tester"}]}' | json_pp

{
   "apiVersion" : "v2",
   "message" : "adding secret failed -> Error found on handling secrets from underlying data-store: Received a '404' response from the secret store",
   "statusCode" : 500
}
  1. And got 400 error when the path contains the suffix //
% curl http://0.0.0.0:59860/api/v2/secret -X POST -H "Content-Type: application/json" \
  -d '{"apiVersion":"v2", "path": "mqtt//", "secretData":[{"key":"username","value":"tester"}]}' | json_pp

{
   "apiVersion" : "v2",
   "message" : "adding secret failed -> Error found on handling secrets from underlying data-store: Received a '400' response from the secret store",
   "statusCode" : 500
}

I thought we can improve the error message to return the actual error from the vault
https://github.com/edgexfoundry/go-mod-secrets/blob/master/internal/pkg/vault/secrets.go#L450

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm, please set logging to DEBUG so it logs the URL it is attempting to use so we have a better understanding of what is happening. Note // is never valid. So that error would be expected. Also please try just mqtt as the expected valid values are /mqtt & 'mqtt`

Yes, that error message could be improved. ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just verified with my Device SDK PR, that the two valid options are actually mqtt and mqtt/, which make more sense for specifying a sub-path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. I got the sane errors using /mqtt and /mqtt//, but no errors using nqtt or mqtt/ or even mqtt/something

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the sane errors using /mqtt and /mqtt//, but no errors using nqtt or mqtt/ or even mqtt/something

Yes, I also verified that. The user just needs to use DEBUG log to inspect and resolve the error.

I have no problem now. The changes made, please help review.

Add secret creation API for storing EdgeX service exclusive secret to the Secret Store.

Close: #3333
Signed-off-by: weichou <[email protected]>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@weichou1229 weichou1229 requested a review from lenny-goodell May 31, 2021 03:58
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lenny-goodell lenny-goodell merged commit 20e3038 into edgexfoundry:master Jun 2, 2021
@weichou1229 weichou1229 deleted the issue-3333 branch June 3, 2021 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[V2] Add secret creation API for support-notification service

3 participants