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

Provide Descriptive Error when Enterprise-only Paths Called in Open-source Version #18870

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

marcboudreau
Copy link
Contributor

@marcboudreau marcboudreau commented Jan 26, 2023

This PR addresses Issue #11539. It expands the entPaths slice of framework.Path pointers, which currently only contains a single path, replication/status, to include all paths for enterprise-only features. This will allow the open-source version of Vault to return a more descriptive error if enterprise Vault CLI commands are sent to it.

All paths related to enterprise-only features will share a common handler that returns a logical.Response that contains the error message enterprise-only feature along with a logical.ErrUnsupportedPath error. From the client perspective, here's the response received from an open-source edition Vault server, using curl:

$ curl -H "X-Vault-Token: $VAULT_TOKEN" -d '{"custom_metadata": {"foo": "abc", "bar": "123"}}' "$VAULT_ADDR/v1/sys/namespaces/ns1"
{"errors":["enterprise-only feature"]}

And using the vault CLI:

$ vault namespace create ns1  
Error creating namespace: Error making API request.

URL: PUT http://localhost:8200/v1/sys/namespaces/ns1
Code: 404. Errors:

* enterprise-only feature

Behaviour Change

This change includes a minor behavioural change to the handling of the existing replication/status path, so that requests sent to it will result in the same response as all of the other paths being introduced. Sending a read operation to this path in the open-source edition, used to result in an HTTP 200 response with the following body:

{
  "data": {
    "mode": "disabled"
  }
}

After this change, this endpoint will behave as described above.

@marcboudreau marcboudreau changed the title Define Enterprise-only Paths with Common Handler Provide Descriptive Error when Enterprise-only Paths Called in Open-source Version Jan 26, 2023
Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Looks really good! I wonder if it'd be worthwhile to add some go tests for this change?

@marcboudreau
Copy link
Contributor Author

I've refactored the code a bit, so that we can add a test in the enterprise repository that will flag any discrepancies between the results of the entStubPaths and the entPaths functions.

Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Still looks good 😄. I added one nit about the commented-out path, but it is not a blocker.

"replication/dr/secondary/recover": {logical.UpdateOperation},
"replication/dr/secondary/reindex": {logical.UpdateOperation},
"replication/reindex": {logical.UpdateOperation},
// "replication/status": {logical.ReadOperation},
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worthwhile to drop this or pull the comment from below up here and note that it will be handled separately below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think dropping the comment is better. Without that comment the elements of the slice will all be aligned.

@marcboudreau marcboudreau merged commit d11f7a2 into main Apr 21, 2023
@marcboudreau marcboudreau deleted the marcboudreau/issue-11539 branch April 21, 2023 20:14
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.

2 participants