You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "soreana (via GitHub)" <gi...@apache.org> on 2023/02/27 10:45:10 UTC

[GitHub] [cloudstack] soreana opened a new pull request, #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

soreana opened a new pull request, #7293:
URL: https://github.com/apache/cloudstack/pull/7293

   ### 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.
   
   <!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
   
   <!-- For new features, provide link to FS, dev ML discussion etc. -->
   <!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
   
   <!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
   <!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
   <!-- Fixes: # -->
   
   <!--- ********************************************************************************* -->
   <!--- NOTE: AUTOMATATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
   <!--- PLEASE PUT AN 'X' in only **ONE** box -->
   <!--- ********************************************************************************* -->
   
   ### Types of changes
   
   - [ ] Breaking change (fix or feature that would cause existing functionality to change)
   - [ ] New feature (non-breaking change which adds functionality)
   - [x] 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
   - [x] Minor
   - [ ] Trivial
   
   
   <!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] soreana commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "soreana (via GitHub)" <gi...@apache.org>.
soreana commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1634144293

   > @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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] soreana commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "soreana (via GitHub)" <gi...@apache.org>.
soreana commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446175938

   > 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?


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] phsm commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446191247

   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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446219431

   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.


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] weizhouapache commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446361825

   > 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
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1634371826

   > > 
   > 
   > @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?


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] weizhouapache merged pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache merged PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] phsm commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446229364

   > 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).


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] DaanHoogland commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1630845243

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


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] phsm commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "phsm (via GitHub)" <gi...@apache.org>.
phsm commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1631021574

   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. 


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [cloudstack] sonarcloud[bot] commented on pull request #7293: replace vm.getPrivateMacAddress() with nic.getMacAddress()

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #7293:
URL: https://github.com/apache/cloudstack/pull/7293#issuecomment-1446194195

   SonarCloud Quality Gate failed.&nbsp; &nbsp; [![Quality Gate failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/failed-16px.png 'Quality Gate failed')](https://sonarcloud.io/dashboard?id=apache_cloudstack&pullRequest=7293)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7293&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7293&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_cloudstack&pullRequest=7293&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=CODE_SMELL) [![E](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/E-16px.png 'E')](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_cloudstack&pullRequest=7293&resolved=false&types=CODE_SMELL)
   
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7293&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7293&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7293&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_cloudstack&pullRequest=7293&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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: commits-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org