-
Notifications
You must be signed in to change notification settings - Fork 224
Adding dual-stack support for Cluster API GCP Provider #1582
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
base: main
Are you sure you want to change the base?
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions 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-sigs/prow repository. |
|
/hold |
✅ Deploy Preview for kubernetes-sigs-cluster-api-gcp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: barbacbd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @barbacbd. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
salasberryfin
left a 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.
Thanks @barbacbd, I left a couple of comments.
e85c011 to
db1abd6
Compare
|
Leaving this as a reference here. For
from the sdk: We can fill this out but it may not make any changes since we will not have ipv6 only vms |
salasberryfin
left a 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.
A couple of "nice to haves" that could be added here:
- E2E test case using a dual stack network configuration.
- A brief entry in the CAPG book that documents the new feature (this can be a follow-up PR).
| // InternalIpv6PrefixLength: The prefix length of the primary internal IPv6 range. | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +kubebuilder:validation:Maximum=128 | ||
| // +optional | ||
| InternalIpv6PrefixLength int `json:"internalIpv6PrefixLength,omitempty"` | ||
|
|
||
| // Ipv6Address: An IPv6 internal network address for this network interface. | ||
| // To use a static internal IP address, it must be unused and in the same | ||
| // region as the instance's zone. If not specified, Google Cloud will | ||
| // automatically assign an internal IPv6 address from the instance's subnetwork. | ||
| // +optional | ||
| Ipv6Address string `json:"ipv6Address,omitempty"` |
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.
If these are related, is it useful to have it as a combined CIDR string?
api/v1beta/types.go:
Adding support for machines and simple network changes for IPV6 or dual stack work.
- InternalIpv6PrefixLength
- IPv6Address
- StackType
cloud/scope/machine.go:
Adding support for instances and networks to allow dual stack components.
- InstanceNetworkInterfaceSpec
- InternalIpv6PrefixLength
- Ipv6AccessConfigs
- ExternalIPv6
- ExternalIpv6PrefixLength
- Type (when present always set to DIRECT_IPV6)
- Name (always set to External IPv6)
- IPv6AccessType
- Ipv6Address
- StackType
- InstanceSpec
- PrivateIpv6GoogleAccess
- InstanceNetworkInterfaceAliasIPRangesSpec
** This did not change. The AliasIPs appear to only support IPv4 CIDR or single address format.
cloud/interfaces.go:
Expose a couple of new functions to get StackType and IPvAddress information from the cluster specs. These are only
getter functions for the Machine.go to access.
api/v1beta1/types.go: Add a constant for the IPV6/DualStack resource support. cloud/*: HealthChecks, Addresses, and Forwarding rules will all return lists. These lists will contain the ipv4 resource and (during dual stack) the ipv6 resource.
2c83a82 to
a6fa61b
Compare
|
@barbacbd: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
api/v1beta/types.go:
Adding support for machines and simple network changes for IPV6 or dual stack work.
cloud/scope/machine.go:
Adding support for instances and networks to allow dual stack components.
InstanceNetworkInterfaceSpec
InstanceSpec
InstanceNetworkInterfaceAliasIPRangesSpec ** This did not change. The AliasIPs appear to only support IPv4 CIDR or single address format.
cloud/interfaces.go:
Expose a couple of new functions to get StackType and IPvAddress information from the cluster specs. These are only getter functions for the Machine.go to access.
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
This PR will serve as a focal point for adding dual stack support to CAPG.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
#1486
#478
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: