Root cause 3421 and revert patches 0042 and 0043 #3427
Assignees
Labels
blocked
The issue cannot be resolved without 3rd party action.
kind/bug
Some behavior is incorrect or out of spec
resolution/fixed
This issue was fixed
Comments
VenelinMartinov
added
kind/bug
|
I created a GRPC test for iterating on this which just runs the func TestLBPanic(t *testing.T) {
replay(t, `[
{
"method": "/pulumirpc.ResourceProvider/Configure",
"request": {
"variables": {
"aws:config:region": "us-east-1",
"aws:config:skipCredentialsValidation": "false",
"aws:config:skipMetadataApiCheck": "true",
"aws:config:skipRegionValidation": "true"
},
"args": {
"region": "us-east-1",
"skipCredentialsValidation": "false",
"skipMetadataApiCheck": "true",
"skipRegionValidation": "true",
"version": "6.22.0"
},
"acceptSecrets": true,
"acceptResources": true,
"sendsOldInputs": true,
"sendsOldInputsToDelete": true
},
"response": {
"supportsPreview": true
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
},
{
"method": "/pulumirpc.ResourceProvider/Create",
"request": {
"urn": "urn:pulumi:dev::lb_panic::aws:lb/listener:Listener::payload-lb-listner-http",
"properties": {
"__defaults": [],
"defaultActions": [
{
"__defaults": [],
"targetGroupArn": "arn:aws:elasticloadbalancing:us-east-1:616138583583:targetgroup/payload-tg-cae0e1a/92aa6f538c20f368",
"type": "forward"
}
],
"loadBalancerArn": "arn:aws:elasticloadbalancing:us-east-1:616138583583:loadbalancer/app/payload-lb-f60c982/289d36c768b0b480",
"port": 80,
"protocol": "HTTP"
}
},
"response:": "*",
"metadata": {
"kind": "resource",
"mode": "client",
"name": "aws"
}
}
]`)
}
config.Config["forward"] = []interface{}{}
config.Raw["forward"] = []interface{}{}
EDIT: This was an artifact of my local testing setup, not actually true. I'll continue trying to hack it into working. |
|
I've enrolled the listener into Here is the result of dumping cfg=cty.ObjectVal(map[string]cty.Value{"alpn_policy":cty.NullVal(cty.String), "arn":cty.NullVal(cty.String), "certificate_arn":cty.NullVal(cty.String), "default_action":cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"authenticate_cognito":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"authentication_request_extra_params":cty.Map(cty.String), "on_unauthenticated_request":cty.String, "scope":cty.String, "session_cookie_name":cty.String, "session_timeout":cty.Number, "user_pool_arn":cty.String, "user_pool_client_id":cty.String, "user_pool_domain":cty.String}))), "authenticate_oidc":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"authentication_request_extra_params":cty.Map(cty.String), "authorization_endpoint":cty.String, "client_id":cty.String, "client_secret":cty.String, "issuer":cty.String, "on_unauthenticated_request":cty.String, "scope":cty.String, "session_cookie_name":cty.String, "session_timeout":cty.Number, "token_endpoint":cty.String, "user_info_endpoint":cty.String}))), "fixed_response":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"content_type":cty.String, "message_body":cty.String, "status_code":cty.String}))), "forward":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"stickiness":cty.List(cty.Object(map[string]cty.Type{"duration":cty.Number, "enabled":cty.Bool})), "target_group":cty.Set(cty.Object(map[string]cty.Type{"arn":cty.String, "weight":cty.Number}))}))), "order":cty.NullVal(cty.Number), "redirect":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"host":cty.String, "path":cty.String, "port":cty.String, "protocol":cty.String, "query":cty.String, "status_code":cty.String}))), "target_group_arn":cty.StringVal("arn:aws:elasticloadbalancing:us-east-1:616138583583:targetgroup/payload-tg-cae0e1a/92aa6f538c20f368"), "type":cty.StringVal("forward")})}), "id":cty.NullVal(cty.String), "load_balancer_arn":cty.StringVal("arn:aws:elasticloadbalancing:us-east-1:616138583583:loadbalancer/app/payload-lb-f60c982/289d36c768b0b480"), "mutual_authentication":cty.NullVal(cty.List(cty.Object(map[string]cty.Type{"ignore_client_certificate_expiry":cty.Bool, "mode":cty.String, "trust_store_arn":cty.String}))), "port":cty.NumberIntVal(80), "protocol":cty.StringVal("HTTP"), "ssl_policy":cty.NullVal(cty.String), "tags":cty.NullVal(cty.Map(cty.String)), "tags_all":cty.NullVal(cty.Map(cty.String)), "timeouts":cty.NullVal(cty.Object(map[string]cty.Type{"create":cty.String, "update":cty.String}))})
plan=&struct { PlannedState cty.Value; PlannedMeta map[string]interface {}; PlannedDiff *terraform.InstanceDiff }{
PlannedState: cty.ObjectVal(map[string]cty.Value{"alpn_policy":cty.NullVal(cty.String), "arn":cty.UnknownVal(cty.String), "certificate_arn":cty.NullVal(cty.String), "default_action":cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"authenticate_cognito":cty.ListValEmpty(cty.Object(map[string]cty.Type{"authentication_request_extra_params":cty.Map(cty.String), "on_unauthenticated_request":cty.String, "scope":cty.String, "session_cookie_name":cty.String, "session_timeout":cty.Number, "user_pool_arn":cty.String, "user_pool_client_id":cty.String, "user_pool_domain":cty.String})), "authenticate_oidc":cty.ListValEmpty(cty.Object(map[string]cty.Type{"authentication_request_extra_params":cty.Map(cty.String), "authorization_endpoint":cty.String, "client_id":cty.String, "client_secret":cty.String, "issuer":cty.String, "on_unauthenticated_request":cty.String, "scope":cty.String, "session_cookie_name":cty.String, "session_timeout":cty.Number, "token_endpoint":cty.String, "user_info_endpoint":cty.String})), "fixed_response":cty.ListValEmpty(cty.Object(map[string]cty.Type{"content_type":cty.String, "message_body":cty.String, "status_code":cty.String})), "forward":cty.ListValEmpty(cty.Object(map[string]cty.Type{"stickiness":cty.List(cty.Object(map[string]cty.Type{"duration":cty.Number, "enabled":cty.Bool})), "target_group":cty.Set(cty.Object(map[string]cty.Type{"arn":cty.String, "weight":cty.Number}))})), "order":cty.UnknownVal(cty.Number), "redirect":cty.ListValEmpty(cty.Object(map[string]cty.Type{"host":cty.String, "path":cty.String, "port":cty.String, "protocol":cty.String, "query":cty.String, "status_code":cty.String})), "target_group_arn":cty.StringVal("arn:aws:elasticloadbalancing:us-east-1:616138583583:targetgroup/payload-tg-cae0e1a/92aa6f538c20f368"), "type":cty.StringVal("forward")})}), "id":cty.UnknownVal(cty.String), "load_balancer_arn":cty.StringVal("arn:aws:elasticloadbalancing:us-east-1:616138583583:loadbalancer/app/payload-lb-f60c982/289d36c768b0b480"), "mutual_authentication":cty.UnknownVal(cty.List(cty.Object(map[string]cty.Type{"ignore_client_certificate_expiry":cty.Bool, "mode":cty.String, "trust_store_arn":cty.String}))), "port":cty.NumberIntVal(80), "protocol":cty.StringVal("HTTP"), "ssl_policy":cty.UnknownVal(cty.String), "tags":cty.NullVal(cty.Map(cty.String)), "tags_all":cty.NullVal(cty.Map(cty.String)), "timeouts":cty.NullVal(cty.Object(map[string]cty.Type{"create":cty.String, "update":cty.String}))}),
PlannedMeta: {
"_new_extra_shim": map[string]interface {}{
"protocol": "HTTP",
},
"e2bfb730-ecaa-11e6-8f88-34363bc7c4c0": map[string]interface {}{
"create": float64(3e+11),
"update": float64(3e+11),
},
},
PlannedDiff: *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"arn":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "default_action.#":*terraform.ResourceAttrDiff{Old:"", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "default_action.0.order":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "default_action.0.target_group_arn":*terraform.ResourceAttrDiff{Old:"", New:"arn:aws:elasticloadbalancing:us-east-1:616138583583:targetgroup/payload-tg-cae0e1a/92aa6f538c20f368", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "default_action.0.type":*terraform.ResourceAttrDiff{Old:"", New:"forward", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "load_balancer_arn":*terraform.ResourceAttrDiff{Old:"", New:"arn:aws:elasticloadbalancing:us-east-1:616138583583:loadbalancer/app/payload-lb-f60c982/289d36c768b0b480", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:true, Sensitive:false, Type:0x0}, "mutual_authentication.#":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "port":*terraform.ResourceAttrDiff{Old:"", New:"80", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "protocol":*terraform.ResourceAttrDiff{Old:"", New:"HTTP", NewComputed:false, NewRemoved:false, NewExtra:"HTTP", RequiresNew:false, Sensitive:false, Type:0x0}, "ssl_policy":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, RawConfig:cty.NilVal, RawState:cty.NilVal, RawPlan:cty.NilVal, Meta:map[string]interface {}(nil)},
}Noticed that
Perhaps we should be ending up with a |
|
I dumped some TF state from and got the following: Noticeably, Can't really compare |
|
Might be easier to observe the diffs with just an update to the relevant arguments, instead of a import * as aws from "@pulumi/aws";
const vpc = new aws.ec2.Vpc("main", { cidrBlock: "10.0.0.0/16" });
const subnet = new aws.ec2.Subnet("main", {
vpcId: vpc.id,
cidrBlock: "10.0.12.0/24",
availabilityZone: "us-east-1a"
});
const subnet2 = new aws.ec2.Subnet("subnet2", {
vpcId: vpc.id,
cidrBlock: "10.0.5.0/24",
availabilityZone: "us-east-1b"
});
const secGroup = new aws.ec2.SecurityGroup("allowTls", {
description: "Allow TLS inbound traffic and all outbound traffic",
vpcId: vpc.id,
tags: {
Name: "allow_tls",
},
});
const loadbalancer = new aws.lb.LoadBalancer("payload-lb", {
loadBalancerType: "application",
securityGroups: [secGroup.id],
subnets: [subnet.id, subnet2.id],
internal: true,
});
const targetGroup = new aws.lb.TargetGroup("payload-tg", {
port: 80,
protocol: "HTTP",
targetType: "ip",
vpcId: vpc.id,
});
export const vpcId = vpc.id;
export const subnet1Id = subnet.id
export const subnet2Id = subnet2.id
new aws.lb.Listener("payload-lb-listner-http", {
loadBalancerArn: loadbalancer.arn,
port: 80,
protocol: "HTTP",
defaultActions: [
{
type: "forward",
// type: "redirect",
targetGroupArn: targetGroup.arn,
redirect: {
statusCode: "HTTP_301",
host: "google.com"
}
},
],
});And the equivalent TF one: provider "aws" {
region = "us-east-1"
version = "5.37"
}
resource "aws_lb" "test" {
name = "test-lb-tf"
load_balancer_type = "application"
subnets = ["subnet-0b2a83a27caf03420", "subnet-0e3cfe0880125b082"]
internal = true
}
resource "aws_lb_target_group" "test" {
name = "tf-example-lb-tg"
port = 80
protocol = "HTTP"
vpc_id = "vpc-018901b37128b063b"
}
resource "aws_lb_listener" "front_end" {
load_balancer_arn = aws_lb.test.arn
port = "80"
protocol = "HTTP"
default_action {
type = "forward"
# type = "redirect"
target_group_arn = aws_lb_target_group.test.arn
redirect {
status_code = "HTTP_301"
host = "google.com"
}
}
}Having both the Will try comparing the diffs etc again between these two. EDIT: Actually, I might have better luck logging on the upstream provider side and comparing what the upstream provider gets when used via TF and via pulumi - I'll try that next. |
|
I dumped the resource data from the In TF: Also TF: as well as some differences in Specifically, what I believe is causing the issue here is |
|
Also, unrelated, but noticed the lack of defaults applied on the TF side. I believe this is related to pulumi/pulumi-terraform-bridge#1546 (comment) but I have yet to write down what I know for this in a separate issue. |
|
I verified that I have a "fix" here: pulumi/pulumi-terraform-bridge#1688 and have verified that it gets rid of the diff in the values passed to the LB Create, barring resource ARNs, defaults, tags and the state being nil. |
VenelinMartinov
added a commit
to pulumi/pulumi-terraform-bridge
that referenced
this issue
Feb 23, 2024
Addresses pulumi/pulumi-aws#3427 We should make sure we send the upstream provider an empty array where MaxItemsOne properties are expected instead of a nil, since that is what TF does.
|
pulumi/pulumi-terraform-bridge#1688 was not the full story - Looks like TF does send nulls for MaxItemsOne properties sometimes: |
|
Investigating the issue again in pulumi/pulumi-terraform-bridge#1725. I've ran downstream tests on all the bridged providers and collected the following groups of errors: Observing some issues around defaults and ConflictsWith/ExactlyOneOf: Issues around MinLength 1: Attribute must be a single value, not a list: Value for unconfigurable attribute: |
|
I'll work through them all but most of these look like Check issues - these should be solvable by not doing the default empty list during Check, similarly to what we did for default values in pulumi/pulumi-terraform-bridge#1546 The last one I haven't seen before but might also be an issue with Check. The plan is to write regression tests for all of these in the bridge and in at least one provider for each and then make sure the change passes before re-merging. |
|
Yeah we can pretest the PR against all the providers again before merging. But I'm curious what you find on the Check issues. Thanks! |
|
The RKE error is the only one I was not able to repro in tests so far: https://github.com/pulumi/pulumi-rke/actions/runs/8143179673/job/22290626266 We have a slightly odd schema there. "role": {
Type: schema.TypeList,
Required: true,
Description: "Node roles in k8s cluster [controlplane/worker/etcd])",
Elem: &schema.Schema{
Type: schema.TypeString,
ValidateFunc: validation.StringInSlice(rkeClusterNodesRoles, true),
},
},
"roles": {
Type: schema.TypeString,
Optional: true,
Deprecated: "Use role instead",
Description: "Node role in kubernetes cluster [controlplane/worker/etcd], specified by a comma-separated string",
ValidateFunc: validation.StringInSlice(rkeClusterNodesRoles, true),
},us: "nodes": {
Elem: &tfbridge.SchemaInfo{
Fields: map[string]*tfbridge.SchemaInfo{
"roles": {
Name: "rolesDeprecated",
CSharpName: "RolesDeprecated",
},
},
},
},Then the test has: roles: [ "controlplane", "etcd", "worker" ],Interestingly, no My attempts at reproing are in https://github.com/pulumi/pulumi-terraform-bridge/pull/1725/files |
|
Sanitized GRPC log from before my change: {
"method": "/pulumirpc.ResourceProvider/Check",
"request": {
"urn": "urn:pulumi:dev::nodejs-singlenode::rke:index/cluster:Cluster::actions",
"olds": {},
"news": {
"cloudProvider": {
"name": "aws"
},
"clusterName": "nodejs-test-cluster",
"enableCriDockerd": true,
"nodes": [
{
"address": "35.91.63.227",
"internalAddress": "172.31.20.126",
"roles": [
"controlplane",
"etcd",
"worker"
],
"user": "ubuntu"
}
]
},
"randomSeed": "u4bzc8sXQnlykomRfaRaMyMIySbIvcoLDM8bOH6UoKE="
},
"response": {
"inputs": {
"__defaults": [
"customCerts",
"dind",
"dindDnsServer",
"dindStorageDriver",
"disablePortCheck",
"kubernetesVersion",
"updateOnly"
],
"cloudProvider": {
"__defaults": [],
"name": "aws"
},
"clusterName": "nodejs-test-cluster",
"customCerts": false,
"dind": false,
"dindDnsServer": "",
"dindStorageDriver": "",
"disablePortCheck": false,
"enableCriDockerd": true,
"kubernetesVersion": "v1.26.9-rancher1-1",
"nodes": [
{
"__defaults": [],
"address": "35.91.63.227",
"internalAddress": "172.31.20.126",
"roles": [
"controlplane",
"etcd",
"worker"
],
"user": {
"4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270",
"value": "ubuntu"
}
}
],
"updateOnly": false
}
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "rke"
}
}After: {
"method": "/pulumirpc.ResourceProvider/Check",
"request": {
"urn": "urn:pulumi:dev::nodejs-singlenode::rke:index/cluster:Cluster::actions",
"olds": {},
"news": {
"cloudProvider": {
"name": "aws"
},
"clusterName": "nodejs-test-cluster",
"enableCriDockerd": true,
"nodes": [
{
"address": "35.91.63.227",
"internalAddress": "172.31.20.126",
"roles": [
"controlplane",
"etcd",
"worker"
],
"user": "ubuntu"
}
]
},
"randomSeed": "oKVLyrq0I4TuGOa3V1UKwDPUVugpOi32s8KXbhaj60M="
},
"response": {
"inputs": {
"__defaults": [
"customCerts",
"dind",
"dindDnsServer",
"dindStorageDriver",
"disablePortCheck",
"kubernetesVersion",
"updateOnly"
],
"authentication": null,
"authorization": null,
"bastionHost": null,
"cloudProvider": {
"__defaults": [],
"awsCloudConfig": null,
"awsCloudProvider": null,
"azureCloudConfig": null,
"azureCloudProvider": null,
"name": "aws",
"openstackCloudConfig": null,
"openstackCloudProvider": null,
"vsphereCloudConfig": null,
"vsphereCloudProvider": null
},
"clusterName": "nodejs-test-cluster",
"customCerts": false,
"dind": false,
"dindDnsServer": "",
"dindStorageDriver": "",
"disablePortCheck": false,
"dns": null,
"enableCriDockerd": true,
"ingress": null,
"kubernetesVersion": "v1.26.9-rancher1-1",
"monitoring": null,
"network": null,
"nodes": [
{
"__defaults": [],
"address": "35.91.63.227",
"internalAddress": "172.31.20.126",
"rolesDeprecated": [
"controlplane",
"etcd",
"worker"
],
"user": {
"4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270",
"value": "ubuntu"
}
}
],
"restore": null,
"rotateCertificates": null,
"services": null,
"servicesEtcdDeprecated": null,
"servicesKubeApiDeprecated": null,
"servicesKubeControllerDeprecated": null,
"servicesKubeProxyDeprecated": null,
"servicesKubeSchedulerDeprecated": null,
"servicesKubeletDeprecated": null,
"systemImages": null,
"updateOnly": false,
"upgradeStrategy": null
},
"failures": [
{
"reason": "Missing required argument. The argument \"nodes.0.role\" is required, but no definition was found.. Examine values at 'actions.nodes[0].roles'."
},
{
"reason": "Attribute must be a single value, not a list. Examine values at 'actions.nodes[0].rolesDeprecated'."
}
]
},
"metadata": {
"kind": "resource",
"mode": "client",
"name": "rke"
}
}Notice that the |
|
The RKE error seems present on master too, so unrelated to my changes. I've raised pulumi/pulumi-terraform-bridge#1729 for that. pulumi/pulumi-terraform-bridge@f963008 should have addressed the other issues, pre-testing the change in all providers now. |
VenelinMartinov
added a commit
to pulumi/pulumi-terraform-bridge
that referenced
this issue
Mar 19, 2024
This PR refactors the various functions related to converting pulumi inputs to TF inputs, hiding some public functions and simplifying the interface there. It also exposes a new option when making inputs: `EnableMaxItemsOneDefaults` which will allow us to fix pulumi/pulumi-aws#3427 once we can properly test the changes. Opted not to land the behavioural changes for `EnableMaxItemsOneDefaults` yet because of the multiple rounds of varied downstream regressions - seems like we need to add a framework for testing against the TF behaviour in order to land this. --------- Co-authored-by: Ian Wahbe <[email protected]>
|
Blocked by pulumi/pulumi-terraform-bridge#1767 |
|
I understand this is still blocked right. Marking it as such, we'll revisit periodically. |
This was referenced Jun 14, 2024
t0yv0
added a commit
that referenced
this issue
Jun 26, 2024
Fixes unexpected alb.Listener target group refresh. The change was fixed upstream but patched out due to an unrelated problem causing a panic in Pulumi. The root cause is now fixed in the bridge. This change drops the two obsolete patches to catch up with upstream, and enrolls alb.Listener in PlanResourceChange version of the bridge that fixes the issue. Fixes #3427 Fixes #3722 Fixes #3723 Fixes #4079
|
This issue has been addressed in PR #4083 and shipped in release v6.42.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What happened?
TLDR:
lb.Listenerpanics if theforwardparameter indefaultActionsis not specified.We shipped #3426 (comment) as a quick fix for #3421
We should properly root cause the issue and revert the patches once we fix it.
More details in #3421 (comment)
Note that the issue does not reproduce in TF and
PlanResourceChangedoes not fix it #3426 (comment)Example
TestRegress3421is a regression test for the issue - it should pass without the two upstream patches.panic:
Output of
pulumi about.
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).
The text was updated successfully, but these errors were encountered: