VAULT-8703 Add warning for dangerous undocumented overrides, if used, in status response #17855
Conversation
| @@ -402,6 +402,9 @@ func (t TableFormatter) OutputSealStatusStruct(ui cli.Ui, secret *api.Secret, da | |||
| if status.LastWAL != 0 { | |||
| out = append(out, fmt.Sprintf("Last WAL | %d", status.LastWAL)) | |||
| } | |||
| if len(status.Warnings) > 0 { | |||
| out = append(out, fmt.Sprintf("Warnings | %v", status.Warnings)) | |||
There was a problem hiding this comment.
I suspect %v isn't going to do a great job of handling multiple strings containing whitespace, it won't be clear where one ends and the next begins.
There was a problem hiding this comment.
Yeah, it does kinda just append them. I think to some extent that should be on us to word them well, but similarly, I would hope we don't get into the kind of state where someone has 10 of these kinds of warnings.
In other words, this could be optimistic, but I feel like >1 warning is a very rare case, as long as we keep what we warn on broadly consistent.
There was a problem hiding this comment.
This might look funny but would it be worth quoting them so there is at least some means of separation?
| func (core *Core) getStatusWarnings() []string { | ||
| var warnings []string | ||
| if core.GetCoreConfigInternal() != nil && core.GetCoreConfigInternal().DisableSSCTokens { | ||
| warnings = append(warnings, "Server Side Consistent Tokens are disabled, due to the "+ |
There was a problem hiding this comment.
Should we have a policy that we introduce warnings like this, we link them to something external that we can modify? I just worry that otherwise we may have to come back multiple times to this code to add extra details in response to user questions.
There was a problem hiding this comment.
Depending on what you mean, this could amount to us having some level of documentation for these (otherwise) undocumented options, which does have potential implications. I'm not entirely against this, but I worry it would go against our core goal for the kinds of options we want to warn about.
@ccapurso can probably confirm in more detail, but I think the main onus for this is some kind of way for us to tell at-a-glance that this feature is being used, as well as reminding the customer that it's not particularly supported. I hope we wouldn't require too much detail here, and I hope the number of customers that see these warnings remains extremely low.
There was a problem hiding this comment.
My understanding of the original discussion was that these warnings were going to identify that exceptional overrides were being used. Long-lived, documented overrides are often less likely to be harmful. The means to identify something in this way is almost more for identifying potential things to look at during incident response.
There was a problem hiding this comment.
this could amount to us having some level of documentation for these (otherwise) undocumented options
Forgot about that wrinkle.
If you went with something shorter like "VAULT_DISABLE_SERVER_SIDE_CONSISTENT_TOKENS=true", that would address my other comment about %v, and this one too, in the sense that I'm not worried that we'll have to revisit the verbiage. It wouldn't do a great job of "reminding the customer that it's not particularly supported", but I guess I'm afraid that we're biting off more than we can chew in attempting to pithily tell the user they should be moving away from this setting.
There was a problem hiding this comment.
In that case, we wouldn't necessarily be giving warnings -- which might be okay, to be clear. If we shorten it, we'd need a different name, like "Unrecommended Settings" (open to suggestions if you can think of something that sounds better), but I guess my worry with that is that that's also lacking in description too. I guess in either case, we're picking our poison.
There was a problem hiding this comment.
Yeah, I'm not hung up on this, I'm going to approve and leave it for you to decide. My concerns may be unwarranted, I just imagine various scenarios where we get into trouble by trying to thread this needle of giving vague warnings without expanding on them anywhere. Like what if a big company using the var tries to upgrade, sees this, and tells us: "Look, we know we need to move away from this, and we're working on it, but our users will freak out if they see this message, can you tone it down? We can't upgrade with this text in vault status."
There was a problem hiding this comment.
Yeah, I definitely see the argument. Like I said, I guess I see this as picking our poison, as something like "Unsupported Settings" could make customers raise their hackles too.
I think I'll keep it as-is, especially as most customers hopefully won't see this, and "Warnings" feels like quite a generic name if we do want to change the contents in the future.
… in status response (hashicorp#17855) * VAULT-8703 Add warning for dangerous undocumented overrides, if used, in status response * VAULT-8703 add changelog * VAULT-8703 fix append
If this environment variable is set, you will now get the following output:
"Warnings" will not be present if this option is not given.
Also, json output:
I couldn't find an appropriate docs page to add the note about this new warning option in, but I think it's probably fine without, as it only affects customers that have added out-of-docs options.