CLI: add an option for renew command fail on non-fulfillable request… #29060
Conversation
|
|
saiaunghlyanhtet
requested review from
ltcarbonell
and removed request for
a team
saiaunghlyanhtet
force-pushed
the
feature/valut-token-renew-fail
branch
from
6944989 to
0bfa0c1
Compare
There was a problem hiding this comment.
Hi @saiaunghlyanhtet, thank you for the contribution! I'm currently gathering some opinions regarding this CLI-only implementation, will get back to you end of next week at the latest on that.
saiaunghlyanhtet
force-pushed
the
feature/valut-token-renew-fail
branch
2 times, most recently
from
f9a4096 to
06a493c
Compare
saiaunghlyanhtet
force-pushed
the
feature/valut-token-renew-fail
branch
from
06a493c to
94a436b
Compare
changelog/29060.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:improvement | |||
| CLI: adds an optional flag (--fail-if-not-fullfilled) to the renew command, which lets the renew command fail on unfulfillable requests and allows command chaining to allow further executions. | |||
There was a problem hiding this comment.
there's one remaining typo in this fulfilled. Also in the PR title and description.
| @@ -53,3 +59,7 @@ flags](/vault/docs/commands) included on all commands. | |||
| Vault will not honor this request for periodic tokens. If not supplied, Vault will use | |||
| the default TTL. This is specified as a numeric string with suffix like "30s" | |||
| or "5m". This is aliased as "-i". | |||
|
|
|||
| - `--fail-if-not-fulfilled` - Allow command chaining after renew request fail. | |||
There was a problem hiding this comment.
I believe this should match the command Usage field that's displayed in vault token renew --help. This seems to be a consistent pattern across options for many (all?) commands. I'd suggest setting both Usage and these docs to an initial short description of what the flag does, like your Fail if the requested TTL increment cannot be fully fulfilled. and then move on with these additional details of how it can be used.
Also a nit: the current text seems a bit repetitive by mentioning that the flag allows command chaining twice.
| Allow command chaining after renew command fail: | ||
|
|
||
| ```shell-session | ||
| $ vault token renew -increment=30m 96ddf4bc-d217-f3ba-f9bd-017055595017 --fail-if-not-fulfilled | vault login | ||
| ``` | ||
|
|
There was a problem hiding this comment.
great addition of an example of using the command chaining! We just need to change | to ||
Also, all of the help texts that I could find for existing commands show at most 3 examples and say For a full list of examples, please see the documentation so I think it's better to keep this additional example in the docs only, as you currently have it.
There was a problem hiding this comment.
Can you show me the file path for documentation as I cannot find it.
There was a problem hiding this comment.
The file referenced by this thread is part of the documentation, it's what shows up on the website. On my previous comment I was just saying that I agree with your idea of keeping this additional example only here in the docs as opposed to also listing it together with the other examples in the CLI help text.
command/token_renew_test.go
Outdated
| if tc.fail { | ||
| client.Auth().Token().Renew(token, 1) | ||
| } | ||
|
|
There was a problem hiding this comment.
Sorry I missed this in the previous iteration, but doesn't it suffice for this test to ask for a bigger increment than what is allowed by the policy? Why is this call here necessary? I don't understand the idea behind this new fail field
There was a problem hiding this comment.
Asking for a bigger increment does not suffice as error messages are different. So, it would be better if we allow the new "fail" field, and add a test case for this renew command. For tc.fail call, I forgot to remove it.
There was a problem hiding this comment.
Asking for a bigger increment does not suffice as error messages are different.
I would disagree there, if the token max_ttl is set to like 60 min and we try to renew it for 61 min with the new --fail-if-not-fulfilled flag set I would expect the command to fail and the message Token renewal completed with capped duration, failing the command because of --fail-if-not-fulfilled to be printed. However we might have to set the ExplicitMaxTTL when first generating the token.
There was a problem hiding this comment.
I am not clear about setting ExplicitMaxTTL. Do I need to set the ExplicitMaxTTL in token_create.go or in the test setup?
There was a problem hiding this comment.
only in the test setup for sure. We could update testTokenAndAccessor to have a MaxTTL, but would need to be careful not to break any of the other tests that rely on it. Or we could rely on the default MaxTTL of 32 days and have the test case request the renewal for 33 days in order to have a capped duration. Or this test specifically could call client.Auth().Token().Create() directly, without the helper function, passing whatever MaxTTL works for the test.
There was a problem hiding this comment.
Just used the default MaxTTL as updating testTokenAndAccessor and calling client.Auth().Token().Create() have some problems.
saiaunghlyanhtet
changed the title
command/token_renew.go
Outdated
| @@ -140,5 +150,10 @@ func (c *TokenRenewCommand) Run(args []string) int { | |||
| return 2 | |||
| } | |||
|
|
|||
| if c.flagFailIfNotFulfilled && secret.LeaseDuration < int(increment.Seconds()) { | |||
There was a problem hiding this comment.
I just noticed this secret.LeaseDuration is always 0 on token renewal, we should instead be checking the secret.Auth.LeaseDuration field, which aligns with the docs for this endpoint.
This also highlights that we're missing a test case where the new flag is set but the increment is not capped, so the the new warning is not printed and the command doesn't fail. Can you add that?
… to allow command chaining Signed-off-by: saiaunghlyanhtet <[email protected]>
saiaunghlyanhtet
force-pushed
the
feature/valut-token-renew-fail
branch
from
94a436b to
028f058
Compare
Description
What does this PR do?
This pull request adds an optional flag (--fail-if-not-fulfilled) to the renew command, which lets the renew command fail on unfulfillable requests and allows command chaining to allow further executions.
Related Issue: #28710