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

Route table duplicate "0.0.0.0/0" rule creation #1111

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

kon-angelo
Copy link
Contributor

How to categorize this PR?

/area control-plane
/kind bug
/platform aws

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Fix an issue where the "0.0.0.0/0" route creation would fail if the nat-gateway was previously deleted.

@kon-angelo kon-angelo requested review from a team as code owners October 30, 2024 15:59
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 30, 2024
@kon-angelo
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Oct 30, 2024

Testrun: e2e-btwqj
Workflow: e2e-btwqj-wf
Phase: Succeeded

+---------------------+-----------------------------+-----------+----------+
|        NAME         |            STEP             |   PHASE   | DURATION |
+---------------------+-----------------------------+-----------+----------+
| infrastructure-test | infrastructure-test-recover | Succeeded | 27m15s   |
| bastion-test        | bastion-test                | Succeeded | 4m38s    |
| dnsrecord-test      | dnsrecord-test              | Succeeded | 5m51s    |
| infrastructure-test | infrastructure-test-tf      | Succeeded | 34m34s   |
| infrastructure-test | infrastructure-test-flow    | Succeeded | 26m21s   |
| infrastructure-test | infrastructure-test-migrate | Succeeded | 31m7s    |
+---------------------+-----------------------------+-----------+----------+

@gardener-robot gardener-robot added needs/review Needs review area/control-plane Control plane related kind/bug Bug platform/aws Amazon web services platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Oct 30, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 30, 2024
if !slices.ContainsFunc(desired.Routes, func(r *Route) bool {
desiredRouteDestination, err := r.DestinationId()
if err != nil {
log.Error(err, "failed to find suitable destination for desired route", "route")
Copy link
Member

Choose a reason for hiding this comment

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

This will result in a line like msg="failed to find suitable destination for desired route" error="no route destination found", which is a little bit redundant for my taste.
Maybe something more like

log.Error(err, "error deleting route", "route")

Which is not really that much better but does not contain the same information twice.
Also, I think there is an argument missing, don't you mean:

log.Error(err, "failed to find suitable destination for desired route", "route", route)

@@ -2134,6 +2134,7 @@ func IsNotFoundError(err error) bool {
// IsAlreadyAssociatedError returns true if the given error is a awserr.Error indicating that an AWS resource was already associated.
func IsAlreadyAssociatedError(err error) bool {
if aerr, ok := err.(awserr.Error); ok && aerr.Code() == "Resource.AlreadyAssociated" {
aerr.Code()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a debug line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 31, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2024
@kon-angelo
Copy link
Contributor Author

@AndreasBurger @hebelsan PTAL

@kon-angelo
Copy link
Contributor Author

/test

@testmachinery
Copy link

testmachinery bot commented Oct 31, 2024

Testrun: e2e-76rtb
Workflow: e2e-76rtb-wf
Phase: Succeeded

+---------------------+-----------------------------+-----------+----------+
|        NAME         |            STEP             |   PHASE   | DURATION |
+---------------------+-----------------------------+-----------+----------+
| infrastructure-test | infrastructure-test-recover | Succeeded | 26m28s   |
| bastion-test        | bastion-test                | Succeeded | 6m36s    |
| dnsrecord-test      | dnsrecord-test              | Succeeded | 6m31s    |
| infrastructure-test | infrastructure-test-tf      | Succeeded | 36m12s   |
| infrastructure-test | infrastructure-test-flow    | Succeeded | 27m28s   |
| infrastructure-test | infrastructure-test-migrate | Succeeded | 31m18s   |
+---------------------+-----------------------------+-----------+----------+

Copy link
Contributor

@hebelsan hebelsan left a comment

Choose a reason for hiding this comment

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

lgtm, however we never make use of the bool return value modified of the function UpdateRouteTable.
Should we therefore drop it?

@kon-angelo
Copy link
Contributor Author

lgtm, however we never make use of the bool return value modified of the function UpdateRouteTable. Should we therefore drop it?

The API for the updater is locally consistent, so I don't agree necessarily agree. We can consider it in a different change

hebelsan
hebelsan previously approved these changes Oct 31, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 31, 2024
@AndreasBurger AndreasBurger merged commit bd19ff5 into gardener:master Oct 31, 2024
10 checks passed
@kon-angelo kon-angelo deleted the route-table-00 branch October 31, 2024 12:45
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/control-plane Control plane related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review platform/aws Amazon web services platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants