-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Prevent deploying IPv6 network if Zone has no IPv6 DNS configured #4177
Conversation
If you have a IPv6 enabled network and you haven't specified the IPv6 DNS 1 and DNS 2 under the zone it causes dnsmasq inside the Virtual Router not to start
84a0250
to
ce658ac
Compare
Fix logic on checkIp6Parameters
362c064
to
e060158
Compare
@@ -3781,7 +3781,7 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId, | |||
ipv4 = true; | |||
} | |||
|
|||
if (startIPv6 != null) { | |||
if (vlanIp6Cidr != null) { |
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.
CloudStack uses SLAAC for managing IPv6 ranges, therefore it is not necessary to have a start/end IPv6 address; on the other hand, IPv6 CIDR is mandatory for IPv6 networks.
That is why I changed this and other pieces of code that had start/end ipv6 address as mandatory.
@@ -1195,7 +1195,7 @@ public Network createGuestNetwork(CreateNetworkCmd cmd) throws InsufficientCapac | |||
if (startIP != null) { | |||
ipv4 = true; | |||
} | |||
if (startIPv6 != null) { | |||
if (isNotBlank(ip6Cidr) && isNotBlank(ip6Gateway)) { |
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.
Due to SLAAC implementation, an IPv6 network needs CIDR and Gateway. With the refactored code a network is "marked" as ipv6 if IPv6 CIDR and Gateway are not null.
@@ -90,6 +91,11 @@ | |||
private static final long PHYSICAL_NETWORK_1_ID = 1L; | |||
private static final long PHYSICAL_NETWORK_2_ID = 2L; | |||
|
|||
private static final String IPV6_CIDR = "fd59:16ba:559b:243d::/64"; |
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.
Nowadays IPv6 networking support only /64
CIDR. Added Unit tests covering this.
Fixed conflict with the master. |
@blueorangutan package |
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2081 |
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.
code looks good and makes functional sense. are integration tests for this sensible/possible? (not playing down on the importance of the unit tests you created)
@blueorangutan package |
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2094 |
@blueorangutan package |
@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✖centos7 ✖centos8 ✔debian. JID-2221 |
@blueorangutan package |
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.
LGTM
@DaanHoogland @rhtyd @Pearl1594 @sureshanaparti looking at the codebase for IPv6 there are still a lot of things to improve. I will leave some of these parts for future PRs as they require testing and so far I made manual tests specifically for the reported issue #4157. This PR is partially improving the IPv6, mostly avoiding some critical deployment failures due to a lack of ipv6 gateway/DNS. Unfortunately, most of our execution flow regarding IPv6 needs to be refactored, this current PR adds a small contribution on behalf of issue #3569, but mainly focused on fixing #4157. I can re-run a couple of manual tests just for the sake. For the (near) future, I think that we should have some IPv6 Marvin tests. I can look into contributing with it. |
@GabrielBrascher the issue this PR solves is marked for 4.13.2. do you want to have this merged on master anyway? |
c52730d
to
ee923d8
Compare
@DaanHoogland sounds good. Rebased/cherry-picked respective commits aiming branch 4.13. |
@blueorangutan package |
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2303 |
not sure if the ape still knows 4.13 but i think it should |
I am not sure as well. Travis seems to not work. Maybe due to the migration of java 8 to java 11 |
@DaanHoogland do you think that it would be better to aim this to 4.15 so we can run tests? |
@GabrielBrascher you can go for 4.14 as well (or just master) |
ee923d8
to
c52730d
Compare
@blueorangutan package |
@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2311 |
@blueorangutan test |
@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-3107)
|
Description
If an IPv6 enabled network is created but the Zone hasn't IPv6 DNS1 or DNS2 configured then dnsmasq inside the Virtual Router does not start.
This PRs adds validation that allows creating an IPv6 network only if the Zone has at least DNS1 or DNS2 configured.
Fixes: #4157
Types of changes