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

Fix API parameter names #320

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Conversation

wiebeck
Copy link
Contributor

@wiebeck wiebeck commented Sep 10, 2024

This fixes the API spec files so that the generated parameters are named exactly like in the Vault API documentation, e.g. standbyok has to be all-lowercase and root_token_p_g_p_key had too many underscores.

@kdubb kdubb force-pushed the fix-api-param-names branch from 1207f4a to aab9025 Compare September 27, 2024 21:17
@kdubb kdubb requested a review from a team as a code owner September 27, 2024 21:17
@kdubb
Copy link
Contributor

kdubb commented Sep 27, 2024

@wiebeck Are you experiencing errors because of the names? It's been a while since I implemented this but these parameters/properties should all be tested in the client module.

@kdubb kdubb force-pushed the fix-api-param-names branch from aab9025 to 925bcdf Compare September 27, 2024 23:38
@wiebeck
Copy link
Contributor Author

wiebeck commented Sep 30, 2024

@wiebeck Are you experiencing errors because of the names? It's been a while since I implemented this but these parameters/properties should all be tested in the client module.

@kdubb: In fact I did. Our Vault was in a standby state and we wanted to try out the corresponding standbyOk parameter just to find out that it didn't do anything. Then we tried directly calling the Vault API via cURL and took a look at the API docs where we noticed the different letter case. It actually made a difference when calling Vault API via cURL.

Besides that the sys/init parameters ending on *PGPKeys will lead to generated API parameter names like _p_g_p_keys which is simply wrong.

@kdubb
Copy link
Contributor

kdubb commented Sep 30, 2024

If that's the case, we need tests for these (or figure out why the tests are not failing).

@wiebeck
Copy link
Contributor Author

wiebeck commented Nov 26, 2024

If that's the case, we need tests for these (or figure out why the tests are not failing).

I could need some help with the tests. :-/

@kdubb
Copy link
Contributor

kdubb commented Dec 3, 2024

@wiebeck I'd like to get this merged. What help do you need?

@vsevel
Copy link
Contributor

vsevel commented Dec 4, 2024

for the standbyok parameter, it is going to be difficult to test.
however I was able to demonstrate the issue @wiebeck is reporting using the sealedcode parameter (which we don't support at the moment).
here is how to reproduce manually:

% docker run --cap-add=IPC_LOCK -p 8200:8200 --name=dev-vault --rm hashicorp/vault
% export VAULT_TOKEN=hvs.XXXXXXXX

% curl -kv -X POST -H "X-Vault-Token: $VAULT_TOKEN" http://localhost:8200/v1/sys/seal

# no overriding retuns the default code 503
% curl -kv -H "X-Vault-Token: $VAULT_TOKEN" http://localhost:8200/v1/sys/health    
HTTP/1.1 503 Service Unavailable
...
{"initialized":true,"sealed":true,...

# correct case returns the new code 500
% curl -kv -H "X-Vault-Token: $VAULT_TOKEN" http://localhost:8200/v1/sys/health\?sealedcode\=500
HTTP/1.1 500 Internal Server Error

# wrong case returns the default code 503
% curl -kv -H "X-Vault-Token: $VAULT_TOKEN" http://localhost:8200/v1/sys/health\?sealedCode\=500
HTTP/1.1 503 Service Unavailable

# invalid parameter is completely ignored
% curl -kv -H "X-Vault-Token: $VAULT_TOKEN" http://localhost:8200/v1/sys/health\?XXXXX\=500
HTTP/1.1 503 Service Unavailable

it is going to be difficult to test the standbyok parameter because that would mean to start a cluster with an active and a standby vault.
I am wondering if vault ignores all unknown query parameters. or also invalid fields in body? I am hoping we did not miss too many of these.

I also tested the rootTokenPGPKey for the init. if we spell it correctly, and we set an invalid PGP key, this would fail with a 400, but if we keep the current invalid spelling, then the parameter is completely ignored.

this may prove difficult to test as well. we would need to set pgp keys in the init inside VaultTestExtension. do you see an easy way to do this @kdubb ?

I understand now better the PR. just look at VaultSysInitParams. the @JsonProperty annotations do not reflect the official API. the surprise for me is that we do not get bad requests for invalid parameters (probably to ease for compatibility across versions). and look at VaultSysHealthRequestFactory the query params are wrongly spelled.
@wiebeck is correct. I think we should go ahead with the changes.

@vsevel
Copy link
Contributor

vsevel commented Dec 10, 2024

Did you have a look @kdubb ?

@vsevel
Copy link
Contributor

vsevel commented Dec 19, 2024

hello @wiebeck can you go ahead and squash/rebase your commits.
unless there last minute comments, I think we can go ahead.
thanks for your patience.

@wiebeck wiebeck force-pushed the fix-api-param-names branch from 925bcdf to 436e128 Compare January 3, 2025 13:59
@wiebeck
Copy link
Contributor Author

wiebeck commented Jan 3, 2025

hello @wiebeck can you go ahead and squash/rebase your commits. unless there last minute comments, I think we can go ahead. thanks for your patience.

Alright, squashed and rebased. Happy new year everyone!

@vsevel vsevel merged commit ef066a6 into quarkiverse:main Jan 6, 2025
1 check passed
@vsevel
Copy link
Contributor

vsevel commented Jan 6, 2025

thanks @wiebeck

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.

3 participants