CLOUDSTACK-10177: Only pass IPv6 address to Security Group Python scr…#2355
Conversation
…ipt if present Otherwise we send down a 'null' to a ProcessBuilder in Java instead of a String and this causes a NPE. We should check first if the Instance has a IPv6 address before sending it there. Signed-off-by: Wido den Hollander <wido@widodh.nl>
GabrielBrascher
left a comment
There was a problem hiding this comment.
Thanks, Wido. Code LGTM.
|
Hi, take a look here. Massive fixes to make 4.10 work. Includes this fix and other ipv6 mods. |
|
@GabrielBrascher @wido |
| cmd.add("--vmid", vmId); | ||
| cmd.add("--vmip", guestIP); | ||
| cmd.add("--vmip6", guestIP6); | ||
| if (StringUtils.isNotBlank(guestIP6)) { |
|
LGTM. |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1350 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1740)
|
|
@bwsw @GabrielBrascher @rhtyd : I am closing this one in favor of #2320 I will review that one |
|
@wido I found your commit eaffe3a in wido/ipv6-icmpv6all. Together with current PR, I believe, they make ipv6 in 4.10 functional, unfortunately your PR wido/ipv6-icmpv6all wasn't included in 4.10 by unknown reason. I suppose it's wise to keep current PR, because together:
they do all necessary IPv6+sg work. But I still feel that in security_group.py code should include try-except: for ip in rule['ipv4']:
try:
if protocol == 'all':
execute('iptables -I ' + vmchain + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
elif protocol != 'icmp':
execute('iptables -I ' + vmchain + ' -p ' + protocol + ' -m ' + protocol + ' --dport ' + range + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
else:
execute('iptables -I ' + vmchain + ' -p icmp --icmp-type ' + range + ' ' + direction + ' ' + ip + ' -j ' + action)
except:
pass
for ip in rule['ipv6']:
try:
if protocol == 'all':
execute('ip6tables -I ' + vmchain + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
elif 'icmp' != protocol:
execute('ip6tables -I ' + vmchain + ' -p ' + protocol + ' -m ' + protocol + ' --dport ' + range + ' -m state --state NEW ' + direction + ' ' + ip + ' -j ' + action)
else:
if range == 'any':
execute('ip6tables -I ' + vmchain + ' -p icmpv6 ' + direction + ' ' + ip + ' -j ' + action)
else:
execute('ip6tables -I ' + vmchain + ' -p icmpv6 --icmpv6-type ' + range + ' ' + direction + ' ' + ip + ' -j ' + action)
except:
passat least because user can pass wrong icmp type/code and we should handle it at least skipping but not failing all security group configuration. So if you could improve current PR with try-catch I think it's more reasonable than diving into: #2320 as it includes several cherry-picks which already in master and it is quite messy and I believe it shouldn't be merged to master. Just for those who willing to make 4.10 work. |
|
@wido this is still open, are you closing this? |
|
@wido I agree with you and don't mind at all. I published that PR not for inclusion but to help people who would like to get 4.10 up and running asap. |
|
Ok @bwsw! We should then merge this one and have a few separate PRs which go into master to fix it for 4.11. How does that sound? I will send another PR for the try/except |
|
@wido Great! |
|
LGTM, merging. All errors seen are intermittent and env related. |
…ipt if present
Otherwise we send down a 'null' to a ProcessBuilder in Java instead of a String and this
causes a NPE.
We should check first if the Instance has a IPv6 address before sending it there.
Signed-off-by: Wido den Hollander wido@widodh.nl