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
Fixes #29156: Failing sensitive variables values are not logged #30552
Fixes #29156: Failing sensitive variables values are not logged #30552
Conversation
#29156 looked good as a first issue for me. And I agree very much with the problem stated. |
Hi @gbataille, thanks for the contribution! We are looking into it, but I cannot commit to when this can be reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
I think the general idea here is a good solution to this problem. There are a couple of improvements I'd like to make before going ahead.
Whenever possible, we should leave in the non-file source information (e.g. set using -var="foo=..."
). I left an inline note about this.
For sensitive variables set from a file, I agree that the best option we have is to replace the source range. However, we need to inform the user where the variable value comes from, by including the .tfvars filename and line number in the detail string.
In this case, we can also follow the existing pattern for non-file sourced values of using the variable declaration as the source range, to give the user extra context.
Here are some example diagnostic designs based on these thoughts:
╷
│ Error: Invalid value for input variable
│
│ on main.tf line 1:
│ 1: variable "foo" {
│
│ Unsuitable value for var.foo set using -var="foo=...": a bool is required.
╵
╷
│ Error: Invalid value for input variable
│
│ on main.tf line 1:
│ 1: variable "foo" {
│
│ The given value is not suitable for var.foo, which is sensitive: a bool is required. Value
| declared at terraform.tfvars line 1.
╵
(Note the trailing full stop at the end of the description string, which is Terraform house style.)
Diagnostics for non-sensitive variable values should not change, so I've omitted these.
Let me know if anything is unclear here, or if you'd like me to review alternative designs. Thanks again!
internal/terraform/eval_variable.go
Outdated
// and not by the operator directly. In such a case, the failing secret value should not be | ||
// displayed to the operator | ||
if cfg.Sensitive { | ||
subject = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any need to remove the subject here when it comes from a non-file source, and we probably also need to treat the logic for appending to the diagnostic detail message separately too. I think therefore we probably want an if cfg.Sensitive { … }
block inside both branches of the if nonFileSource != "" { … }
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's how I coded it originally and changed it back to that.
But I read all your comments and they make sense to me. I'll try to take some time this week to update the PR
00417a6
to
e6a918d
Compare
@gbataille is attempting to deploy a commit to the HashiCorp Team on Vercel. A member of the Team first needs to authorize it. |
sorry for not having come back sooner. Did not take the time.
--> I did not find yet a "smart" way to do it.
I have in the meantime added the full stop at the end of the message and moved the logic to just one branch of the code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! I think this is really close.
While I like your ideas about adding sensitive value redaction to diagnostics, I agree that it would be a lot of work. What I'd propose for this PR is to adjust one diagnostic to include a reference to the origin of the invalid variable value in the detail text, while keeping the code snippet pointing at the variable definition. See inline suggestion.
There are also some failing tests to fix up, but once those two things are done I think this is good to go!
internal/terraform/eval_variable.go
Outdated
// displayed to the operator | ||
if cfg.Sensitive { | ||
subject = nil | ||
detail += fmt.Sprintf("\n\n%s is marked as sensitive. The failing variable assignment is not printed.", addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detail += fmt.Sprintf("\n\n%s is marked as sensitive. The failing variable assignment is not printed.", addr) | |
detail += fmt.Sprintf("\n\n%s is marked as sensitive. Invalid value defined at %s.", addr, sourceRange.ToHCL()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I see. feels a bit hackish in the sense that diagnostics and subject handling are managed in common way up there, but I definitely can make that work (don't know when though, sorry :| )
e6a918d
to
378ee6a
Compare
I pushed a tiny tweak to add back the config declaration as the source parameter in both cases, resulting in diags like this: With that in place this looks good to me. We're in the freeze for 1.2.0 at the moment, but I'm marking this approved and plan to merge it once that freeze is over. Thanks again for working on this! |
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
This change will go out in the upcoming 1.2.2 release. Thanks once more! |
cool, cheers
…---
Gregory Bataille
On Mon, May 30, 2022 at 5:40 PM Alisdair McDiarmid ***@***.***> wrote:
This change will go out in the upcoming 1.2.2 release. Thanks once more!
—
Reply to this email directly, view it on GitHub
<#30552 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIPX2JW6V7YWCZ63IUJQU3VMTOO5ANCNFSM5OXQLPTQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
No description provided.