Ignore username/password changes in registryAuth config #1327

flostadler · 2025-01-03T12:06:37Z

This PR modifies how the Docker provider handles changes to registry authentication credentials. Previously, any changes to the registryAuth configuration would trigger resource updates. Now, changes to usernames and passwords are ignored while changes to registry addresses still trigger updates. This aligns the native docker.Image resource (which already ignored username/password changes) with the rest of the bridged resources.
To handle nested json-encoded configuration (related to pulumi/pulumi#17667) the provider is leveraging the UnfoldProperties method from the bridge. This way it can transparently operate on the config without having to worry about the json encoding.

Key changes:

Extended the native provider diff logic to handle provider configuration as well (Taken from [blocked] Skip updates when only provider auth changes #986). This maintains the existing behavior where registry address changes trigger updates
Added support for transparently handling JSON-encoded nested configuration. This is necessary in lieu of Permit providers to opt out of receiving JSON-encoded config pulumi#17667.
Added e2e tests for both raw and JSON-encoded credential changes

Fixes #952

Changes to registry authentication credentials (username/password) should not trigger resource updates, while changes to registry addresses should. This applies to both raw config and JSON-encoded nested configuration. Key changes: - Added logic to ignore username/password updates in registryAuth arrays - Added handling for JSON-encoded nested configuration - Added tests to verify credential changes don't trigger diffs - Maintained address changes as a trigger for updates

github-actions · 2025-01-03T12:08:01Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

codecov · 2025-01-03T12:08:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (a738187) to head (de3def7).

Additional details and impacted files

@@          Coverage Diff           @@
##           master   #1327   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           3       3           
  Lines         147     147           
======================================
  Misses        147     147

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

corymhall

LGTM!

t0yv0 · 2025-01-08T14:15:37Z

provider/hybrid.go

+		return nil, fmt.Errorf("error unwrapping new config: %w", err)
+	}
+
+	return dp.nativeProvider.DiffConfig(ctx, request)


Isn't this going to make it a no-op?

// DiffConfig diffs the configuration for this provider. func (p *dockerNativeProvider) DiffConfig(context.Context, *rpc.DiffRequest) (*rpc.DiffResponse, error) { return &rpc.DiffResponse{}, nil }

t0yv0 · 2025-01-08T14:16:01Z

provider/provider.go

-func (p *dockerNativeProvider) DiffConfig(context.Context, *rpc.DiffRequest) (*rpc.DiffResponse, error) {
-	return &rpc.DiffResponse{}, nil
+func (p *dockerNativeProvider) DiffConfig(ctx context.Context, req *rpc.DiffRequest) (*rpc.DiffResponse, error) {
+	return p.Diff(ctx, req)


t0yv0 · 2025-01-08T14:22:54Z

So to clarify.

Prior to this change, provider configuration was handled by the bridge. The bridge default logic caused unwanted behavior: changes to usernames and passwords triggered cascading updates. To address the problem, given difficulty in customizing the bridge behavior to what is wanted, DiffConfig method is re-implemented entirely to use a custom method.

t0yv0 · 2025-01-08T14:26:27Z

I'd like to double-check that the bridge could not be reconfigured to give the desired behavior here quickly.

Presumably this code was tripping up a force replace?

https://github.com/pulumi/pulumi-terraform-bridge/blob/e2b32971aeb4b9ff1483d27f46dd09a9d515bff3/pkg/tfbridge/provider.go#L824

t0yv0 · 2025-01-08T14:29:02Z

provider/provider_test.go

+				}),
+			},
+			want: &rpc.DiffResponse{
+				Changes: rpc.DiffResponse_DIFF_NONE,


Ah so to clarify you want to completely hide the changes? Displaying the changes without causing a cascading replacement is undesirable? Why? Does not the user expect to see changes reflected after they are made?

Those are settings that are likely to change on every run (e.g. for ECR). We’re already doing this for the Image resource. This change aligns the bridged resources with that.

without this, drift detection wouldn’t work for most major clouds

Gotcha so we want this as default behavior.

flostadler · 2025-01-08T14:30:16Z

I'd like to double-check that the bridge could not be reconfigured to give the desired behavior here quickly.

Presumably this code was tripping up a force replace?

https://github.com/pulumi/pulumi-terraform-bridge/blob/e2b32971aeb4b9ff1483d27f46dd09a9d515bff3/pkg/tfbridge/provider.go#L824

The issue is that the password and in turn the auth config block are marked as secret and that shows up as a replacement in the preview no matter what. We cannot un-secret the password because it’s sensitive.

t0yv0 · 2025-01-08T14:47:46Z

To be pedantic it seems to be an update and not a replacement:

Previewing update (dev)

View Live: https://app.pulumi.com/anton-pulumi-corp/docker-952/dev/previews/df71e2d0-8cd4-4f56-ac19-93588dd00b5f

  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:dev::docker-952::pulumi:pulumi:Stack::docker-952-dev]
    ~ pulumi:providers:docker: (update)
        [id=b633b4d6-1415-4599-b0f9-77b28f170d6f]
        [urn=urn:pulumi:dev::docker-952::pulumi:providers:docker::docker-provider]
        host        : "unix:///Users/anton/.colima/docker.sock"
        registryAuth: [secret]
        version     : "4.5.8"
Resources:
    ~ 1 to update
    1 unchanged

t0yv0 · 2025-01-08T15:07:34Z

All right given we want the new behavior, let's take the change, just let's talk a little bit about our options to keep the code a bit cleaner.

Would it be possible to not reuse the Diff method for Docker Image as the DiffConfig implementation for the provider? These have different schemas and different purposes and are just accidentally similar. I think sharing this code reduces duplication but creates dense code that's harder to understand. Could we have some extra code instead that's simpler?

If going down the path of implementing a custom diff method, one interesting hint can be seen here:

pulumi-docker/provider/provider.go

Line 304 in 962342e

d := oldInputs.Diff(news)

func (props PropertyMap) Diff(other PropertyMap, ignoreKeys ...IgnoreKeyFunc) *ObjectDiff {

Note the IgnoreKeyFunc perhaps it can take care of the ignored keys elegantly instead of post-processing ObjectDiff.

Another possibility is to keep using the bridge implementation but do pre-processing or ask it to ignore keys e.g.

func (dp dockerHybridProvider) DiffConfig(ctx context.Context, request *rpc.DiffRequest) (*rpc.DiffResponse, error) {
	urn := resource.URN(request.GetUrn())
	label := fmt.Sprintf("%s.DiffConfig(%s)", dp.name, urn)
	logging.V(9).Infof("%s executing", label)

	var err error
	request.Olds, err = dp.unwrapJSONConfig(label, request.GetOlds())
	if err != nil {
		return nil, fmt.Errorf("error unwrapping old config: %w", err)
	}
	request.News, err = dp.unwrapJSONConfig(label, request.GetNews())
	if err != nil {
		return nil, fmt.Errorf("error unwrapping new config: %w", err)
	}

	request.IgnoreChanges = []string{"registryAuth"}
	return dp.bridgedProvider.DiffConfig(ctx, request)
}

This might not work but may be worth a quick try.

There is also this https://github.com/pulumi/pulumi-terraform-bridge/blob/e2b32971aeb4b9ff1483d27f46dd09a9d515bff3/unstable/propertyvalue/ignore_changes.go#L24 that can pre-process the maps.

flostadler · 2025-01-08T15:07:39Z

To be pedantic it seems to be an update and not a replacement:

Previewing update (dev)

View Live: https://app.pulumi.com/anton-pulumi-corp/docker-952/dev/previews/df71e2d0-8cd4-4f56-ac19-93588dd00b5f

  pulumi:pulumi:Stack: (same)
    [urn=urn:pulumi:dev::docker-952::pulumi:pulumi:Stack::docker-952-dev]
    ~ pulumi:providers:docker: (update)
        [id=b633b4d6-1415-4599-b0f9-77b28f170d6f]
        [urn=urn:pulumi:dev::docker-952::pulumi:providers:docker::docker-provider]
        host        : "unix:///Users/anton/.colima/docker.sock"
        registryAuth: [secret]
        version     : "4.5.8"
Resources:
    ~ 1 to update
    1 unchanged

Sorry, yeah what I wanted to say (and the PR description has it correct) is that it shows as an update for the provider and updates for all dependent resources.

t0yv0 · 2025-01-08T15:10:41Z

provider/provider.go

@@ -339,20 +339,47 @@ func (p *dockerNativeProvider) Diff(_ context.Context, req *rpc.DiffRequest) (*r
 func diffUpdates(updates map[resource.PropertyKey]resource.ValueDiff) map[string]*rpc.PropertyDiff {
 	updateDiff := map[string]*rpc.PropertyDiff{}
 	for key, valueDiff := range updates {
-		update := true
+		// Include all the same updates by default.


Sharing the diffUpdates function for cross-purposes is the most dense bit in this PR I think - it might be correct but this is pretty non-obivous code. I'd love if we had a separate version of this function for provider config specifically or even better if we didn't have to use this function to post-process updates but got the updates to compute as we want them in the first place.

flostadler · 2025-01-08T15:12:18Z

All right given we want the new behavior, let's take the change, just let's talk a little bit about our options to keep the code a bit cleaner.

Would it be possible to not reuse the Diff method for Docker Image as the DiffConfig implementation for the provider? These have different schemas and different purposes and are just accidentally similar. I think sharing this code reduces duplication but creates dense code that's harder to understand. Could we have some extra code instead that's simpler?

If going down the path of implementing a custom diff method, one interesting hint can be seen here:

pulumi-docker/provider/provider.go

Line 304 in 962342e

d := oldInputs.Diff(news)
func (props PropertyMap) Diff(other PropertyMap, ignoreKeys ...IgnoreKeyFunc) *ObjectDiff {
Note the IgnoreKeyFunc perhaps it can take care of the ignored keys elegantly instead of post-processing ObjectDiff.

Another possibility is to keep using the bridge implementation but do pre-processing or ask it to ignore keys e.g.
func (dp dockerHybridProvider) DiffConfig(ctx context.Context, request *rpc.DiffRequest) (*rpc.DiffResponse, error) {
	urn := resource.URN(request.GetUrn())
	label := fmt.Sprintf("%s.DiffConfig(%s)", dp.name, urn)
	logging.V(9).Infof("%s executing", label)

	var err error
	request.Olds, err = dp.unwrapJSONConfig(label, request.GetOlds())
	if err != nil {
		return nil, fmt.Errorf("error unwrapping old config: %w", err)
	}
	request.News, err = dp.unwrapJSONConfig(label, request.GetNews())
	if err != nil {
		return nil, fmt.Errorf("error unwrapping new config: %w", err)
	}

	request.IgnoreChanges = []string{"registryAuth"}
	return dp.bridgedProvider.DiffConfig(ctx, request)
}
This might not work but may be worth a quick try.

There is also this https://github.com/pulumi/pulumi-terraform-bridge/blob/e2b32971aeb4b9ff1483d27f46dd09a9d515bff3/unstable/propertyvalue/ignore_changes.go#L24 that can pre-process the maps.

Yeah, more than happy to give these other options a try. I’m mostly worried that we end up with two very similar functions that basically do the same thing. But let me quickly prototype so we can compare it side by side

t0yv0 · 2025-01-08T15:14:18Z

provider/provider_test.go

+
+			got, err := p.Diff(context.TODO(), req)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("dockerNativeProvider.Diff() error = %v, wantErr %v", err, tt.wantErr)


This is a very confusing test suite we are testing dockerNativeProvider.Diff; the only job of dockerNativeProvider.Diff is to compute diffs for the Image resource. However we're testing it with registryAuth field that is not even part of the Image resource schema. I think we can keep the test suite but target

func (dp dockerHybridProvider) DiffConfig(ctx context.Context, request *rpc.DiffRequest) (*rpc.DiffResponse, error) {

Yeah, more reason to split out the methods!

t0yv0 · 2025-01-08T15:26:48Z

I’m mostly worried that we end up with two very similar functions that basically do the same thing

I think that's good in my book. It's only accidentally similar. There's generic code for doing diffs, and there's specific distinct jobs for diffing provider config vs Image resource config, which have different concrete schemas. The schemas will evolve separately in the future. Slightly duplicative code seems the way to go.

t0yv0 · 2025-01-08T15:29:41Z

Capturing some slack convo that helped me understand the trade-offs here.

This workaround does not do what the user wants because it persists password from state. This will get stale in 1hr or so after the token expires.

import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as docker from "@pulumi/docker";

const creds = aws.ecr.getAuthorizationTokenOutput({});

export const dockerProvider = new docker.Provider("docker-provider", {
  registryAuth: [
    {
      username: creds.apply((c) => c.userName),
      password: creds.apply((c) => c.password),
      address: creds.apply((c) => c.proxyEndpoint),
    },
  ],
}, {ignoreChanges: ["registryAuth"]});

Instead, what we get with this PR is that the password in Pulumi statefile will become obsolete, but the real password used by the provider will keep changing to be the latest issued token. This is because the changes scope ignoreChanges effect to DiffConfig but do not affect CheckConfig and Check lifecycle methods. This seems like an acceptable workaround.

flostadler · 2025-01-08T16:17:02Z

Capturing some slack convo that helped me understand the trade-offs here.

This workaround does not do what the user wants because it persists password from state. This will get stale in 1hr or so after the token expires.
import * as pulumi from "@pulumi/pulumi";
import * as aws from "@pulumi/aws";
import * as docker from "@pulumi/docker";

const creds = aws.ecr.getAuthorizationTokenOutput({});

export const dockerProvider = new docker.Provider("docker-provider", {
  registryAuth: [
    {
      username: creds.apply((c) => c.userName),
      password: creds.apply((c) => c.password),
      address: creds.apply((c) => c.proxyEndpoint),
    },
  ],
}, {ignoreChanges: ["registryAuth"]});
Instead, what we get with this PR is that the password in Pulumi statefile will become obsolete, but the real password used by the provider will keep changing to be the latest issued token. This is because the changes scope ignoreChanges effect to DiffConfig but do not affect CheckConfig and Check lifecycle methods. This seems like an acceptable workaround.

To clarify a bit more, the outputs of the provider in state will be frozen, but the input will still be updated. The input is what's passed to configure in the case of refresh/delete so this change doesn't break any of those workflows.

EronWright · 2025-01-08T20:03:03Z

Another approach to consider here, is to add support for configuring explicit providers using the stack configuration file. In this way, one could maintain the password as a stack configuration setting (which is updatable even for refresh/destroy), rather than as a Provider argument.

flostadler · 2025-01-08T20:56:47Z

Another approach to consider here, is to add support for configuring explicit providers using the stack configuration file. In this way, one could maintain the password as a stack configuration setting (which is updatable even for refresh/destroy), rather than as a Provider argument.

I think this would generally be a great idea! Sadly it wouldn’t solve this particular problem. The issue here mostly surfaces in cases where the provider is configured with short-lived credentials retrieved by a function (e.g. Amazon ECR).

flostadler · 2025-01-09T21:58:57Z

@t0yv0 it took a bit of experimentation, but I managed to get a version working that uses ignoreChanges and minimal post processing on the DiffConfig response!
This is minimally invasive and doesn't touch the native provider portion at all, PTAL :)

flostadler added 2 commits January 3, 2025 12:23

Add tests

3eac17e

flostadler requested review from blampe and a team January 3, 2025 12:06

flostadler self-assigned this Jan 3, 2025

Linting

a25db5c

flostadler marked this pull request as draft January 3, 2025 12:23

base64 encode secret output in test

f4dc4c8

flostadler marked this pull request as ready for review January 3, 2025 12:44

flostadler requested a review from t0yv0 January 6, 2025 14:03

Remove lock file from commit

962342e

flostadler requested a review from corymhall January 7, 2025 13:43

corymhall approved these changes Jan 7, 2025

View reviewed changes

t0yv0 reviewed Jan 8, 2025

View reviewed changes

flostadler added 2 commits January 9, 2025 22:48

Separate DiffConfig and Diff

ca0d8a9

Merge branch 'master' into flostadler/skip-provider-auth-updates

de3def7

flostadler requested review from corymhall and t0yv0 January 9, 2025 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore username/password changes in registryAuth config #1327

Ignore username/password changes in registryAuth config #1327

flostadler commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

codecov bot commented Jan 3, 2025 •

edited

Loading

corymhall left a comment

t0yv0 Jan 8, 2025

t0yv0 Jan 8, 2025

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

t0yv0 Jan 8, 2025

flostadler Jan 8, 2025

t0yv0 Jan 8, 2025

flostadler commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

flostadler commented Jan 8, 2025

t0yv0 Jan 8, 2025

flostadler commented Jan 8, 2025

t0yv0 Jan 8, 2025

flostadler Jan 8, 2025

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

flostadler commented Jan 8, 2025

EronWright commented Jan 8, 2025

flostadler commented Jan 8, 2025

flostadler commented Jan 9, 2025

Ignore username/password changes in registryAuth config #1327

Are you sure you want to change the base?

Ignore username/password changes in registryAuth config #1327

Conversation

flostadler commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

Does the PR have any schema changes?

codecov bot commented Jan 3, 2025 • edited Loading

Codecov Report

corymhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flostadler commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

flostadler commented Jan 8, 2025

Choose a reason for hiding this comment

flostadler commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Jan 8, 2025

t0yv0 commented Jan 8, 2025

flostadler commented Jan 8, 2025

EronWright commented Jan 8, 2025

flostadler commented Jan 8, 2025

flostadler commented Jan 9, 2025

codecov bot commented Jan 3, 2025 •

edited

Loading