-
Notifications
You must be signed in to change notification settings - Fork 6
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
Create events on route updates failures and don't keep node routes to main route table #8
Conversation
/invite @ScheererJ @DockToFuture @axel7born |
41fc737
to
a869359
Compare
pkg/updater/routes.go
Outdated
for _, route := range table.Routes { | ||
if route.NatGatewayId != nil && aws.StringValue(route.DestinationCidrBlock) == "0.0.0.0/0" { | ||
// all subnet route tables have a NAT gateway route |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it makes sense to make the inverse check of looking at IGW instead of the NAT GWs. Would it make any sense in BYO-VPC cases where there may be other route tables ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Checking of IGW now in main route table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it always required to have an internet gateway? Would it be possible to do it differently in a bring-your-own scenario, e.g. via vpc peering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently this is checked at runtime: https://github.com/gardener/gardener-extension-provider-aws/blob/907425e071477b973e56f319c43786a43ede67af/pkg/controller/infrastructure/configvalidator.go#L119 especially for BYO scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have simplified the main table recognition by using the name tag.
What do you think? @ScheererJ @kon-angelo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
a869359
to
ecf73d1
Compare
ecf73d1
to
9406165
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
What this PR does / why we need it:
Events are now created in the
kube-system
namespace on route updates, especially if they are failing.Note, that on normal operation the event is created only once.
If the route update has errors, the event is created everytime. As soon as it recovers an event for normal operation is created one time.
These events will be analysed by a health check of the provider-aws extension.
Additionally, the main table will not contain the routes to the worker nodes anymore. It is sufficient to have them in the route tables for the subnets. If the VPC is used by multiple clusters, keeping the routes in the main table would cause quota issues (
RouteLimitExceeded
) just sooner. There is a hard limit of 1000 routes per route table according to the AWS documentation, see Amazon VPC quotasWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: