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

[python] Pulumi Import Codegen Fails #2517

Closed
phillipedwards opened this issue May 8, 2023 · 8 comments
Closed

[python] Pulumi Import Codegen Fails #2517

phillipedwards opened this issue May 8, 2023 · 8 comments
Assignees
Labels
area/import An issue related to `pulumi import` or the import resource option. kind/bug Some behavior is incorrect or out of spec p1 Bugs severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed

Comments

@phillipedwards
Copy link
Member

What happened?

When running a pulumi import ... command, the importing of the resource succeeds, however, the codegen part fails with an obscure error.

error: anonymous.pp:24,22-25,14: cannot assign expression of type (null) to location of type   list(  { onDeregistration: output(string) | string, onUnhealthy: output(string) | string }
  | output({ onDeregistration: string, onUnhealthy: string }))
| output(list({ onDeregistration: string, onUnhealthy: string }))?: ; 

Expected Behavior

Pulumi import succeeds and correctly generates the required code definition.

Steps to reproduce

Create pulumi python program on pulumi and generate the appropriate code definition.

pulumi import ....

See error shown above.

Output of pulumi about

CLI          
Version      3.66.0
Go Version   go1.20.3
Go Compiler  gc

Plugins
NAME    VERSION
aws     5.40.0
python  unknown

Host     
OS       ubuntu
Version  22.04
Arch     x86_64

Additional context

No response

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).

@phillipedwards phillipedwards added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team area/import An issue related to `pulumi import` or the import resource option. labels May 8, 2023
@joeduffy joeduffy added the p1 Bugs severe enough to be the next item assigned to an engineer label May 9, 2023
@phillipedwards
Copy link
Member Author

Repro for issue:

  • Create a vanilla Pulumi python program pulumi new aws-python and pulumi up
  • Create an isolated AWS Target Group via the AWS Console; do not attach it to any load balancer (not sure if this impacts the codegen part)
  • Run pulumi import aws:lb/targetGroup:TargetGroup {your_name} {target_group_arn}

@Zaid-Ajaj Zaid-Ajaj self-assigned this May 10, 2023
@Zaid-Ajaj
Copy link

I've been able to reproduce the issue locally, looking into it now

@Zaid-Ajaj
Copy link

Found out the main root cause of the issue. Pulumi import reads the state from the AWS provider, then it writes out a PCL block of text from that state. Finally it gives the PCL text to one of our code generators to emit the final piece of code from that resource.

The problem here is that AWS provider is reading the state from the cloud incorrectly returning the following:

Resource input state
{
  "urn": "urn:pulumi:stack::project::aws:lb/targetGroup:TargetGroup::test-target-group",
  "custom": true,
  "type": "aws:lb/targetGroup:TargetGroup",
  "inputs": {
    "__defaults": [
      
    ],
    "deregistrationDelay": 300,
    "healthCheck": {
      "healthyThreshold": 5,
      "matcher": "200",
      "path": "/",
      "timeout": 5,
      "unhealthyThreshold": 2
    },
    "ipAddressType": "ipv4",
    "loadBalancingAlgorithmType": "round_robin",
    "loadBalancingCrossZoneEnabled": "use_load_balancer_configuration",
    "name": "test-target-group",
    "port": 80,
    "protocol": "HTTP",
    "protocolVersion": "HTTP1",
    "stickiness": {
      "enabled": false,
      "type": "lb_cookie"
    },
    "targetFailovers": [
      null
    ],
    "vpcId": "<redacted>"
  },
  "parent": "urn:pulumi:dev::pythonimportissue::pulumi:pulumi:Stack::pythonimportissue-dev",
  "protect": true,
  "provider": "<redacted>"
}

Notice here the input for property targetFailovers:

"targetFailovers": [
  null
]

has recorded a single element during Read but that element is null. This does not match with the schema of the property targetFailovers which is

array<{ 
  onDeregistration: string, 
  onUnhealthy: string 
}>

The code generators expect such elements to be non-nullable which is why the PCL binder fails when type-checking the resulting PCL text

Generated PCL text block
resource test-target-group "aws:lb/targetGroup:TargetGroup" {
    deregistrationDelay = 300
    healthCheck ={
        healthyThreshold = 5,
        matcher = "200",
        path = "/",
        timeout = 5,
        unhealthyThreshold = 2
    }
    ipAddressType = "ipv4"
    loadBalancingAlgorithmType = "round_robin"
    loadBalancingCrossZoneEnabled = "use_load_balancer_configuration"
    name = "test-target-group"
    port = 80
    protocol = "HTTP"
    protocolVersion = "HTTP1"
    stickiness ={
        enabled = false,
        type = "lb_cookie"
    }
    targetFailovers =[
        null]
    vpcId = "<redacted>"
    options {
      protect =true
    }
}

Will move the issue to pulumi/pulumi-aws since the root cause is in the implementation of Read in the AWS provider where it should return inputs for the resource state that match up with the schema of that resource.

@Zaid-Ajaj Zaid-Ajaj transferred this issue from pulumi/pulumi May 10, 2023
@Zaid-Ajaj Zaid-Ajaj removed their assignment May 10, 2023
@Zaid-Ajaj Zaid-Ajaj removed the needs-triage Needs attention from the triage team label May 10, 2023
@danielrbradley
Copy link
Member

Looking into the read implementation, the logic looks slightly questionable .. if there's no attributes at all then an empty array is returned. If neither target_failover.on_deregistration or target_failover.on_unhealthy are in the attribute set then an array with a single empty map will be returned. My first thought is that this empty map might be getting converted into the null within the bridge. Need to attach a debugger to follow this the rest of the way.

https://github.com/pulumi/terraform-provider-aws/blob/patched-v4.65.0/internal/service/elbv2/target_group.go#L1117C24-L1134

@t0yv0
Copy link
Member

t0yv0 commented May 10, 2023

Watching this, I'm available to investigate this as a potential bridge bug.

@t0yv0
Copy link
Member

t0yv0 commented May 10, 2023

Just some more info to help reproduce. This provisions a TargetGroup.

import pulumi
import pulumi_aws as aws

main = aws.ec2.Vpc("main", cidr_block="10.0.0.0/16")
test = aws.lb.TargetGroup("test",
    port=80,
    protocol="HTTP",
    vpc_id=main.id)

pulumi.export('arn', test.arn)

Then I can reproduce with:

pulumi import aws:lb/targetGroup:TargetGroup foo $(pulumi stack output arn)
error: anonymous.pp:27,22-28,14: cannot assign expression of type (null) to location of type   list(  { onDeregistration: output(string) | string, onUnhealthy: output(string) | string }
  | output({ onDeregistration: string, onUnhealthy: string }))
| output(list({ onDeregistration: string, onUnhealthy: string }))?: ; 

@t0yv0
Copy link
Member

t0yv0 commented May 11, 2023

Found that this line upstream generates a list with an empty map element:

https://github.com/pulumi/terraform-provider-aws/blob/d058da74480f4f9e2faa385eeca82db6915c98a7/internal/service/elbv2/target_group.go#L1133

There is an issue in the bridge that translates the empty map to null. Repro-as-unit-test is available:

pulumi/pulumi-terraform-bridge#1105

We have a potential fix that changes that but there's a lot of testing diligence to roll that out.

I recommend finding some workarounds in the meanwhile so we can reduce urgency from P1.

Perhaps import code could tolerate it somehow?

Could we patch upstream code to return an empty list?

@danielrbradley
Copy link
Member

To resolve the P1, I'll do a patch on the upstream implementation to remove the map if it's got zero elements.

danielrbradley added a commit to pulumi/terraform-provider-aws that referenced this issue May 12, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
danielrbradley added a commit that referenced this issue May 12, 2023
- Add patch to elbv2 target_group to fix #2517
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label May 15, 2023
rquitales pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jun 2, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
rquitales pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jun 27, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
rquitales pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jun 28, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jul 7, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jul 7, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jul 17, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Jul 24, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Aug 4, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Aug 9, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
iwahbe pushed a commit to pulumi/terraform-provider-aws that referenced this issue Aug 30, 2023
Workaround for pulumi/pulumi-aws#2517
Don't return single empty map. Return empty list instead.
t0yv0 added a commit that referenced this issue Jan 23, 2024
Fixes #3253

This PR upgrades the provider to match upstream provider version
v5.32.0.

List of unusual things that defeated upgrade-provider and required
manual intervention:

- The patch for elbv2 target workaround in #2517 does not apply cleanly
to new upstream, had to be redone; I've tested manually that 2517 does
not reproduce on top of the new provider, but need to backlog adding a
test for it; the relevant ticket is in
#2749

- Upstream is gradually removing AWS Go SDK v1. This affected legacy
bucket resource that Pulumi retains in spite of upstream having it
removed. This is now working again but required another invasive patch
to undo upstream changes.

- New "verified permissions" module needed to be registered in
resources.go to support automatic module inference.

- Several new Plugin Framework based resources required patching to
support tags; this is slowing us down until we can implement
#2962

- Run test of provider shim was breaking as it didn't get all the
dependencies, now fixed

- Patch from #3290 wouldn't
apply because of wrong format (`make upstream.rebase` broken) - had to
reformat it

- doc_edits.go would keep appending edits to replacements.json; this is fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/import An issue related to `pulumi import` or the import resource option. kind/bug Some behavior is incorrect or out of spec p1 Bugs severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

6 participants