diff --git a/hypershift-operator/controllers/nodepool/conditions.go b/hypershift-operator/controllers/nodepool/conditions.go index 735a21deb9c..6ae435dbc2a 100644 --- a/hypershift-operator/controllers/nodepool/conditions.go +++ b/hypershift-operator/controllers/nodepool/conditions.go @@ -599,7 +599,7 @@ func (r *NodePoolReconciler) setMachineAndNodeConditions(ctx context.Context, no r.setAllNodesHealthyCondition(nodePool, machines) - err = r.setCIDRConflictCondition(nodePool, machines, hc) + err = r.setCIDRConflictCondition(ctx, nodePool, machines, hc) if err != nil { return err } @@ -747,7 +747,13 @@ func (r *NodePoolReconciler) setAllMachinesReadyCondition(nodePool *hyperv1.Node SetStatusCondition(&nodePool.Status.Conditions, *allMachinesReadyCondition) } -func (r *NodePoolReconciler) setCIDRConflictCondition(nodePool *hyperv1.NodePool, machines []*capiv1.Machine, hc *hyperv1.HostedCluster) error { +type cidrConflictEntry struct { + ip string + cidr string +} + +func (r *NodePoolReconciler) setCIDRConflictCondition(ctx context.Context, nodePool *hyperv1.NodePool, machines []*capiv1.Machine, hc *hyperv1.HostedCluster) error { + log := ctrl.LoggerFrom(ctx) maxMessageLength := 256 if len(machines) < 1 || len(hc.Spec.Networking.ClusterNetwork) < 1 { @@ -755,25 +761,68 @@ func (r *NodePoolReconciler) setCIDRConflictCondition(nodePool *hyperv1.NodePool return nil } - clusterNetworkStr := hc.Spec.Networking.ClusterNetwork[0].CIDR.String() - clusterNetwork, err := netip.ParsePrefix(clusterNetworkStr) - if err != nil { - return err + var clusterNetworks []netip.Prefix + for _, cn := range hc.Spec.Networking.ClusterNetwork { + prefix, err := netip.ParsePrefix(cn.CIDR.String()) + if err != nil { + return err + } + clusterNetworks = append(clusterNetworks, prefix) } messages := []string{} for _, machine := range machines { + seen := make(map[string]struct{}) + var conflicting []cidrConflictEntry + hasAddressOutsideClusterNetwork := false + for _, addr := range machine.Status.Addresses { if addr.Type != capiv1.MachineExternalIP && addr.Type != capiv1.MachineInternalIP { continue } + ipaddr, err := netip.ParseAddr(addr.Address) if err != nil { return err } - if clusterNetwork.Contains(ipaddr) { - messages = append(messages, fmt.Sprintf("machine [%s] with ip [%s] collides with cluster-network cidr [%s]", machine.Name, addr.Address, clusterNetworkStr)) + key := ipaddr.String() + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + + if ipaddr.IsLinkLocalUnicast() || ipaddr.IsLinkLocalMulticast() { + continue + } + + matchedCIDR := "" + for _, cn := range clusterNetworks { + if cn.Contains(ipaddr) { + matchedCIDR = cn.String() + break + } + } + if matchedCIDR != "" { + conflicting = append(conflicting, cidrConflictEntry{ip: addr.Address, cidr: matchedCIDR}) + } else { + hasAddressOutsideClusterNetwork = true + } + } + + // When a machine has addresses both inside and outside the cluster network, + // the in-network addresses are typically CNI-internal (e.g. OVN-Kubernetes + // management port IPs on KubeVirt VMs) and do not represent a real conflict. + // Only report a conflict when ALL of a machine's addresses fall within the + // cluster network, which indicates the machine's infrastructure IP genuinely + // overlaps with the pod CIDR. + if !hasAddressOutsideClusterNetwork { + for _, entry := range conflicting { + messages = append(messages, fmt.Sprintf("machine [%s] with ip [%s] collides with cluster-network cidr [%s]", machine.Name, entry.ip, entry.cidr)) } + } else if len(conflicting) > 0 { + log.V(4).Info("Skipping CNI-internal addresses for CIDR conflict check", + "machine", machine.Name, + "suppressedCount", len(conflicting)) } } @@ -798,6 +847,8 @@ func (r *NodePoolReconciler) setCIDRConflictCondition(nodePool *hyperv1.NodePool ObservedGeneration: nodePool.Generation, } SetStatusCondition(&nodePool.Status.Conditions, *cidrConflictCondition) + } else { + removeStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolClusterNetworkCIDRConflictType) } return nil diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go index 4836adfc574..03dfc985fe8 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go @@ -2012,6 +2012,120 @@ func TestSetMachineAndNodeConditions(t *testing.T) { Messages: []string{hyperv1.AllIsWellMessage}, }, }, + { + name: "When machine has addresses both inside and outside cluster network it should not report cidr collision", + machinesGenerator: func() []client.Object { + return []client.Object{ + &capiv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + Namespace: "myns-cluster-name", + Annotations: map[string]string{ + nodePoolAnnotation: "myns/np-name", + }, + }, + Status: capiv1.MachineStatus{ + Conditions: []capiv1.Condition{ + { + Type: capiv1.ReadyCondition, + Status: corev1.ConditionTrue, + }, + { + Type: capiv1.MachineNodeHealthyCondition, + Status: corev1.ConditionTrue, + }, + }, + Addresses: capiv1.MachineAddresses{ + { + Type: capiv1.MachineInternalIP, + Address: "192.168.1.10", + }, + { + Type: capiv1.MachineExternalIP, + Address: "192.168.1.10", + }, + { + Type: capiv1.MachineInternalIP, + Address: "10.10.10.2", + }, + { + Type: capiv1.MachineExternalIP, + Address: "10.10.10.2", + }, + }, + }, + }, + } + }, + expectedAllMachine: &testCondition{ + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Messages: []string{hyperv1.AllIsWellMessage}, + }, + expectedAllNodes: &testCondition{ + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Messages: []string{hyperv1.AllIsWellMessage}, + }, + expectedCIDRCollision: nil, + }, + { + name: "When machine has out-of-cluster address alongside link-local and in-cluster addresses it should not report cidr collision", + machinesGenerator: func() []client.Object { + return []client.Object{ + &capiv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + Namespace: "myns-cluster-name", + Annotations: map[string]string{ + nodePoolAnnotation: "myns/np-name", + }, + }, + Status: capiv1.MachineStatus{ + Conditions: []capiv1.Condition{ + { + Type: capiv1.ReadyCondition, + Status: corev1.ConditionTrue, + }, + { + Type: capiv1.MachineNodeHealthyCondition, + Status: corev1.ConditionTrue, + }, + }, + Addresses: capiv1.MachineAddresses{ + { + Type: capiv1.MachineInternalIP, + Address: "192.168.1.10", + }, + { + Type: capiv1.MachineInternalIP, + Address: "169.254.0.2", + }, + { + Type: capiv1.MachineInternalIP, + Address: "fe80::1", + }, + { + Type: capiv1.MachineInternalIP, + Address: "10.10.10.2", + }, + }, + }, + }, + } + }, + expectedAllMachine: &testCondition{ + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Messages: []string{hyperv1.AllIsWellMessage}, + }, + expectedAllNodes: &testCondition{ + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Messages: []string{hyperv1.AllIsWellMessage}, + }, + expectedCIDRCollision: nil, + }, } { t.Run(tc.name, func(tt *testing.T) { gg := NewWithT(tt) @@ -2299,6 +2413,226 @@ func TestAggregateMachineReasonsAndMessages(t *testing.T) { } } +// TestSetCIDRConflictConditionDualStack tests setCIDRConflictCondition directly +// (unit-level isolation) while TestSetMachineAndNodeConditions above exercises it +// indirectly through the parent setMachineAndNodeConditions flow (integration-level). +// Both exist because the dual-stack and condition-clearing scenarios are easier to +// express with direct calls, while the integration cases verify end-to-end wiring. +func TestSetCIDRConflictConditionDualStack(t *testing.T) { + t.Parallel() + r := NodePoolReconciler{} + + for _, tc := range []struct { + name string + clusterNetwork []hyperv1.ClusterNetworkEntry + machines []*capiv1.Machine + preExistingCondition bool + expectConflict bool + expectMessageContains []string + }{ + { + name: "When all addresses are inside cluster networks it should report dual-stack collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + {Type: capiv1.MachineInternalIP, Address: "fd01::5"}, + }, + }, + }, + }, + expectConflict: true, + expectMessageContains: []string{"10.128.0.5", "10.128.0.0/14", "fd01::5", "fd01::/48"}, + }, + { + name: "When machine has address outside all cluster networks it should not report dual-stack collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineExternalIP, Address: "192.168.1.10"}, + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + {Type: capiv1.MachineInternalIP, Address: "fd01::5"}, + }, + }, + }, + }, + expectConflict: false, + }, + { + name: "When IPv4-only machine has address outside cluster network on dual-stack cluster it should not report collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineExternalIP, Address: "192.168.1.10"}, + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + }, + }, + }, + }, + expectConflict: false, + }, + { + name: "When IPv6-only address is in second CIDR it should detect dual-stack collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + {CIDR: *ipnet.MustParseCIDR("fd01::/48")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineInternalIP, Address: "fd01::a"}, + }, + }, + }, + }, + expectConflict: true, + expectMessageContains: []string{"fd01::a", "fd01::/48"}, + }, + { + name: "When machine has only link-local and in-cluster addresses it should report cidr collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineInternalIP, Address: "169.254.0.2"}, + {Type: capiv1.MachineInternalIP, Address: "fe80::1"}, + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + }, + }, + }, + }, + expectConflict: true, + expectMessageContains: []string{"10.128.0.5", "10.128.0.0/14"}, + }, + { + name: "When machine has only link-local addresses it should not report cidr collision", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineInternalIP, Address: "169.254.0.2"}, + {Type: capiv1.MachineInternalIP, Address: "fe80::1"}, + }, + }, + }, + }, + expectConflict: false, + }, + { + name: "When previously-conflicting machine gains out-of-cluster address it should clear the cidr conflict condition", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineExternalIP, Address: "192.168.1.10"}, + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + }, + }, + }, + }, + preExistingCondition: true, + expectConflict: false, + }, + { + name: "When multiple machines have mixed conflict states it should only report the conflicting one", + clusterNetwork: []hyperv1.ClusterNetworkEntry{ + {CIDR: *ipnet.MustParseCIDR("10.128.0.0/14")}, + }, + machines: []*capiv1.Machine{ + { + ObjectMeta: metav1.ObjectMeta{Name: "conflicting-node"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineInternalIP, Address: "10.128.0.5"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "healthy-node"}, + Status: capiv1.MachineStatus{ + Addresses: capiv1.MachineAddresses{ + {Type: capiv1.MachineExternalIP, Address: "192.168.1.10"}, + {Type: capiv1.MachineInternalIP, Address: "10.128.0.6"}, + }, + }, + }, + }, + expectConflict: true, + expectMessageContains: []string{"conflicting-node", "10.128.0.5"}, + }, + } { + t.Run(tc.name, func(tt *testing.T) { + tt.Parallel() + gg := NewWithT(tt) + np := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: "np-ds", Namespace: "myns"}, + } + if tc.preExistingCondition { + np.Status.Conditions = []hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolClusterNetworkCIDRConflictType, + Status: corev1.ConditionTrue, + Reason: hyperv1.InvalidConfigurationReason, + }, + } + } + hc := &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Networking: hyperv1.ClusterNetworking{ + ClusterNetwork: tc.clusterNetwork, + }, + }, + } + + err := r.setCIDRConflictCondition(tt.Context(), np, tc.machines, hc) + gg.Expect(err).ToNot(HaveOccurred()) + + cond := FindStatusCondition(np.Status.Conditions, hyperv1.NodePoolClusterNetworkCIDRConflictType) + if tc.expectConflict { + gg.Expect(cond).ToNot(BeNil(), "expected CIDR conflict condition to be set") + gg.Expect(cond.Status).To(Equal(corev1.ConditionTrue)) + for _, want := range tc.expectMessageContains { + gg.Expect(cond.Message).To(ContainSubstring(want)) + } + } else { + gg.Expect(cond).To(BeNil(), "expected no CIDR conflict condition") + } + }) + } +} + func newKVInfraMapMock(objects []client.Object) kvinfra.KubevirtInfraClientMap { return kvinfra.NewMockKubevirtInfraClientMap( fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(objects...).Build(),