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.

Sign up for GitHub

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

Normalize Read result for empty collections #2069

Closed
wants to merge 6 commits into from
Closed

Normalize Read result for empty collections #2069

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e3f3de
Normalize Read result for empty collections
t0yv0 Jun 6, 2024
fc0e64f
First pass
t0yv0 Jun 6, 2024
103ed4a
Scope to provider2.go
t0yv0 Jun 6, 2024
5385d82
Use logger
t0yv0 Jun 6, 2024
2044e8d
Remove fmt.Println
t0yv0 Jun 6, 2024
437997b
Avoid import cycle
t0yv0 Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions pkg/tfshim/sdk-v2/provider2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"

"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge"
shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)
Expand Down Expand Up @@ -254,13 +254,66 @@ func (p *planResourceChangeImpl) Refresh(
if rr.stateValue.IsNull() {
return nil, nil
}
normStateValue := p.normalizeNullValues(ctx, res, c, rr.stateValue)
return &v2InstanceState2{
resourceType: rr.resourceType,
stateValue: rr.stateValue,
stateValue: normStateValue,
meta: rr.meta,
}, nil
}

// Compensate for a TF problem causing spurious diffs between null and empty collections highlighted as dirty refresh in
// Pulumi. When applying resource changes TF will call normalizeNullValues with apply=true which may erase the
// distinction between empty and null collections. This makes state returned from Read differ from the state returned by
// Create and Update and written to the state store. While the problem is present in both TF and Pulumi bridged
// providers, it is worse in Pulumi because refresh is emitting a warning that something is changing.
//
// To compensate for this problem, this method corrects the Pulumi Read result during `pulumi refresh` to also erase
// empty collections from the Read result if the old input is missing or null.
//
// This currently only handles top-level properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

We likely need to handle this for every block, right

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like it, but I don't have an e2e example handy.

//
// See: https://github.com/pulumi/terraform-plugin-sdk/blob/upstream-v2.33.0/helper/schema/grpc_provider.go#L1514
func (p *planResourceChangeImpl) normalizeNullValues(
ctx context.Context,
res *schema.Resource,
config shim.ResourceConfig,
state cty.Value,
) cty.Value {
if config == nil {
return state
}
sm := res.SchemaMap()
m := state.AsValueMap()
for tfName, v := range m {
sch, ok := sm[tfName]
if !ok {
continue
}
t := sch.Type
isCollection := t == schema.TypeList || t == schema.TypeMap || t == schema.TypeSet
if !isCollection {
continue
}

if _, ok := sch.Elem.(*schema.Resource); ok || sch.ConfigMode == schema.SchemaConfigModeBlock {
Copy link
Member Author

Choose a reason for hiding this comment

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

What's the best way to decide if it's a block or attr these days @VenelinMartinov

Copy link
Contributor

Choose a reason for hiding this comment

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

You can get the attributes of a resource from res.CoreConfigSchema().Attributes

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like this:

	sch := res.CoreConfigSchema()
	for key, attr := range sch.Attributes {
		if !attr.Type.IsCollectionType() {
			continue
		}

		if config.IsSet(key) {
			continue
		}

		stateVal, ok := m[key]
		if !ok {
			continue
		}
		if !(stateVal.Type().IsCollectionType() && !stateVal.IsNull() && stateVal.LengthInt() == 0) {
			continue
		}

		m[key] = cty.NullVal(attr.Type)
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

I've continued this in #2065

continue
}
if config.IsSet(tfName) {
continue
}

if !(v.Type().IsCollectionType() && !v.IsNull() && v.LengthInt() == 0) {
continue
}

msg := fmt.Sprintf("normalizeNullValues: replacing %s=[] not found in config", tfName)
tfbridge.GetLogger(ctx).Debug(msg)
m[tfName] = cty.NullVal(v.Type())
}
return cty.ObjectVal(m)
}

func (p *planResourceChangeImpl) NewDestroyDiff(
ctx context.Context, t string, opts shim.TimeoutOptions,
) shim.InstanceDiff {
Expand Down
Loading