Prototype: Add the ability to ignore changes to volatile provider configuration (WIP) #1149
Conversation
The password and username of the registryAuth config can be volatile for many cloud registries. For AWS ECR you need to exchange AWS credentials for short lived tokens to authenticate to the registry. This will lead to perma-diffs in the provider config. This change adds the ability to ignore certain volatile fields of the registryAuth configuration.
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
flostadler
changed the title
| skipPush: true | ||
| }); | ||
|
|
||
| const ecrProvider = new docker.Provider("ecr-provider", { |
There was a problem hiding this comment.
Here we had a permadiff before.
| var ignoreRegistryAuthChanges bool = true | ||
| if v := news["ignoreRegistryAuthChanges"]; v.HasValue() && v.IsBool() { | ||
| ignoreRegistryAuthChanges = v.BoolValue() | ||
| } |
There was a problem hiding this comment.
We really should do type validation here. If the flag is present but not a bool (or a marker type), that should be a check failure.
If it is a marker type, say an unknown, what is the correct behavior? Can we mark this value as plain?
There was a problem hiding this comment.
I don't think we need to gate this behind provider config, FWIW. It's a bug fix IMO.
There was a problem hiding this comment.
Maybe I was a bit overcautious with that idea. My reasoning was that it might be surprising for users that a change in provider config doesn't show an update.
But you're right, that's probably only a very small subset of users (e.g. folks configuring static credentials).
FWIW The image resource just ignores changes to username&password as well without some flag.
| } | ||
|
|
||
| // copyMapIgnoringKeys creates a shallow copy of the given map without the specified keys that should be ignored. | ||
| func copyMapIgnoringKeys(original map[string]interface{}, ignoreKeys map[string]bool) map[string]interface{} { |
There was a problem hiding this comment.
This makes it clear that we only care if a key is present.
| func copyMapIgnoringKeys(original map[string]interface{}, ignoreKeys map[string]bool) map[string]interface{} { | |
| func copyMapIgnoringKeys(original map[string]interface{}, ignoredKeys map[string]struct{}) map[string]interface{} { |
| err := json.Unmarshal([]byte(olds["registryAuth"].StringValue()), &oldAuthData) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| err = json.Unmarshal([]byte(news["registryAuth"].StringValue()), &newAuthData) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Ugh... IIRC I marked #986 as blocked on pulumi/pulumi#15032 because I really, really didn't want to propagate this pattern of ingesting provider config as JSON blobs.
There was a problem hiding this comment.
Oh, I must've missed that PR when I had a look at the issue. Thanks for pointing me into that direction!
pulumi/pulumi#15032 definitely seems like the better approach here. With the yaml provider behaving differently then the others (TIL!) this would end up being quite messy with the serialized json..
Do we have plans for merging that in soon? We wanted to tackle pulumi/pulumi-awsx#1203 this iteration and that depends on not having this perma-diff. But it's not worth adding even more cruft just to get it out as soon as possible IMO
The password and username of the registryAuth config can be volatile for many cloud registries. For AWS ECR you need to exchange AWS credentials for short lived tokens to authenticate to the registry.
This will lead to perma-diffs in the provider config.
This change adds a prototype to explore whether it's viable to ignore certain volatile fields of the registryAuth configuration.
fixes #952