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

add custom diff for LBListener forward options #1528

Merged

Conversation

erhancagirici
Copy link
Collaborator

Description of your changes

Fixes the update loop for LBListener.

Will fix #1352

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

manual

@erhancagirici erhancagirici force-pushed the lb-listener-custom-diff branch from 65e9b47 to 77d0d73 Compare October 18, 2024 12:42
@mergenci mergenci force-pushed the lb-listener-custom-diff branch from 2bd169a to 1ccb269 Compare October 18, 2024 14:04
mergenci
mergenci previously approved these changes Oct 18, 2024
Copy link
Collaborator

@mergenci mergenci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @erhancagirici 🙏 LGTM.

@mergenci mergenci marked this pull request as ready for review October 18, 2024 15:19
@mergenci
Copy link
Collaborator

/test-examples="examples/elbv2/v1beta1/lblistener.yaml"

@mergenci mergenci dismissed their stale review October 18, 2024 17:22

Uptest failure

@mergenci
Copy link
Collaborator

I realized that we should merge this PR after we consume upbound/terraform-provider-aws#276, so I dismissed my approval.

@mergenci
Copy link
Collaborator

/test-examples="examples/elbv2/v1beta1/lblistener.yaml"

@mergenci mergenci force-pushed the lb-listener-custom-diff branch from 5f16395 to 0585e02 Compare October 19, 2024 07:46
@mergenci
Copy link
Collaborator

/test-examples="examples/elbv2/v1beta1/lblistener.yaml"

@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/elbv2/v1beta1/lblistener.yaml"

@mergenci
Copy link
Collaborator

To summarize, Uptest failed because of a regression in LBListenerTarget introduced by Terraform provider version bump v5.68.0. The regression caused the resource to enter an update loop:

2024-10-19T22:45:51+03:00	DEBUG	provider-aws	Diff detected	{"uid": "ac9bbe35-e483-4f51-bfea-10fddcc1ddec", "name": "issue-1352-lbtargetgroup", "gvk": "elbv2.aws.upbound.io/v1beta1, Kind=LBTargetGroup", "instanceDiff": "*terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{\"target_health_state.0.unhealthy_draining_interval\":*terraform.ResourceAttrDiff{Old:\"\", New:\"0\", NewComputed:false, 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)}"}

The update loop was present in the Uptest we ran in the version bump PR, but we overlooked it because we only checked whether LBListener panicked, which it did.

Copy link
Collaborator

@mergenci mergenci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your weekend operation Erhan 🙂 Now LGTM for real.

@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/elbv2/v1beta2/lblistener-forward-single-targetgroup.yaml"

@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/elbv2/v1beta2/lblistener-forward-multiple-targetgroups.yaml"

@erhancagirici erhancagirici force-pushed the lb-listener-custom-diff branch from bc9f497 to a8c1fc4 Compare October 21, 2024 08:47
@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/elbv2/v1beta2/lblistener-forward-single-targetgroup.yaml"

@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/elbv2/v1beta2/lblistener-forward-multiple-targetgroups.yaml"

@erhancagirici erhancagirici force-pushed the lb-listener-custom-diff branch from a8c1fc4 to 545d410 Compare October 21, 2024 09:27
@erhancagirici
Copy link
Collaborator Author

added new example manifests to cover forward default action through default_action.0.forward (on top of the current example that default_action.0.target_group_arn) and the new tests also pass.

single target group: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/11436536351/job/31814191141
multiple target groups: https://github.com/crossplane-contrib/provider-upjet-aws/actions/runs/11436538196/job/31816315624

@erhancagirici erhancagirici merged commit 5c35bab into crossplane-contrib:main Oct 21, 2024
12 checks passed
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.

[Bug]: ELBv2 Provider v1.5.0+ LBListener 'panic'
2 participants