Repository: openshift/assisted-service (language: python)

Issue:
OCPBUGS-31631: Deploy dual stack with IPv6 on top of bond/vlan fails
IPv6 L2 connectivity check uses nmap.  nmap uses NDP (Network Discovery Protocol) to check L2 connectivty to IPv6 hosts. There are cases that NDP response does not arrive or it is not captured by nmap which means that the system assumes there is no connectivity to the host.  However, there might be L3 connectivity (ping) to the same host. So change adds fallback for IPv6 L2 connectivity.  In case there is failure in L2 connectivity to IPv6 host and success in L3 connecivity to the same host, the host is assumed connected on L2, providing that the remote address is part of the L2 subnet being checked.

<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.

Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
information on how to link a PR to an existing issue.

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

- Should this PR be tested by the reviewer?
- Is this PR relying on CI for an e2e test run?
- Should this PR be tested in a specific environment?
- Any logs, screenshots, etc that can help with the review process?

-->

## List all the issues related to this PR

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [x] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [ ] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [x] No tests needed

## Checklist

- [ ] Title and description added to both, commit and PR.
- [ ] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [ ] This change does not require a documentation update (docstring, `docs`, README, etc)
- [ ] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?

[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md
/cc @tsorya 



Discussion / hints:

Fix patch:
diff --git a/internal/network/connectivity_groups.go b/internal/network/connectivity_groups.go
index 7c6afead847..73cde7233a0 100644
--- a/internal/network/connectivity_groups.go
+++ b/internal/network/connectivity_groups.go
@@ -376,6 +376,11 @@ type l2Query struct {
 	connectivityReport models.ConnectivityReport
 }
 
+func (l *l2Query) isIPv6() bool {
+	_, bits := l.parsedCidr.Mask.Size()
+	return bits == net.IPv6len*8
+}
+
 func (l *l2Query) next() strfmt.UUID {
 	for l.current != len(l.connectivityReport.RemoteHosts) {
 		rh := l.connectivityReport.RemoteHosts[l.current]
@@ -386,6 +391,17 @@ func (l *l2Query) next() strfmt.UUID {
 				return rh.HostID
 			}
 		}
+		if l.isIPv6() {
+			// nmap uses NDP (Network Discovery Protocol) for IPv6 L2 discovery.  There are cases that NDP responses do not
+			// arrive and parsed by nmap, but still there is connectivity between the hosts.  In this case we fallback to L3
+			// check and verify that the remote address is in the required subnet.
+			for _, l3 := range rh.L3Connectivity {
+				ip := net.ParseIP(l3.RemoteIPAddress)
+				if ip != nil && l.parsedCidr.Contains(ip) && l3.Successful {
+					return rh.HostID
+				}
+			}
+		}
 	}
 	return ""
 }


Test patch:
diff --git a/internal/network/connectivity_groups_test.go b/internal/network/connectivity_groups_test.go
index 6204c4a386a..5625e1ef2dd 100644
--- a/internal/network/connectivity_groups_test.go
+++ b/internal/network/connectivity_groups_test.go
@@ -515,6 +515,55 @@ func GenerateL2ConnectivityGroupTests(ipV4 bool, net1CIDR, net2CIDR string) {
 	})
 }
 
+var _ = Describe("L2 Ipv6 with L3 fallback", func() {
+	var (
+		nodes    []*node
+		net1CIDR string
+		hosts    []*models.Host
+	)
+
+	BeforeEach(func() {
+		net1CIDR = "2001:db8::/120"
+		nodes = generateIPv6Nodes(7, "2001:db8::/120", "fe80:5054::/120")
+		hosts = []*models.Host{
+			{
+				ID: nodes[0].id,
+				Connectivity: createConnectivityReport(
+					createL2Remote(nodes[1], l2LinkNet1),
+					createL2Remote(nodes[2], l2LinkNet1)),
+			},
+			{
+				ID: nodes[1].id,
+				Connectivity: createConnectivityReport(
+					createL2Remote(nodes[0], l2LinkNet1),
+					createL2Remote(nodes[2], l2LinkNet1)),
+			},
+			{
+				ID: nodes[2].id,
+				Connectivity: createConnectivityReport(
+					createL2Remote(nodes[0], l2LinkNet1)),
+			},
+		}
+	})
+	It("Missing L2 connectivity", func() {
+		ret, err := CreateL2MajorityGroup(net1CIDR, hosts)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(ret).To(HaveLen(0))
+	})
+
+	It("Missing L2 connectivity. Add L3 connectivity", func() {
+		hosts[2].Connectivity = createConnectivityReport(
+			createL2Remote(nodes[0], l2LinkNet1),
+			createL3Remote(nodes[1], l3LinkNet1))
+		ret, err := CreateL2MajorityGroup(net1CIDR, hosts)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(ret).To(HaveLen(3))
+		Expect(ret).To(ContainElement(*nodes[0].id))
+		Expect(ret).To(ContainElement(*nodes[1].id))
+		Expect(ret).To(ContainElement(*nodes[2].id))
+	})
+})
+
 var _ = Describe("L3 connectivity groups all", func() {
 	GenerateL3ConnectivityGroupTests(true, "1.2.3.0/24", "2.2.3.0/24")
 	GenerateL3ConnectivityGroupTests(false, "2001:db8::/120", "fe80:5054::/120")