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 import resources with provider default tags #4169

Merged
merged 18 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
135 changes: 135 additions & 0 deletions provider/provider_yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,19 @@ import (
"runtime"
"strings"
"testing"
"time"

"github.com/aws/aws-sdk-go-v2/config"
s3sdk "github.com/aws/aws-sdk-go-v2/service/s3"
"github.com/aws/aws-sdk-go-v2/service/s3/types"
"github.com/pulumi/providertest/pulumitest"
"github.com/pulumi/providertest/pulumitest/assertpreview"
"github.com/pulumi/providertest/pulumitest/opttest"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optrefresh"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optup"
"github.com/pulumi/pulumi/sdk/v3/go/common/apitype"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -382,6 +387,127 @@ resources:
})
}

// TestDefaultTagsImport tests the scenario where `tagsAll` and `tags` both
// exist and the user is importing a resource.
func TestDefaultTagsImport(t *testing.T) {
if testing.Short() {
t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without credentials")
}

bucketName := fmt.Sprintf("mybucket-%d", time.Now().UnixNano())
file1 := `
name: default-tags
runtime: yaml
resources:
awsProvider:
type: pulumi:providers:aws
properties:
defaultTags:
tags:
Project: x
Environment: dev
myBucket:
type: aws:s3:BucketV2
properties:
bucket: %s
tags:
CreatedBy: pulumi-aws
options:
provider: ${awsProvider}
retainOnDelete: %s
%s
outputs:
bucketName: ${myBucket.id}
`
workdir := t.TempDir()
cwd, err := os.Getwd()
assert.NoError(t, err)

first := fmt.Sprintf(file1, bucketName, "true", "")
err = os.WriteFile(filepath.Join(workdir, "Pulumi.yaml"), []byte(first), 0o600)
require.NoError(t, err)

ptest := pulumitest.NewPulumiTest(t, workdir,
opttest.SkipInstall(),
opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin")),
opttest.TestInPlace(),
)
ptest.Up()
tags := getBucketTagging(ptest.Context(), bucketName)
assert.Subset(t, tags, []types.Tag{
{
Key: pulumi.StringRef("Project"),
Value: pulumi.StringRef("x"),
},
{
Key: pulumi.StringRef("Environment"),
Value: pulumi.StringRef("dev"),
},
{
Key: pulumi.StringRef("CreatedBy"),
Value: pulumi.StringRef("pulumi-aws"),
},
})

// destroy with retainOnDelete: true
ptest.Destroy()

err = os.WriteFile(
filepath.Join(workdir, "Pulumi.yaml"),
[]byte(fmt.Sprintf(file1, bucketName, "false", fmt.Sprintf(" import: %s", bucketName))),
0o600,
)
require.NoError(t, err)

// up with an import
importResult := ptest.Up(optup.Diff())

changes := *importResult.Summary.ResourceChanges
assert.Equal(t, 1, changes["import"])
assert.Equal(t, importResult.Summary.Result, "succeeded")
}

func TestRegress4080(t *testing.T) {
if testing.Short() {
t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without credentials")
}

file1 := `
name: test-aws-1655-pf
runtime: yaml
description: |
Initial deployment without tags
resources:
app:
type: aws:appconfig:Application
aws-provider:
type: pulumi:providers:aws
res:
options:
provider: ${aws-provider}
properties:
applicationId: ${app.id}
type: aws:appconfig:Environment
`
workdir := t.TempDir()
cwd, err := os.Getwd()
assert.NoError(t, err)

err = os.WriteFile(filepath.Join(workdir, "Pulumi.yaml"), []byte(file1), 0o600)
require.NoError(t, err)

ptest := pulumitest.NewPulumiTest(t, workdir,
opttest.SkipInstall(),
opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin")),
opttest.TestInPlace(),
)
ptest.Up()

res := ptest.Refresh(optrefresh.ExpectNoChanges())
fmt.Printf("stdout: %s", res.StdOut)
fmt.Printf("stderr: %s", res.StdErr)
}

// Make sure that legacy Bucket supports deleting tags out of band and detecting drift.
func TestRegress3674(t *testing.T) {
ptest := pulumiTest(t, filepath.Join("test-programs", "regress-3674"), opttest.SkipInstall())
Expand Down Expand Up @@ -472,6 +598,15 @@ func configureS3() *s3sdk.Client {
return s3sdk.NewFromConfig(cfg)
}

func getBucketTagging(ctx context.Context, awsBucket string) []types.Tag {
s3 := configureS3()
tagging, err := s3.GetBucketTagging(ctx, &s3sdk.GetBucketTaggingInput{
Bucket: &awsBucket,
})
contract.AssertNoErrorf(err, "failed to get bucket tagging")
return tagging.TagSet
}

func deleteBucketTagging(ctx context.Context, awsBucket string) {
s3 := configureS3()
_, err := s3.DeleteBucketTagging(ctx, &s3sdk.DeleteBucketTaggingInput{
Expand Down
13 changes: 13 additions & 0 deletions provider/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -5945,6 +5945,19 @@ compatibility shim in favor of the new "name" field.`)
prov.Resources[key].PreCheckCallback = applyTags
}

// also override read so that it works during import
if transform := prov.Resources[key].TransformOutputs; transform != nil {
prov.Resources[key].TransformOutputs = func(ctx context.Context, pm resource.PropertyMap) (resource.PropertyMap, error) {
config, err := transform(ctx, pm)
if err != nil {
return nil, err
}
return applyTagsOutputs(ctx, config)
}
} else {
prov.Resources[key].TransformOutputs = applyTagsOutputs
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that TransformOutputs affects Create and Update, not just read - is this intended here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a side effect, but as long as it is true that tags and defaultTags should always be equal then I think we are fine.

}

if prov.Resources[key].GetFields() == nil {
prov.Resources[key].Fields = map[string]*tfbridge.SchemaInfo{}
}
Expand Down
34 changes: 34 additions & 0 deletions provider/tags.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,40 @@ func applyTags(
}
if allTags.IsNull() {
delete(ret, "tags")
delete(ret, "tagsAll")
corymhall marked this conversation as resolved.
Show resolved Hide resolved
return ret, nil
}
ret["tags"] = allTags
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set allTags to tags? Shouldn't tags only include the tags specified on the resource?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was also confused here, I was expecting something like this:

  • (1) we receive data from the Read call that has "tags" and "tags_all"

  • (2) I think here we need to understand what this data is and do we trust it; e.g. the TF provider would be grabbing all tags from the AWS hopefully, but then it would also possibly applying the defaultTags to that result to either union or exclude them? We had to disable this behavior for Create/Update lifecycle since we couldn't get the implementation to behave correctly and trust it; perhaps something similar needs to be tweaked here for reads

  • (3) suppose we figured out (2) correctly and we obtained what we believe is the set of in-AWS resource tags. Now what do we decide to surface as "tags". Perhaps here we want to exclude surfacing a k:v pair in "tags" if the defaultTags already has it set at provider level. So perhaps take the set difference here?

Unfortunately it's more complicated than this as TransformOutputs hook is hiding some differences around the context this code is executing in. Let me add below.

ret["tagsAll"] = allTags

return ret, nil
}

// similar to applyTags, but applied to the `TransformOutputs` method that is run
// during Read
func applyTagsOutputs(
ctx context.Context, config resource.PropertyMap,
) (resource.PropertyMap, error) {
ret := config.Copy()
configTags := resource.NewObjectProperty(resource.PropertyMap{})
if t, ok := config["tags"]; ok {
configTags = t
}

meta := resource.PropertyMap{}
if at, ok := config["tagsAll"]; ok {
meta["defaultTags"] = resource.NewObjectProperty(resource.PropertyMap{
"tags": at,
})
}

allTags, err := mergeTags(ctx, configTags, meta)
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate a little bit of elaboration here on what we're trying to do, and how does it work, I see this:

As written, allTags = mergeTags(ctx, config.tags, {defaultTags: config.tagsAll}).

I think if I recall what TransformOutputs is doing this will be assigning config to the tags and tagsAll coming out of the TF provider. This seems to then ignore the current defaultTags on the provider itself as it were.

Why does this work? I thought perhaps we're not trusting the tags_all output from the provider internals, is it useful here?

if err != nil {
return nil, err
}
if allTags.IsNull() {
delete(ret, "tags")
delete(ret, "tagsAll")
return ret, nil
}
ret["tags"] = allTags
Expand Down
Loading
Loading