replace vm.getPrivateMacAddress() with nic.getMacAddress()

[soreana]

Description

We have discovered that when securitygroups firewall rules are created, they use the MAC address from VM's private_mac_address field. However, if a VM has multiple NICs, then the same MAC address is issued to securitygroup.py which in some cases caused a trouble for the firewall rules.

This patch makes Cloudstack use the NIC's MAC address instead of VM's private MAC address.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

[weizhouapache]

code lgtm
not tested yet

@soreana
can you describe what the issue is and how to reproduce the issue ?

[soreana]

code lgtm not tested yet

@soreana can you describe what the issue is and how to reproduce the issue ?

@phsm Can you elaborate a little bit more?

[phsm]

Hi Wei,

We have found this inconsistency while debugging some other issue that turned out to be unrelated to this piece of code.

So in short: there is no reproduce scenario, besides our suspicion that these lines may introduce bugs.

The generateRulesetCmd() is used to generate a set of firewall rules for a vNIC.
It currently uses the private_mac_address VM field which contains a single MAC address that I believe belongs to the latest vNIC of the VM.
Thus, it would be logical that this function should use the MAC address of the vNIC it is creating a firewall rule for.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

[DaanHoogland]

on first sight this looks good, but it raises more questions when you think of it. Most significantly,

1. Why is this implemented twice? Should this not be a single place that does this (and even a single SecurityGroupMnagerImpl instead of two.)
2. if we can not reproduce than how do we know this doesn´t have side effects or removes side effects that we need.
   I am a bit worried, but again, looks good on first sight.

[phsm]

on first sight this looks good, but it raises more questions when you think of it. Most significantly,

1. Why is this implemented twice? Should this not be a single place that does this (and even a single `SecurityGroupMnagerImpl` instead of two.)

2. if we can not reproduce than how do we know this doesn´t have side effects or removes side effects that we need.
   I am a bit worried, but again, looks good on first sight.

1. Not sure, perhaps legacy vs modern implementation? I'm not sure how to check which of these 2 files is actually used somewhere else.
2. Yes this is the tricky part. Perhaps @weizhouapache knows more of the mac address purpose in "security_group.py" script? He's AFAIK the last person who made major changes there (additional IP addresses for a VM feature).

[weizhouapache]

2. security_group.py"

@phsm @soreana
yes, the last major changes were made by me 3 years ago. see #3639

since there is no critical issue found by you, let's revisit after 4.18.0 release

[DaanHoogland]

@soreana @phsm @weizhouapache what is the status of this PR?

[phsm]

Well I still think that this is a potential bug, and needs to be fixed.

To recap: it always uses the first NIC MAC address while applying security groups on any NIC, not just the first one. But we couldn't find the reproduce scenario for it.

Not sure what else can be done for this PR to get it moving.

[soreana]

@soreana @phsm @weizhouapache what is the status of this PR?

@DaanHoogland We had an issue with the Ubuntu upgrade and our hypervisor's network configuration. We noticed that the Mac address sets incorrectly.

At first, we thought it is a CloudStack issue, but after further investigation, the root cause happened to be our hypervisor's network configuration. While we were checking the code, we noticed that this Mac address sets incorrectly, though we couldn't reproduce it.

[DaanHoogland]

@DaanHoogland We had an issue with the Ubuntu upgrade and our hypervisor's network configuration. We noticed that the Mac address sets incorrectly.

At first, we thought it is a CloudStack issue, but after further investigation, the root cause happened to be our hypervisor's network configuration. While we were checking the code, we noticed that this Mac address sets incorrectly, though we couldn't reproduce it.

Does that render this PR unnecessary?

[weizhouapache]

although I did not get any difference on the iptables/ebtables rules, I think this changes make sense.

[DaanHoogland]

in the DB I see vm_instance.private_mac_address sometimes being null, so this change makes sense. As we have a reproduction problem I say, let's merge and deal with problems as we reproduce them.