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

Preserve sensitivity when set at impossible levels. #819

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paddycarver
Copy link
Contributor

Right now, anything that is represented by a *schema.Schema can be set
as sensitive. This is problematic, as there are two things that we use
*schema.Schema to represent that can't actually be set to Sensitive.

First, a block is represented as a *schema.Schema with an Elem property
set to another *schema.Schema or a *schema.Resource. Blocks can't be set
to sensitive, however; they have no flag for it. Right now the SDK just
discards that flag, ignoring it. This commit changes that to have all
the attributes inside the block inherit its sensitivity. Attributes
can be sensitive, so this preserves the intent as best we can.

Second, sub-attributes, pieces of complex attributes, are also
represented as a *schema.Schema with an Elem property. These are rarer;
to create these, a *schema.Schema needs to have an Elem property, and
needs to be computed only (not optional). The SDK automatically converts
this to an attribute when in SchemaConfigModeAuto. The other way to
create these is to set a *schema.Schema with an Elem property to
SchemaConfigModeAttr manually. Any fields below that *schema.Schema are
then parts of a complex attribute, not attributes in their own right.
But Sensitive can only be set on attributes, not on parts of attributes.
In that situation, right now the SDK just discards the Sensitive flag.
This commit changes that behavior so that in this situation, if any part
of the attribute is marked as sensitive, the entire attribute becomes
sensitive, getting as close to the intent as we can.

This is intended to resolve #201.

Right now, anything that is represented by a *schema.Schema can be set
as sensitive. This is problematic, as there are two things that we use
*schema.Schema to represent that can't actually be set to Sensitive.

First, a block is represented as a *schema.Schema with an Elem property
set to another *schema.Schema or a *schema.Resource. Blocks can't be set
to sensitive, however; they have no flag for it. Right now the SDK just
discards that flag, ignoring it. This commit changes that to have all
the attributes inside the block inherit its sensitivity. Attributes
_can_ be sensitive, so this preserves the intent as best we can.

Second, _sub-attributes_, pieces of complex attributes, are also
represented as a *schema.Schema with an Elem property. These are rarer;
to create these, a *schema.Schema needs to have an Elem property, and
needs to be computed only (not optional). The SDK automatically converts
this to an attribute when in SchemaConfigModeAuto. The other way to
create these is to set a *schema.Schema with an Elem property to
SchemaConfigModeAttr manually. Any fields below that *schema.Schema are
then parts of a complex attribute, not attributes in their own right.
But Sensitive can only be set on attributes, not on parts of attributes.
In that situation, right now the SDK just discards the Sensitive flag.
This commit changes that behavior so that in this situation, if any part
of the attribute is marked as sensitive, the entire attribute becomes
sensitive, getting as close to the intent as we can.
@paddycarver
Copy link
Contributor Author

I honestly have no idea how we're going to test this, but we'll figure it out.

@bflad
Copy link
Member

bflad commented Oct 27, 2021

Some additional unit test cases in TestSchemaMapCoreConfigSchema could help as configschema implements the Sensitive field in its Attribute type. It won't necessarily represent real world functionality, but at least it should verify the lowest level expectations. Maybe creating a TestGetProviderSchema in grpc_provider_test.go, similar but pared down to https://github.com/hashicorp/terraform-plugin-framework/blob/d6d71f60fdcee50b68417b14920e8129451e2426/tfsdk/serve_test.go#L261-L294, could also be helpful to verify the protocol messages as a last resort.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

bflad added a commit to hashicorp/terraform-docs-common that referenced this pull request Jul 25, 2022
Reference: hashicorp/terraform-plugin-framework#214
Reference: hashicorp/terraform-plugin-sdk#819
Reference: hashicorp/terraform-plugin-sdk#201

As part of investigating some differences between sdk/v2 and framework, the individual sensitivity configuration offered by nested attributes in protocol version 6 is important to call out explicitly.
bflad added a commit to hashicorp/terraform-docs-common that referenced this pull request Jul 26, 2022
#76)

Reference: hashicorp/terraform-plugin-framework#214
Reference: hashicorp/terraform-plugin-sdk#819
Reference: hashicorp/terraform-plugin-sdk#201

As part of investigating some differences between sdk/v2 and framework, the individual sensitivity configuration offered by nested attributes in protocol version 6 is important to call out explicitly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sensitive properties of nested blocks are shown during a plan in v12.6
3 participants