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: don't reveal nested attributes with sensitive schema #32004

Merged
merged 2 commits into from Nov 2, 2022

Conversation

brandonc
Copy link
Contributor

@brandonc brandonc commented Oct 12, 2022

Description

Fixes several bugs in the plan diff rendering of nested attributes

  • When a schema attribute is sensitive, but also contains nested attributes, terraform reveals the nested attributes. The renderer checks for sensitive values at this nesting level, but ignores sensitive schema.
  • There is a mixture of "(sensitive)" and "(sensitive value)" output in the plan diff and I changed them all to "(sensitive value)"
  • Certain nested attribute modes don't output consistent new values for delete plans or unknown values

I also added a lot more test coverage for single nested attribute mode and sensitive schema nested attributes

Sample Config
terraform {
  required_providers {
    nested = {
      source = "alisdair/nested"
    }
  }
}

resource "nested_single" "example" {
  name = "my_nested_single"

  sensitive_value = {
    string = "secret"
  }
}

resource "nested_list" "example" {
  name = "my_nested_list"

  sensitive_values = [{
    string = "secret"
  }]
}

resource "nested_blocks" "example" {
  name = "my_nested_list"

  list {
    sensitive_value = {
      string = "secret"
    }
  }
}

Before:

Screen Shot 2022-10-12 at 3 02 10 PM

After:

Screen Shot 2022-10-21 at 9 40 56 AM

Target Release

1.4.0, 1.3.5

Draft CHANGELOG entry

BUG FIXES

When rendering a diff of nested attributes, Terraform now hides the entire nested object if it is marked sensitive by the provider schema

Copy link
Contributor

@Uk1288 Uk1288 left a comment

Choose a reason for hiding this comment

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

🎉 good work! I was able to smoke test this with deep nested resources as well.

internal/command/format/diff.go Outdated Show resolved Hide resolved
internal/command/format/diff.go Outdated Show resolved Hide resolved
@brandonc
Copy link
Contributor Author

brandonc commented Oct 24, 2022

@jbardin Hey, James, I revisited this change and fixed several inconsistencies with nested attributes, as well as standardized the (sensitive value) output in the plan diff. I also modified the PR description accordingly.

I can't see what's going on with the vercel deployment, but there were some minor documentation changes to reflect the (sensitive value) output.

Previously, there was mixed usage of "(sensitive)" and "(sensitive value)" and even though it was more common to see "(sensitive)", the thought is that it's a value we are hiding rather than describing something already shown.
@brandonc brandonc added the 1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Nov 2, 2022
@brandonc brandonc merged commit be5984d into main Nov 2, 2022
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

github-actions bot commented Dec 3, 2022

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 Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.3-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants