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

Provider property allowedAccountIDs are interpreted as numbers in v6 #2806

Closed
ringods opened this issue Sep 13, 2023 · 7 comments · Fixed by #2817
Closed

Provider property allowedAccountIDs are interpreted as numbers in v6 #2806

ringods opened this issue Sep 13, 2023 · 7 comments · Fixed by #2817
Assignees
Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Milestone

Comments

@ringods
Copy link
Member

ringods commented Sep 13, 2023

What happened?

Customer reported they have upgraded the provider to 6.0.x and we are seeing this error:

Diagnostics: pulumi:providers:aws (default_6_0_4): error: 
rpc error: code = Unknown desc = cannot encode provider configuration to call ValidateProviderConfig: 
objectEncoder failed on property "allowed_account_ids": 
set encoder failed: encoding element 0 ({5.30YYYYYYYYYe+11}): Expected a string, got: {5.30YYYYYYYYYe+11}

I wrapped this to multiple lines for readability and redacted the real account ID with Y characters.

Expected Behavior

Same behaviour as in v5.

Steps to reproduce

Start with v5 and this as working stack config:

config:
  aws:allowedAccountIds:
    - 0530YYYYYYYY

Upgrade the provider to v6 and the error appears.

Output of pulumi about

CLI          
Version      3.82.1
Go Version   go1.21.0
Go Compiler  gc

Plugins
NAME    VERSION
aws     6.0.4
nodejs  unknown

Host     
OS       darwin
Version  13.5.2
Arch     arm64

This project is written in nodejs: executable='/Users/ringods/.volta/bin/node' version='v16.19.1'

Current Stack: team-ce/zendesk-4011/ringo

Found no resources associated with team-ce/ringo

Found no pending operations associated with team-ce/ringo

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/v-ringo-pulumi-corp
User           v-ringo-pulumi-corp
Organizations  v-ringo-pulumi-corp, team-ce, pulumi

Dependencies:
NAME            VERSION
@pulumi/pulumi  3.82.1
@types/node     16.18.50
@pulumi/aws     6.0.4

Pulumi locates its logs in /var/folders/yq/10j1hf8s1ks9f23yxrdbbbb40000gn/T/ by default

Additional context

Workaround is to quote the values, so they are definitely interpreted as strings:

config:
  aws:allowedAccountIds:
    - "0530YYYYYYYY"

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@ringods ringods added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Sep 13, 2023
@mikhailshilkov mikhailshilkov added p1 A bug severe enough to be the next item assigned to an engineer impact/regression Something that used to work, but is now broken and removed needs-triage Needs attention from the triage team labels Sep 13, 2023
@mikhailshilkov
Copy link
Member

This seems to be coming from our TFPF integration which is indeed new in v6: https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pf/tfbridge/provider_checkconfig.go#L105

cc @t0yv0 as our TFPF master

@mikhailshilkov mikhailshilkov added this to the 0.94 milestone Sep 13, 2023
@ringods
Copy link
Member Author

ringods commented Sep 13, 2023

I didn't check explicitely, but a similar issue might exist for forbiddenAccountIds.

@mikhailshilkov mikhailshilkov assigned iwahbe and unassigned t0yv0 Sep 13, 2023
@iwahbe
Copy link
Member

iwahbe commented Sep 14, 2023

The following program is sufficient to reproduce the problem:

name: repro
runtime: yaml
resources:
  aws:
    type: pulumi:providers:aws
    properties:
      allowedAccountIds:
        - 6YYYYYYYYYYY
      region: us-west-2

I got the same error message as reported.

@mikhailshilkov
Copy link
Member

Is it literally 6YYYYYYYYYYY or some number like 601234567890?

@iwahbe
Copy link
Member

iwahbe commented Sep 15, 2023

It's a number, yaml parses it as a number. The problem is that our SDKv2 based upstream would coerce the number to a string while the PF based upstream won't.

iwahbe added a commit to pulumi/pulumi-terraform-bridge that referenced this issue Sep 15, 2023
This is necessary for backwards compatibility with SDKv{1,2} based
provider. It is
dangerous; there are many ways to represent bools and numbers so we are
unable to round trip.

This is the bridge side fix for
pulumi/pulumi-aws#2806.
@t0yv0
Copy link
Member

t0yv0 commented Sep 15, 2023

Thanks for that comment and just to make sure the coercion to string does not produce this:

"5.30YYYYYYYYYe+11"

right? But rather "0530YYYYYYYY"?

@iwahbe
Copy link
Member

iwahbe commented Sep 15, 2023

Good catch. I tested that we coerce without error in AWS, but it turns out that AWS doesn't validate these numbers on preview.

Fixed in pulumi/pulumi-terraform-bridge#1371.

t0yv0 added a commit that referenced this issue Sep 18, 2023
Fixes #2812
Fixes #2806

We could not use upgrade-provider for this operation because there was a
conflict between upstream changes editing a go.sum and one of our
patches doing the same, so the patch did not apply cleanly. Resolved
manually by rebating upstream and rebuilding the patch set.

The new upstream version targeted here is 5.17.0.

On top of the upstream upgrade, this PR updates dependencies from
pulumi-terraform-bridge to propagate bugfixes:

- github.com/pulumi/pulumi-terraform-bridge/pf v0.16.1
- github.com/pulumi/pulumi-terraform-bridge/v3 v3.60.0
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Sep 18, 2023
@t0yv0 t0yv0 removed their assignment Sep 18, 2023
mjeffryes pushed a commit to pulumi/pulumi-terraform-bridge that referenced this issue Oct 2, 2023
This is necessary for backwards compatibility with SDKv{1,2} based
provider. It is
dangerous; there are many ways to represent bools and numbers so we are
unable to round trip.

This is the bridge side fix for
pulumi/pulumi-aws#2806.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants