Skip to content
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

Infer subnet for node /128 IPv6 addresses #2338

Merged
merged 1 commit into from Jul 15, 2021

Conversation

jcaamano
Copy link
Contributor

- What this PR does and why is it needed
When node addresses are allocated through DHCPv6 IA_NA, they might get
a /128 prefix address. These node addresses are set on the gateway
router port as well. OVN routers infers on-link routes from subnet
information on the address itself. Since /128 addresses are not on a
subnet, cluster traffic is routed through the node gatway instead of
directly.

Note that OVN does not support learning routes from ICMPv6 route
advertisement and does not support adding on-link static routes.

This patch infers a subnet for the interface looking at the node
routing information and replaces the /128 prefix with the subnet
prefix.

- How to verify it
Added unit tests

- Description for the changelog
Infer subnet for /128 IPv6 addresses allocated through DHCPv6 IA_NA

@jcaamano
Copy link
Contributor Author

/assign @trozet @dcbw

thoughts?

@jcaamano jcaamano changed the title Infer subnet from node /128 IPv6 addresses Infer subnet for node /128 IPv6 addresses Jul 14, 2021
@jcaamano jcaamano marked this pull request as draft July 14, 2021 21:24
@dcbw
Copy link
Contributor

dcbw commented Jul 15, 2021

@jcaamano yeah I'd un-draft this one. Clever workaround. Can you make sure a bug is filed against OVN to handle on-link static routes? Listening to RAs gets complicated fast but could be a future enchancement too.

When node addresses are allocated through DHCPv6 IA_NA, they might get
a /128 prefix address. These node addresses are set on the gateway
router port as well. OVN routers infers on-link routes from subnet
information on the address itself. Since /128 addresses are not on a
subnet, cluster traffic is routed through the node gatway instead of
directly.

Note that OVN does not support learning routes from ICMPv6 route
advertisement and does not support adding on-link static routes.

This patch infers the subnet of the interface looking at the node
routing information and replaces the /128 prefix with the subnet
prefix.

Signed-off-by: Jaime Caamaño Ruiz <jcaamano@redhat.com>
@jcaamano jcaamano marked this pull request as ready for review July 15, 2021 07:10
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 53.155% when pulling 1db8f65 on jcaamano:ipv6-on-subnet into 60fde39 on ovn-org:master.

@jcaamano
Copy link
Contributor Author

/retest

1 similar comment
@jcaamano
Copy link
Contributor Author

/retest

@trozet
Copy link
Contributor

trozet commented Jul 15, 2021

@jcaamano yeah I'd un-draft this one. Clever workaround. Can you make sure a bug is filed against OVN to handle on-link static routes? Listening to RAs gets complicated fast but could be a future enchancement too.

@dcbw you supposed to be on PTO!

Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I was very confused on this PR for a while, but I think I understand now and lgtm. Feel free to confirm/deny if my understanding and thought process is correct:

  1. I didn't understand how with /128 prefix we didn't listen to RA for on link routes, but somehow OVN listened for gateway route from RA.
  2. Now I realize OVN doesn't listen to any RA, and we statically configure the gateway route for ipv6:
	// Add static routes in GR with gateway router as the default next hop.
	for _, nextHop := range l3GatewayConfig.NextHops {
		var allIPs string
		if utilnet.IsIPv6(nextHop) {
			allIPs = "::/0"
		} else {
			allIPs = "0.0.0.0/0"
		}
  1. Since we determine the things anyway from the host and manually configure them in OVN because it does not listen to RA, I dont see any reason determining the prefix from the host should be an issue either.

@trozet trozet merged commit 141f5d8 into ovn-org:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants