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

Revert upstream changes triggering LB panic #3426

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Feb 14, 2024

This reverts the following upstream PRs:
hashicorp/terraform-provider-aws#35671
hashicorp/terraform-provider-aws#35678
as a quick fix to mitigate #3421 until we root-cause it.

Details on my findings so far: #3421 (comment)
It looks to me like the issue originates somewhere in our handling of nulls/empty in the bridge, so seems unlikely to get fixed today.

It also adds a test for LB listeners. I've verified that the test triggers the panic without the patches and that the patches resolve it.

Copy link

Does the PR have any schema changes?

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.

@VenelinMartinov VenelinMartinov changed the title Repro for LB listener panic Revert upstream changes triggering LB panic Feb 14, 2024
@VenelinMartinov VenelinMartinov enabled auto-merge (squash) February 14, 2024 15:59
@t0yv0
Copy link
Member

t0yv0 commented Feb 14, 2024

Does it still panic if this resource is opted into PlanResourceChange handling in here:

		shimv2.WithPlanResourceChange(func(s string) bool {
			switch s {
			case "aws_ssm_document", "aws_wafv2_web_acl":
				return true
			default:
				return false
			}
		}))

@VenelinMartinov
Copy link
Contributor Author

Does it still panic if this resource is opted into PlanResourceChange handling in here:

Unfortunately that doesn't seem to fix it. I still get the panic with the following change there:

shimv2.WithPlanResourceChange(func(s string) bool {
	switch s {
	case "aws_ssm_document", "aws_wafv2_web_acl", "aws_alb_listener", "aws_lb_listener":
		return true
	default:
		return false
	}
}))

@t0yv0
Copy link
Member

t0yv0 commented Feb 14, 2024

Lets make sure an issue (for the patch) remains open to get this root-caused. Let's ship the fix!

@VenelinMartinov
Copy link
Contributor Author

I've opened #3427 to continue the investigation

@VenelinMartinov VenelinMartinov enabled auto-merge (squash) February 14, 2024 16:40
@VenelinMartinov VenelinMartinov added the needs-release/patch When a PR with this label merges, it initiates a release of vX.Y.Z+1 label Feb 14, 2024
@VenelinMartinov VenelinMartinov merged commit 32a25bf into master Feb 14, 2024
18 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/lb_listener_panic branch February 14, 2024 17:20
@github-actions github-actions bot removed the needs-release/patch When a PR with this label merges, it initiates a release of vX.Y.Z+1 label Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants