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

jsonplan and jsonstate: include sensitive_values in state representations #28889

Merged
merged 2 commits into from
Jun 14, 2021

Conversation

mildwonkey
Copy link
Contributor

A sensitive_values field has been added to the resource in state and planned values which is a map of all sensitive attributes with the values set to true. To achieve this, I stole and exported the sensitiveAsBool function that was in jsonplan (it had to be moved to jsonstate to avoid import cycles).

It wasn't entirely clear to me if the values in state would suffice, or if we also need to consult the schema - I believe that this is sufficient for state files written since v0.15, and if that's incorrect or insufficient, I'll add in the provider schema step as well.

I also updated the documentation, and, since we've considered this before, bumped the FormatVersions for both jsonstate and jsonplan.

@mildwonkey mildwonkey requested a review from a team June 7, 2021 17:20
@mwhooker
Copy link
Contributor

mwhooker commented Jun 7, 2021

did you read my mind?


// SensitiveValues is similar to AttributeValues, but with all sensitive
// values replaced with true, and all non-sensitive leaf values omitted.
SensitiveValues map[string]bool `json:"sensitive_values,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

By using a map[string]bool here I think we reduce the granularity of sensitivity when compared to the precise level used in change representation (which uses a json.RawMessage for BeforeSensitive/AfterSensitive). This means that for the following config:

resource "null_resource" "none" {
  triggers = {
    boop = "beep"
    flap = sensitive("honk")
  }
}

the sensitive_values output would be {"triggers":true} rather than {"triggers":{"flap":true}}. As a result, a state renderer would have to hide the entire triggers attribute, rather than just the one sensitive member.

This is compatible with the JSON plan sensitivity output, just more coarse. Is that necessary or could we consider keeping the full sensitivity information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 you're absolutely correct, that ain't right, I shall fix that.

…ions

A sensitive_values field has been added to the resource in state and planned values which is a map of all sensitive attributes with the values set to true.

It wasn't entirely clear to me if the values in state would suffice, or if we also need to consult the schema - I believe that this is sufficient for state files written since v0.15, and if that's incorrect or insufficient, I'll add in the provider schema check as well.

I also updated the documentation, and, since we've considered this before, bumped the FormatVersions for both jsonstate and jsonplan.
@mildwonkey mildwonkey force-pushed the mildwonkey/show-state-sensitive branch from dc5de76 to f18e035 Compare June 11, 2021 13:58
@mildwonkey mildwonkey merged commit ac03d35 into main Jun 14, 2021
mildwonkey added a commit that referenced this pull request Jun 14, 2021
…ions (#28889)

* jsonplan and jsonstate: include sensitive_values in state representations

A sensitive_values field has been added to the resource in state and planned values which is a map of all sensitive attributes with the values set to true.

It wasn't entirely clear to me if the values in state would suffice, or if we also need to consult the schema - I believe that this is sufficient for state files written since v0.15, and if that's incorrect or insufficient, I'll add in the provider schema check as well.

I also updated the documentation, and, since we've considered this before, bumped the FormatVersions for both jsonstate and jsonplan.
alisdair pushed a commit that referenced this pull request Jun 18, 2021
…ions (#28889)

* jsonplan and jsonstate: include sensitive_values in state representations

A sensitive_values field has been added to the resource in state and planned values which is a map of all sensitive attributes with the values set to true.

It wasn't entirely clear to me if the values in state would suffice, or if we also need to consult the schema - I believe that this is sufficient for state files written since v0.15, and if that's incorrect or insufficient, I'll add in the provider schema check as well.

I also updated the documentation, and, since we've considered this before, bumped the FormatVersions for both jsonstate and jsonplan.
@mildwonkey mildwonkey deleted the mildwonkey/show-state-sensitive branch June 21, 2021 14:54
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants