CLOUDSTACK-10242: Properly parse incoming rules to Sec Group#2418
CLOUDSTACK-10242: Properly parse incoming rules to Sec Group#2418yadvr merged 1 commit intoapache:4.11from
Conversation
| return ret | ||
|
|
||
| lines = rules.split(';')[:-1] | ||
| lines = rules.split('NEXT;')[:-1] |
There was a problem hiding this comment.
@wido what is the NEXT here? Can you give a sample of the rule/commands processed by this code?
There was a problem hiding this comment.
I:icmp;-1;-1;0.0.0.0/0,NEXT;I:tcp;0;65535;0.0.0.0/0,NEXT;I:udp;0;65535;0.0.0.0/0,NEXT;
Sure, this is how it looks like. The NEXT word is the last word before a new rule/line starts
There was a problem hiding this comment.
I think this extremely ugly solution is acceptable but i see a possible problem with the preceeding , it spells ',NEXT;' could it be ',NEXT,', instead? and if so, why not just ','? (read at your own risk)
There was a problem hiding this comment.
I think having the , is fine, @DaanHoogland see line 967/968, the cidrs could be comma separated, so splitting on NEXT; looks okay. The NEXT separator is added in SecurityGroupRulesCmd.java
|
I've an outstanding question, other LGTM pending test results. |
|
@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-1665 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
I don't like the seperator but it is not user facing so fine
|
I agree with you @DaanHoogland that's it not very elegant. But the way we pass rules to the Python file isn't my favorite either. The 'NEXT' keyword was the only thing I could split on elegantly without running into troubles. As @rhtyd mentioned it's all done in SecurityGroupRulesCmd.java |
|
Trillian test result (tid-2178)
|
With merge of PR apache#2028 the separator for lines to the Security Group Python script changed from : to ; to support IPv6 addresses. This broke certain situations where rules were parsed improperly. This commit fixes the issue. Signed-off-by: Wido den Hollander <wido@widodh.nl> Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
yadvr
force-pushed
the
CLOUDSTACK-10242
branch
from
aa62a62 to
6eb878d
Compare
|
@wido please send bugfixes to 4.11 branch, this issue is marked blocker and should be send for 4.11 branch. I've rebased your branch, but pay attention to base-branch when sending PR in future, thanks. |
|
@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-1670 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2184)
|
|
LGTM, merging based on test results and reviews. The failing tests were caused by a failure in test_public_ip_range.py that introduces a fake public ip range used by systemvms, causing failure on download urls of template/iso/volumes. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2612 |
…ojects' into '4.20.0.0-scclouds' Extensão no formulário de adição de contas e usuários a projetos para permitir que contas do tipo `User` possam adicionar outros integrantes aos projetos Closes apache#2418 See merge request scclouds/scclouds!982
4 participants
With merge of PR #2028 the separator for lines to the Security Group
Python script changed from : to ; to support IPv6 addresses.
This broke certain situations where rules were parsed improperly.
This commit fixes that
Signed-off-by: Wido den Hollander wido@widodh.nl