DaanHoogland commented on code in PR #14519:
URL: https://github.com/apache/cloudstack/pull/14519#discussion_r4490278778
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtNetworkRulesSystemVmCommandWrapper.java:
##########
@@ -39,7 +39,9 @@ public Answer execute(final NetworkRulesSystemVmCommand
command, final LibvirtCo
final LibvirtUtilitiesHelper libvirtUtilitiesHelper =
libvirtComputingResource.getLibvirtUtilitiesHelper();
final Connect conn =
libvirtUtilitiesHelper.getConnectionByVmName(command.getVmName());
- success =
libvirtComputingResource.configureDefaultNetworkRulesForSystemVm(conn,
command.getVmName());
+ // this command is only sent after a migration; the rules are
re-applied on the
+ // destination host, but the bridge firewall framework must not be
created there
Review Comment:
can you remove these comments or add them to a javadoc
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java:
##########
@@ -5493,6 +5493,13 @@ public boolean applyDefaultNetworkRules(final Connect
conn, final VirtualMachine
isFirstNic = false;
}
}
+ if (vm.getType() != VirtualMachine.Type.User) {
+ // none of the nics is security group enabled, but the system vm
may still share a bridge
+ // with security grouped instances, for instance when the shared
network and the public
+ // network use the same vlan. Program it into an existing bridge
firewall framework,
+ // without creating one.
+ configureDefaultNetworkRulesForSystemVm(conn, vm.getName(), true);
+ }
Review Comment:
these comments seem excessive. if they are relly needed please create a new
method and add the comments as javadoc/top-of-method comments
##########
scripts/vm/network/security_group.py:
##########
@@ -371,8 +371,15 @@ def refactor_ebtable_rules(vm_name):
logging.debug("Refactoring ebtables rules for vm " + vm_name + " is done")
return True
-def default_network_rules_systemvm(vm_name, localbrname):
+def default_network_rules_systemvm(vm_name, localbrname,
only_existing_framework=False):
bridges = get_bridges(vm_name)
+ if only_existing_framework:
+ # do not create the bridge firewall framework, only program the vm
into the bridges
+ # which already have one
Review Comment:
I think this can be reformulated as a pydoc (top-of-method) comment
##########
scripts/vm/network/security_group.py:
##########
@@ -1277,6 +1284,31 @@ def get_br_fw(brname):
return brfwname
+def has_fw_framework(brname):
+ try:
+ execute("iptables -n -L " + get_br_fw(brname))
+ except:
+ return False
+ return True
+
+
+def add_systemvm_rules_for_bridge(brname):
+ # The bridge firewall framework has just been created on this bridge. Any
system vm
+ # (router, secondary storage vm, console proxy) already attached to it has
no rules in
+ # the framework and would be dropped, so program them now.
Review Comment:
```suggestion
"""
The bridge firewall framework has just been created on this bridge. Any
system vm
(router, secondary storage vm, console proxy) already attached to it
has no rules in
the framework and would be dropped, so program them now.
"""
```
##########
scripts/vm/network/security_group.py:
##########
@@ -1277,6 +1284,31 @@ def get_br_fw(brname):
return brfwname
+def has_fw_framework(brname):
+ try:
+ execute("iptables -n -L " + get_br_fw(brname))
+ except:
+ return False
+ return True
+
+
+def add_systemvm_rules_for_bridge(brname):
+ # The bridge firewall framework has just been created on this bridge. Any
system vm
+ # (router, secondary storage vm, console proxy) already attached to it has
no rules in
+ # the framework and would be dropped, so program them now.
+ try:
+ for vm_name in virshlist(['running', 'paused']):
+ if 1 not in [ vm_name.startswith(c) for c in ['r-', 's-', 'v-'] ]:
+ continue
+ if not get_vifs_for_bridge(vm_name, brname):
+ continue
+ logging.debug("Programming default rules on bridge " + brname + "
for system vm " + vm_name)
+ # the link local bridge never has a bridge firewall framework, so
it is skipped anyway
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
