-
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
Add ipv6 support. #1024
Add ipv6 support. #1024
Conversation
desired.CidrBlock = *c.config.Networks.VPC.CIDR | ||
|
||
if !isIPv4(c.ipFamilies) && c.config.Networks.VPC.CIDR == nil { | ||
desired.CidrBlock = "10.0.0.0/16" |
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 think I forgot the outcome of that discussion around this hardcoded IP and it would be nice if here or better in a comment write something about it.
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.
Probably, it is better not to automatically assign an IPv4 address here and leave that to the user?
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.
Though, I need to adapt the validation.
AssignIpv6AddressOnCreation: ptr.To(isIPv6(c.ipFamilies)), | ||
CidrBlock: func(cidr string) string { | ||
if cidr == "" { | ||
return "10.0.32.0/20" |
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.
also here regarding the "magic IP"
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.
Same as above, it is probably better to let the user configure the CIDR for the public range.
/needs rebase |
@@ -1202,6 +1206,79 @@ func (c *Client) DeleteInternetGateway(ctx context.Context, id string) error { | |||
return ignoreNotFound(err) | |||
} | |||
|
|||
func (c *Client) CreateEgressOnlyInternetGateway(ctx context.Context, gateway *EgressOnlyInternetGateway) (*EgressOnlyInternetGateway, error) { |
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.
Public func CreateEgressOnlyInternetGateway
should have a describing comment
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 added a comment.
@@ -395,6 +407,18 @@ func generateTerraformInfraConfig(ctx context.Context, infrastructure *extension | |||
dhcpDomainName = fmt.Sprintf("%s.compute.internal", infrastructure.Spec.Region) | |||
} | |||
|
|||
isIPv4 := true | |||
isIPv6 := false | |||
if sets.New[v1beta1.IPFamily](ipFamilies...).Has(v1beta1.IPFamilyIPv6) { |
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 there a special reason why we use sets, couldn't we just use slices.Contains()
here?
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.
No special reason, I changed it to slices.Contains().
/test |
Testrun: e2e-f7zd9 +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | infrastructure-test | infrastructure-test-migrate | Failed | 22m30s | | infrastructure-test | infrastructure-test-recover | Failed | 23m14s | | bastion-test | bastion-test | Succeeded | 9m0s | | dnsrecord-test | dnsrecord-test | Succeeded | 6m14s | | infrastructure-test | infrastructure-test-tf | Failed | 35m9s | | infrastructure-test | infrastructure-test-flow | Succeeded | 26m43s | +---------------------+-----------------------------+-----------+----------+ |
IPv6 addresses of subnets were different for terraform and flow. This resulted in a subnet changes after migration.
/test |
Testrun: e2e-85nst +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | infrastructure-test | infrastructure-test-recover | Failed | 25m27s | | bastion-test | bastion-test | Succeeded | 8m43s | | dnsrecord-test | dnsrecord-test | Succeeded | 6m8s | | infrastructure-test | infrastructure-test-tf | Failed | 23m52s | | infrastructure-test | infrastructure-test-flow | Failed | 26m35s | | infrastructure-test | infrastructure-test-migrate | Failed | 20m59s | +---------------------+-----------------------------+-----------+----------+ |
/retest |
/test |
Testrun: e2e-vlt45 +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | dnsrecord-test | dnsrecord-test | Succeeded | 6m13s | | infrastructure-test | infrastructure-test-tf | Failed | 24m0s | | infrastructure-test | infrastructure-test-flow | Failed | 23m13s | | infrastructure-test | infrastructure-test-migrate | Failed | 24m54s | | infrastructure-test | infrastructure-test-recover | Succeeded | 27m51s | | bastion-test | bastion-test | Succeeded | 9m42s | +---------------------+-----------------------------+-----------+----------+ |
/test |
Testrun: e2e-sqqbf +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | dnsrecord-test | dnsrecord-test | Succeeded | 5m50s | | infrastructure-test | infrastructure-test-tf | Failed | 31m41s | | infrastructure-test | infrastructure-test-flow | Failed | 26m23s | | infrastructure-test | infrastructure-test-migrate | Failed | 30m27s | | infrastructure-test | infrastructure-test-recover | Failed | 23m48s | | bastion-test | bastion-test | Succeeded | 8m37s | +---------------------+-----------------------------+-----------+----------+ |
/test |
Testrun: e2e-2lxwq +---------------------+-----------------------------+-----------+----------+ | NAME | STEP | PHASE | DURATION | +---------------------+-----------------------------+-----------+----------+ | infrastructure-test | infrastructure-test-tf | Succeeded | 34m17s | | infrastructure-test | infrastructure-test-flow | Succeeded | 26m7s | | infrastructure-test | infrastructure-test-migrate | Succeeded | 30m4s | | infrastructure-test | infrastructure-test-recover | Succeeded | 27m25s | | bastion-test | bastion-test | Succeeded | 4m36s | | dnsrecord-test | dnsrecord-test | Succeeded | 5m53s | +---------------------+-----------------------------+-----------+----------+ |
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:
The PR adds required infrastructure changes to deploy IPv6 shoots.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Release note: