Issue 2090/route table order dependence #236

gfrey · 2025-01-31T14:38:37Z

Fixes issue #2090

Description of changes:

Configure generator for RouteTable to ignore the Routes from the generated delta code.
Extend the customPreCompare code to take care of the routes (computing the delta ignoring the order of routes).
Added unit test for the custom code.
Change the syncRoutes code to use the delta from the compare phase.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2025-01-31T14:38:48Z

Hi @gfrey. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gfrey · 2025-01-31T14:42:01Z

@a-hilaly as discussed yesterday my attempt to address this:

I didn't include the generator output created locally; I assume it is taken care of by the pipeline.
I added a unit-test for my own piece of mind, even though the rest of the code doesn't have any (this required the go.mod change for the assertion library). If this isn't appropriate happy to change.
I tested this change locally in a limited setting. There it was working. Should we add more e2e tests for this?

a-hilaly · 2025-02-03T17:48:07Z

/ok-to-test

gfrey · 2025-02-04T10:49:16Z

/retest

michaelhtm

Hey @gfrey, thanks for the contribution..left a few comments.

generator.yaml

pkg/resource/route_table/hooks.go

michaelhtm · 2025-02-11T21:21:44Z

pkg/resource/route_table/hooks.go

nit: can we also add an e2e test addressing the issue?

To replicate my initial problem, I'd need to understand when order of routes changes in the AWS API. Otherwise, I wouldn't know how to test this.

gfrey · 2025-02-20T11:30:18Z

The failing ec2-kind-e2e test seems to be unrelated to my changes, as it also came up in #241.

michaelhtm

Thanks for your patience @gfrey,
The flaky tests are due to eventual consistencies with tagging..we will have a fix for it in our next release, but this looks good to me, just need to generate with latest code-gen :)

pkg/resource/route_table/hooks.go

michaelhtm

one more comment

pkg/resource/route_table/hooks.go

In the past the routes were not order independent. Whenever the API would return a different order than specified, the routes would be deleted and recreated. The `getMatchingRoute` code also returned the first route that would have a matching field like the VPCPeeringConnectionId. This resulted in routes not being stable. Now the `customPreCompare` handles routes properly, i.e. is order independent. Also the `syncRoutes` uses the computed diff, to create or delete the according routes.

This is the issue that made the e2e tests fail.

This should help to make obvious what the different steps are doing. Also added information on the route related diffs in the delta.

This is not strictly necessary, but as these are already copied, not an issue either.

The delta contains the routes that are different between the "desired" and the "latest" state. The identical ones are ignored.

The differences for complex types (e.g. the lists of tags or routes) are given as the entire complex type, but only the changed parts. This is the way it currently is, so stick to that.

michaelhtm

Thank you for the revision @gfrey!!
left a few more comments

pkg/resource/route_table/hooks.go

- Rename function - Removed `delta` from `syncRoutes` signature and thereby got rid of a lot of additional code. This is possible now that we recompute the differences, instead of using the information from the delta.

michaelhtm

Hi @gfrey, I have one more comment, it should be a small fix, and also rebasing to latest changes (they shouldn't affect your PR much), thanks!

Ps: I accidentally added a commit to this PR, while trying to check it out locally, please feel free to remove it!

pkg/resource/route_table/hooks.go

The CreateRouteInput type supports a lot of references to different types. Those will not be set in latest. So if any references are used, the DeepEqual will always fail. The dedicated compareCreateRouteInput method will only look at the final types.

`latest` is nil if the syncRoutes method is called from createRoutes.

michaelhtm · 2025-02-28T17:47:18Z

/retest

michaelhtm · 2025-02-28T19:00:19Z

/retest

This got lost somewhere on the way, so re-add it now.

gfrey · 2025-03-03T10:25:36Z

/test ec2-kind-e2e

michaelhtm · 2025-03-04T00:37:56Z

/retest

michaelhtm · 2025-03-04T02:22:46Z

/retest

a-hilaly

Awesome! dankeshoen her @gfrey !!

michaelhtm · 2025-03-05T01:08:40Z

/retest

a-hilaly · 2025-03-05T03:57:13Z

/retest

gfrey · 2025-03-10T08:28:04Z

@michaelhtm @a-hilaly Is there anything missing to get this merged? Anything I can help with?

michaelhtm · 2025-03-10T20:33:29Z

Hey @gfrey, just wanted to merge this that fixes the failing test and this one should be GTG

michaelhtm · 2025-03-10T22:12:30Z

/retest

ack-prow · 2025-03-10T22:12:46Z

@gfrey: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ec2-verify-attribution	`b433937`	link	false	`/test ec2-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

michaelhtm · 2025-03-10T22:59:23Z

/lgtm

ack-prow · 2025-03-10T22:59:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, gfrey, michaelhtm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [a-hilaly,michaelhtm]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 31, 2025

ack-prow bot requested review from a-hilaly and rushmash91 January 31, 2025 14:39

eqe-aws requested a review from michaelhtm February 3, 2025 17:28

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 3, 2025

michaelhtm reviewed Feb 11, 2025

View reviewed changes

gfrey force-pushed the issue-2090/route-table-order-dependence branch from 5be11fd to 982441e Compare February 20, 2025 10:38

michaelhtm reviewed Feb 21, 2025

View reviewed changes

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

michaelhtm reviewed Feb 21, 2025

View reviewed changes

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

Gereon Frey added 7 commits February 24, 2025 09:24

Add test for CustomerPreCompare

a5e0f0e

Remove local routes when creating a route table

2386870

This is the issue that made the e2e tests fail.

Add comments to syncRoutes function

6b8268f

This should help to make obvious what the different steps are doing. Also added information on the route related diffs in the delta.

Update generated code using code-generator v0.43.2

9bd0e21

Create deep copies of route inputs

5cce445

This is not strictly necessary, but as these are already copied, not an issue either.

Rename variables to make intent more obvious

86e6f18

The delta contains the routes that are different between the "desired" and the "latest" state. The identical ones are ignored.

gfrey force-pushed the issue-2090/route-table-order-dependence branch from 341d176 to 86e6f18 Compare February 24, 2025 10:51

Gereon Frey added 2 commits February 25, 2025 08:05

Adopt ACK style delta

2ac08c2

The differences for complex types (e.g. the lists of tags or routes) are given as the entire complex type, but only the changed parts. This is the way it currently is, so stick to that.

Fix tests

9d6f263

michaelhtm reviewed Feb 25, 2025

View reviewed changes

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

Gereon Frey and others added 2 commits February 26, 2025 08:54

Add review feedback

7d7fb6c

- Rename function - Removed `delta` from `syncRoutes` signature and thereby got rid of a lot of additional code. This is possible now that we recompute the differences, instead of using the information from the delta.

Merge branch 'main' into issue-2090/route-table-order-dependence

87b8df9

michaelhtm reviewed Feb 27, 2025

View reviewed changes

pkg/resource/route_table/hooks.go Outdated Show resolved Hide resolved

Fix route comparison

13385d6

The CreateRouteInput type supports a lot of references to different types. Those will not be set in latest. So if any references are used, the DeepEqual will always fail. The dedicated compareCreateRouteInput method will only look at the final types.

Fix issue with NPE

9843ec3

`latest` is nil if the syncRoutes method is called from createRoutes.

Remove local routes

ece9faa

This got lost somewhere on the way, so re-add it now.

a-hilaly approved these changes Mar 5, 2025

View reviewed changes

ack-prow bot added the approved label Mar 5, 2025

Merge branch 'main' into issue-2090/route-table-order-dependence

b433937

ack-prow bot assigned michaelhtm Mar 10, 2025

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2025

ack-prow bot merged commit 7f23e2f into aws-controllers-k8s:main Mar 10, 2025
6 of 7 checks passed

gfrey deleted the issue-2090/route-table-order-dependence branch March 11, 2025 07:15

Issue 2090/route table order dependence #236

Issue 2090/route table order dependence #236

Uh oh!

Conversation

gfrey commented Jan 31, 2025 • edited by michaelhtm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented Jan 31, 2025

Uh oh!

gfrey commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly commented Feb 3, 2025

Uh oh!

gfrey commented Feb 4, 2025

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelhtm Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

gfrey Feb 20, 2025

Choose a reason for hiding this comment

Uh oh!

gfrey commented Feb 20, 2025

Uh oh!

michaelhtm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelhtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelhtm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelhtm commented Feb 28, 2025

Uh oh!

michaelhtm commented Feb 28, 2025

Uh oh!

gfrey commented Mar 3, 2025

Uh oh!

michaelhtm commented Mar 4, 2025

Uh oh!

michaelhtm commented Mar 4, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

michaelhtm commented Mar 5, 2025

Uh oh!

a-hilaly commented Mar 5, 2025

Uh oh!

gfrey commented Mar 10, 2025

Uh oh!

michaelhtm commented Mar 10, 2025

Uh oh!

michaelhtm commented Mar 10, 2025

Uh oh!

ack-prow bot commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelhtm commented Mar 10, 2025

Uh oh!

ack-prow bot commented Mar 10, 2025

Uh oh!

gfrey commented Jan 31, 2025 •

edited by michaelhtm

Loading

gfrey commented Jan 31, 2025 •

edited

Loading

michaelhtm left a comment •

edited

Loading

michaelhtm left a comment •

edited

Loading

ack-prow bot commented Mar 10, 2025 •

edited

Loading