IPV6 APIVIP Handling #589
IPV6 APIVIP Handling #589
Conversation
|
Thanks for working on this @dbw7 - I recall @mchiappero was working on some similar changes in #513 so perhaps Marco can review to ensure those changes are also captured here As a next step beyond this, I think we'll need to add dual-stack support too (particularly for the management cluster use-case because I think Rancher only supports dual-stack, not yet single-stack ipv6) I guess that will require a couple of configuration changes:
|
|
@hardys We'd indeed want to add dual-stack support - in fact, the first iteration of @dbw7 work was on it. I pushed back on it for now since it's safer (especially with changes to definition file) to do this in multiple steps where the first one is to enable the IPv6 support and have it thoroughly tested (in SV too) before proceeding with dual-stack. |
RELEASE_NOTES.md
Outdated
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| ## General | |||
|
|
|||
| * Added support for IPV6 addresses in the Kubernetes 'apiVIP' field | |||
There was a problem hiding this comment.
| * Added support for IPV6 addresses in the Kubernetes 'apiVIP' field | |
| * Added support for IPv6 addresses in the Kubernetes 'apiVIP' field |
docs/building-images.md
Outdated
| @@ -261,7 +261,7 @@ kubernetes: | |||
| * `network` - Required for multi-node clusters, optional for single-node clusters; Defines the network configuration | |||
| for bootstrapping a cluster. | |||
| * `apiVIP` - Required for multi-node clusters, optional for single-node clusters; Specifies the IP address which | |||
| will serve as the cluster LoadBalancer, backed by MetalLB. | |||
| will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPV4 and IPV6 addresses. | |||
There was a problem hiding this comment.
| will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPV4 and IPV6 addresses. | |
| will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPv4 and IPv6 addresses. |
pkg/combustion/kubernetes.go
Outdated
| @@ -316,11 +317,18 @@ func (c *Combustion) downloadRKE2Artefacts(ctx *image.Context, cluster *kubernet | |||
| } | |||
|
|
|||
| func kubernetesVIPManifest(k *image.Kubernetes) (string, error) { | |||
| ip, err := netip.ParseAddr(k.Network.APIVIP) | |||
| if err != nil { | |||
| return "", fmt.Errorf("parsing kubernetes APIVIP address: %w", err) | |||
There was a problem hiding this comment.
I feel like this should read as apiVIP instead of all caps, since that's what the field is named in the definition. But I don't feel particularly strongly if you decide to keep it like this.
pkg/kubernetes/cluster.go
Outdated
| setMultiNodeConfigDefaults(kubernetes, serverConfig) | ||
| ip, err := netip.ParseAddr(kubernetes.Network.APIVIP) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", err) |
There was a problem hiding this comment.
Same as above. It's an error message and the user-friendly output shows apiVIP, so whatever you decide just be consistent between this and the previous message I commented on.
| Error: err, | ||
| }) | ||
| } | ||
| if !parsedIP.Is4() && !parsedIP.Is6() { |
There was a problem hiding this comment.
These two checks are redundant, when the address is parsed successfully it's either IPv4 or IPv6. And would also not pass the following IsGlobalUnicast() test anyway.
| @@ -1084,192 +1079,90 @@ func TestValidateAdditionalArtifacts(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestValidateNodes(t *testing.T) { | |||
| func TestValidateNetwork(t *testing.T) { | |||
There was a problem hiding this comment.
Hey Danial, glad to see that most of my code has been reused :)
I saw you re-wrote and simplified this function, which on the one hand is good, but ends having a much smaller coverage. The one I wrote was carefully calibrated around the IsGlobalUnicast() check (and internal sub-checks), so that the different cases of non-global unicast values were sufficiently tested. I think it make sense to include a couple of additional malformed IPs (e.g. 500.168.1.1), but I'd bring back the cases from my initial work as it's just a matter of copying and pasting.
There was a problem hiding this comment.
I'm not sure if I'll have time today to continue my review (by the way, sorry for being late), but in the meantime please have a look at my couple of comments.
Edit: I don't have anything to add. Sorry for not doing a review in a one shot, I wasn't sure I'd have had time but didn't want to hold things until next week, in case. It looks good to me and can't wait to see it included, but I'd address the two main comments right now rather than later.
| @@ -821,3 +821,46 @@ func TestConfigureKubernetes_SuccessfulRKE2ServerWithManifests(t *testing.T) { | |||
| assert.Contains(t, contents, "name: my-nginx") | |||
| assert.Contains(t, contents, "image: nginx:1.14.2") | |||
| } | |||
|
|
|||
| func TestKubernetesVIPManifestValidIPV4(t *testing.T) { | |||
There was a problem hiding this comment.
Nit-picking, but I'd align these two names from "IPV4"/"IPV6" to IPv4 too, as the rest of the code/prints.
No description provided.