-
Notifications
You must be signed in to change notification settings - Fork 100
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 IPv6 ServiceCIDR and write IPv6 ranges to Infra.Status.Networking
#1081
Conversation
@axel7born Labels area/todo, kind/todo do not exist. |
28f9410
to
4cc8bda
Compare
4cc8bda
to
6ddb1c3
Compare
6ddb1c3
to
60b3046
Compare
60b3046
to
4085b14
Compare
/test |
Testrun: e2e-996mj +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | infrastructure-test | infrastructure-test-recover | Failed | 27m1s | | bastion-test | bastion-test | Succeeded | 8m46s | | dnsrecord-test | dnsrecord-test | Succeeded | 6m9s | | infrastructure-test | infrastructure-test-tf | Succeeded | 35m39s | | infrastructure-test | infrastructure-test-flow | Succeeded | 27m57s | | infrastructure-test | infrastructure-test-migrate | Succeeded | 30m58s | +---------------------+-----------------------------+-----------+----------+ |
/test |
Testrun: e2e-8dt5q +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | infrastructure-test | infrastructure-test-flow | Succeeded | 27m10s | | bastion-test | bastion-test | Succeeded | 8m48s | | dnsrecord-test | dnsrecord-test | Succeeded | 5m52s | | infrastructure-test | infrastructure-test-tf | Succeeded | 34m22s | | infrastructure-test | infrastructure-test-migrate | Succeeded | 30m44s | | infrastructure-test | infrastructure-test-recover | Succeeded | 25m23s | +---------------------+-----------------------------+-----------+----------+ |
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
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.
Mostly lgtm
pkg/aws/client/client.go
Outdated
} | ||
var cidrs []string | ||
for _, cidrReservation := range output.SubnetIpv6CidrReservations { | ||
cidrs = append(cidrs, *cidrReservation.Cidr) |
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.
Can we be sure that there won't be a nil pointer dereference?
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.
Actually, It would be weird to get an array containing nil values and a CIDRReservation without a Cidr would also be strange. What should I check for nil? cidrReservation, cidr or both?
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, it would be pretty unusual.
But I think we're better safe than sorry, so I vote for adding a check for both cidrReservation
and cidrReservation.Cidr
.
Co-authored-by: Alexander Hebel <alexander.hebel@sap.com>
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
How to categorize this PR?
/area networking
/kind enhancement
/platform aws
What this PR does / why we need it:
Create an IPv6 CIDR reservation for the first nodes subnet that we find, use it as Service CIDR and write it to
Infra.Status.Networking
.The CIDR of the VPC will be used as Node range and Pod range and written to
Infra.Status.Networking
.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: