From 56b0008c8e14bdcea87b79e020063ec2573b9ce1 Mon Sep 17 00:00:00 2001 From: Bryce Lampe Date: Fri, 9 Feb 2024 10:19:21 -0800 Subject: [PATCH 1/2] Skip updates when provider auth changes --- examples/examples_nodejs_test.go | 8 +-- .../ts/index.ts | 50 +++++++++----- provider/hybrid.go | 3 +- provider/provider.go | 49 +++++++++---- provider/provider_test.go | 69 +++++++++++++++++++ 5 files changed, 144 insertions(+), 35 deletions(-) diff --git a/examples/examples_nodejs_test.go b/examples/examples_nodejs_test.go index 705cc2ee..d0dc85b4 100644 --- a/examples/examples_nodejs_test.go +++ b/examples/examples_nodejs_test.go @@ -192,10 +192,10 @@ func TestSecretsInExplicitProviderNode(t *testing.T) { }) } test := getJsOptions(t).With(integration.ProgramTestOptions{ - Dir: path.Join(getCwd(t), "test-secrets-in-explicit-provider", "ts"), - Quick: true, - SkipRefresh: true, - ExtraRuntimeValidation: check, + Dir: path.Join(getCwd(t), "test-secrets-in-explicit-provider", "ts"), + AllowEmptyPreviewChanges: false, + SkipEmptyPreviewUpdate: false, + ExtraRuntimeValidation: check, }) integration.ProgramTest(t, &test) } diff --git a/examples/test-secrets-in-explicit-provider/ts/index.ts b/examples/test-secrets-in-explicit-provider/ts/index.ts index 044ac18e..68ee5755 100644 --- a/examples/test-secrets-in-explicit-provider/ts/index.ts +++ b/examples/test-secrets-in-explicit-provider/ts/index.ts @@ -2,32 +2,48 @@ import * as pulumi from "@pulumi/pulumi"; import * as docker from "@pulumi/docker"; import * as random from "@pulumi/random"; -const providerWithSecretAddress = new docker.Provider("provider-with-sensitive-address", { - registryAuth: [{ +const providerWithSecretAddress = new docker.Provider( + "provider-with-sensitive-address", + { + registryAuth: [ + { address: pulumi.secret("secret-address"), username: "some-user", - }], -}) + }, + ], + } +); const passwordResource = new random.RandomPassword("random", { - length: 16, - special: false, + length: 16, + special: false, }); -const providerWithSecretUsername = new docker.Provider("provider-with-sensitive-username", { - registryAuth: [{ +const providerWithSecretUsername = new docker.Provider( + "provider-with-sensitive-username", + { + registryAuth: [ + { address: "some-address", username: passwordResource.result, - }], -}) + }, + ], + } +); -const providerWithSecretPassword = new docker.Provider("provider-with-password", { - registryAuth: [{ +const providerWithSecretPassword = new docker.Provider( + "provider-with-password", + { + registryAuth: [ + { address: "some-address", username: "some-user", - password: "secret-password", - }], -}) + password: "secret-password-" + Math.random().toString(36).slice(2, 7), + }, + ], + } +); -export const password = pulumi.unsecret(passwordResource.result) - .apply(x => Buffer.from(x).toString('base64')); +export const password = pulumi + .unsecret(passwordResource.result) + .apply((x) => Buffer.from(x).toString("base64")); diff --git a/provider/hybrid.go b/provider/hybrid.go index 2219a695..adc8119e 100644 --- a/provider/hybrid.go +++ b/provider/hybrid.go @@ -65,8 +65,7 @@ func (dp dockerHybridProvider) CheckConfig(ctx context.Context, request *rpc.Che } func (dp dockerHybridProvider) DiffConfig(ctx context.Context, request *rpc.DiffRequest) (*rpc.DiffResponse, error) { - // Delegate to the bridged provider, as native Provider does not implement it. - return dp.bridgedProvider.DiffConfig(ctx, request) + return dp.nativeProvider.DiffConfig(ctx, request) } func (dp dockerHybridProvider) Configure( diff --git a/provider/provider.go b/provider/provider.go index edfbafd8..c8abf9c2 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -73,7 +73,7 @@ func (p *dockerNativeProvider) CheckConfig(ctx context.Context, req *rpc.CheckRe // DiffConfig diffs the configuration for this provider. func (p *dockerNativeProvider) DiffConfig(ctx context.Context, req *rpc.DiffRequest) (*rpc.DiffResponse, error) { - return &rpc.DiffResponse{}, nil + return p.Diff(ctx, req) } // Configure configures the resource provider with "globals" that control its behavior. @@ -338,27 +338,50 @@ func (p *dockerNativeProvider) Diff(ctx context.Context, req *rpc.DiffRequest) ( func diffUpdates(updates map[resource.PropertyKey]resource.ValueDiff) map[string]*rpc.PropertyDiff { updateDiff := map[string]*rpc.PropertyDiff{} - for key, valueDiff := range updates { - update := true - if string(key) == "registry" && valueDiff.Object != nil { - // only register a diff on "server" field, but not on "username" or "password", - // as they can change frequently and should not trigger a rebuild. - _, update = valueDiff.Object.Updates["server"] + for key, valueDiff := range updates { + // Include all the same updates by default. + updateDiff[string(key)] = &rpc.PropertyDiff{ + Kind: rpc.PropertyDiff_UPDATE, } - if update { - updateDiff[string(key)] = &rpc.PropertyDiff{ - Kind: rpc.PropertyDiff_UPDATE, + // only register a diff on "server" field (or "address" in the case + // of provider config), but not on "username" or "password", as + // they can change frequently and should not trigger a rebuild. + if !(string(key) == "registry" || string(key) == "registryAuth") { + continue + } + keep := false + updates := []*resource.ObjectDiff{} + if valueDiff.Object != nil { + // Resource config. + updates = append(updates, valueDiff.Object) + } else if valueDiff.Array != nil { + // Provider config. + for _, u := range valueDiff.Array.Updates { + updates = append(updates, u.Object) + } + } + // Check each modified resource for server/address changes. If we don't + // find any, don't mark this property for update. + for _, u := range updates { + _, serverUpdate := u.Updates["server"] + _, addressUpdate := u.Updates["address"] + if serverUpdate || addressUpdate || u.Updates == nil { + keep = true + break } } + if !keep { + delete(updateDiff, string(key)) + } } + return updateDiff } // Create allocates a new instance of the provided resource and returns its unique ID afterwards. func (p *dockerNativeProvider) Create(ctx context.Context, req *rpc.CreateRequest) (*rpc.CreateResponse, error) { - urn := resource.URN(req.GetUrn()) label := fmt.Sprintf("%s.Create(%s)", p.name, urn) logging.V(9).Infof("%s executing", label) @@ -548,7 +571,9 @@ func parseCheckpointObject(obj resource.PropertyMap) resource.PropertyMap { } - return nil + // If the map doesn't include __inputs then it already represents its + // inputs, as is the case with provider config. + return obj } type contextHashAccumulator struct { diff --git a/provider/provider_test.go b/provider/provider_test.go index abaf19e1..abee4598 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -87,6 +87,75 @@ func TestDiffUpdates(t *testing.T) { assert.Equal(t, expected, actual) }) + t.Run("DiffConfig happens on changed address name", func(t *testing.T) { + expected := map[string]*rpc.PropertyDiff{ + "registryAuth": { + Kind: rpc.PropertyDiff_UPDATE, + }, + } + input := map[resource.PropertyKey]resource.ValueDiff{ + "registryAuth": { + Array: &resource.ArrayDiff{ + Updates: map[int]resource.ValueDiff{ + 0: { + Object: &resource.ObjectDiff{ + Updates: map[resource.PropertyKey]resource.ValueDiff{ + "address": { + Old: resource.PropertyValue{ + V: "dockerhub", + }, + New: resource.PropertyValue{ + V: "ShinyPrivateGHCR", + }, + }, + }, + }, + }, + }, + }, + }, + } + actual := diffUpdates(input) + assert.Equal(t, expected, actual) + }) + + t.Run("No DiffConfig happens on changed password", func(t *testing.T) { + expected := map[string]*rpc.PropertyDiff{} + input := map[resource.PropertyKey]resource.ValueDiff{ + "registryAuth": { + Array: &resource.ArrayDiff{ + Updates: map[int]resource.ValueDiff{ + 0: { + Object: &resource.ObjectDiff{ + Updates: map[resource.PropertyKey]resource.ValueDiff{ + "password": { + Old: resource.PropertyValue{ + V: "platypus", + }, + New: resource.PropertyValue{ + V: "Schnabeltier", + }, + }, + }, + }, + }, + }, + }, + }, + } + actual := diffUpdates(input) + assert.Equal(t, expected, actual) + }) + + t.Run("No DiffConfig happens on no changes", func(t *testing.T) { + expected := map[string]*rpc.PropertyDiff{} + input := map[resource.PropertyKey]resource.ValueDiff{ + "registryAuth": {}, + } + actual := diffUpdates(input) + assert.Equal(t, expected, actual) + }) + t.Run("Diff happens on unknown new registry", func(t *testing.T) { expected := map[string]*rpc.PropertyDiff{ "registry": { From 827844a701ff055bc052737d0baa12d9104db64d Mon Sep 17 00:00:00 2001 From: Bryce Lampe Date: Fri, 9 Feb 2024 11:40:52 -0800 Subject: [PATCH 2/2] handle no-op and other diff types --- provider/provider.go | 2 ++ provider/provider_test.go | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/provider/provider.go b/provider/provider.go index c8abf9c2..96263084 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -361,6 +361,8 @@ func diffUpdates(updates map[resource.PropertyKey]resource.ValueDiff) map[string for _, u := range valueDiff.Array.Updates { updates = append(updates, u.Object) } + } else if valueDiff.Old.Diff(valueDiff.New) != nil { + continue } // Check each modified resource for server/address changes. If we don't // find any, don't mark this property for update. diff --git a/provider/provider_test.go b/provider/provider_test.go index abee4598..381bf5f3 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -150,7 +150,10 @@ func TestDiffUpdates(t *testing.T) { t.Run("No DiffConfig happens on no changes", func(t *testing.T) { expected := map[string]*rpc.PropertyDiff{} input := map[resource.PropertyKey]resource.ValueDiff{ - "registryAuth": {}, + "registryAuth": { + Old: resource.NewObjectProperty(resource.PropertyMap{}), + New: resource.NewObjectProperty(resource.PropertyMap{}), + }, } actual := diffUpdates(input) assert.Equal(t, expected, actual)