Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write a better error message when secret not found #182

Closed
wants to merge 1 commit into from

Conversation

simonjohansson
Copy link
Contributor

Currently when a secret is not found we get an error message

Response code 404 (Not Found)

If you have multiple secrets it can be tedious to figure out which secret is missing.

This PR fixes that by giving the error message

Unable to retrieve result for "secret/data/notFound". Double check your Key.

@hashicorp-cla
Copy link

hashicorp-cla commented Jan 27, 2021

CLA assistant check
All committers have signed the CLA.

@simonjohansson
Copy link
Contributor Author

@jasonodonnell ping pong :)

} catch (error) {
const {response} = error;
if (response.statusCode === 404) {
throw Error(`Unable to retrieve result for "${path}". Double check your Key.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw Error(`Unable to retrieve result for "${path}". Double check your Key.`)
throw Error(`Unable to retrieve result for "${path}".`)

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing key because it may be due to other things (token perms, connectivity, etc).

secret/data/test secret | NAMED_SECRET ;
secret/data/notFound kehe | NO_SIR ;`);

expect(exportSecrets()).rejects.toEqual(Error(`Unable to retrieve result for "secret/data/notFound". Double check your Key.`));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(exportSecrets()).rejects.toEqual(Error(`Unable to retrieve result for "secret/data/notFound". Double check your Key.`));
expect(exportSecrets()).rejects.toEqual(Error(`Unable to retrieve result for "secret/data/notFound".`));

@ps-jay
Copy link

ps-jay commented Nov 3, 2021

This would be nice to have, see also 400 errors: #273

@dhs3000
Copy link

dhs3000 commented Apr 6, 2022

Hi everyone, it would be great if this could be merged. It would be super helpful to see right away what causes the error.

@calvn
Copy link
Contributor

calvn commented Apr 15, 2022

@simonjohansson we've walk through a few scenarios on both making API calls directly and via the CLI that results in errors, and think that there's value in returning the request path as well as the error itself.

API

# Invalid path - non-existent
$ curl -vv -H "X-Vault-Token: $(vault print token)" http://localhost:8200/v1/secret/data/not-found
*   Trying ::1:8200...
* Connected to localhost (::1) port 8200 (#0)
> GET /v1/secret/data/not-found HTTP/1.1
> Host: localhost:8200
> User-Agent: curl/7.77.0
> Accept: */*
> X-Vault-Token: root
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Cache-Control: no-store
< Content-Type: application/json
< Strict-Transport-Security: max-age=31536000; includeSubDomains
< Date: Fri, 15 Apr 2022 17:03:36 GMT
< Content-Length: 14
<
{"errors":[]}
* Connection #0 to host localhost left intact

# Invalid path - no matching handler
$ curl -vv -H "X-Vault-Request: true" -H "X-Vault-Token: $(vault print token)" http://localhost:8200/v1/blah
*   Trying ::1:8200...
* Connected to localhost (::1) port 8200 (#0)
> GET /v1/blah HTTP/1.1
> Host: localhost:8200
> User-Agent: curl/7.77.0
> Accept: */*
> X-Vault-Request: true
> X-Vault-Token: root
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Cache-Control: no-store
< Content-Type: application/json
< Strict-Transport-Security: max-age=31536000; includeSubDomains
< Date: Fri, 15 Apr 2022 17:01:09 GMT
< Content-Length: 43
<
{"errors":["no handler for route 'blah'"]}
* Connection #0 to host localhost left intact

# Invalid permission
› curl -vv -H "X-Vault-Token: $(vault print token)" http://localhost:8200/v1/blah
*   Trying ::1:8200...
* Connected to localhost (::1) port 8200 (#0)
> GET /v1/blah HTTP/1.1
> Host: localhost:8200
> User-Agent: curl/7.77.0
> Accept: */*
> X-Vault-Token: hvs.CAESIKI_FMCZgw5boLpfQY_h-eWyF0-ZehTboHsDrrAxpn2OGh4KHGh2cy5JZTdEWWFjdHpsTllpQ3BvV1RzbHQ3OVM
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 403 Forbidden
< Cache-Control: no-store
< Content-Type: application/json
< Strict-Transport-Security: max-age=31536000; includeSubDomains
< Date: Fri, 15 Apr 2022 17:36:31 GMT
< Content-Length: 60
<
{"errors":["1 error occurred:\n\t* permission denied\n\n"]}
* Connection #0 to host localhost left intact

CLI

# Invalid path - non-existent
$ vault read secret/data/not-found
No value found at secret/data/not-found

# Invalid path - no matching handler
$ vault read blah
No value found at blah

# Invalid permission
$ vault read blah
Error reading blah: Error making API request.

URL: GET http://localhost:8200/v1/blah
Code: 403. Errors:

* 1 error occurred:
	* permission denied

Could you update the PR such that when the error is caught, the Error that we return contains both the path (similar to what you currently have) as well as the error itself, if that's not empty? We can return all errors this way and not just on 404's.

@simonjohansson
Copy link
Contributor Author

@calvn Howdie. I don't work in the company where we used the action anymore. So I wont have any time/setup to expand the PR.

swenson pushed a commit that referenced this pull request Apr 20, 2022
This is a follow-up PR to #182 to address additional comments.
swenson pushed a commit that referenced this pull request Apr 20, 2022
@swenson
Copy link
Contributor

swenson commented Apr 20, 2022

Thanks @simonjohansson. I'll go ahead and close this one in favor of #306 so that we can address these comments. I've left your commit in so that you still get credit for it.

@swenson swenson closed this Apr 20, 2022
swenson added a commit that referenced this pull request Apr 20, 2022
* Write a better error message when key not found

* Address additional comments on #182

Co-authored-by: Simon Johansson <simon@simonjohansson.com>
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.

8 participants