Skip to content

server: apply network ACL even if there is no network ACLs rules in the ACL list#9374

Merged
DaanHoogland merged 1 commit intoapache:4.19from
weizhouapache:4.19-apply-network-acl-no-rules
Sep 24, 2024
Merged

server: apply network ACL even if there is no network ACLs rules in the ACL list#9374
DaanHoogland merged 1 commit intoapache:4.19from
weizhouapache:4.19-apply-network-acl-no-rules

Conversation

@weizhouapache
Copy link
Copy Markdown
Member

@weizhouapache weizhouapache commented Jul 11, 2024

Description

This PR fixes an issue with network ACL

  • create a new ACL list (without any rule) in a VPC
  • check current iptables rules in VPC VR
  • replace VPC tier with the new ACL list

expected:

  • the iptables rules are replaced (to same rules as default_deny)

actual:

  • the iptables rules are not changed.
  • in management-server.log, it shows
DEBUG [c.c.n.v.NetworkACLManagerImpl]  Applying NetworkACL for network: 517 with Network ACL service provider
DEBUG [o.a.c.n.t.BasicNetworkTopology] No network ACLs to be applied for network 517

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)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Jul 11, 2024

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jul 11, 2024

@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 11, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 4.27%. Comparing base (32cc1d4) to head (7c791e6).
Report is 81 commits behind head on 4.19.

❗ There is a different number of reports uploaded between BASE (32cc1d4) and HEAD (7c791e6). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (32cc1d4) HEAD (7c791e6)
unittests 1 0
Additional details and impacted files 
@@             Coverage Diff              @@
##               4.19   #9374       +/-   ##
============================================
- Coverage     15.07%   4.27%   -10.81%     
============================================
  Files          5405     365     -5040     
  Lines        472672   29505   -443167     
  Branches      58189    5161    -53028     
============================================
- Hits          71257    1261    -69996     
+ Misses       393486   28101   -365385     
+ Partials       7929     143     -7786
Flag Coverage Δ
uitests 4.27% <ø> (ø)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jul 11, 2024

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 10328

@weizhouapache
Copy link
Copy Markdown
Member Author

weizhouapache commented Jul 11, 2024

@blueorangutan test rocky8 kvm-rocky8

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jul 11, 2024

@weizhouapache a [SL] Trillian-Jenkins test job (rocky8 mgmt + kvm-rocky8) has been kicked to run smoke tests

@DaanHoogland
Copy link
Copy Markdown
Contributor

DaanHoogland commented Jul 12, 2024

@weizhouapache there are unit test failures: https://github.com/apache/cloudstack/actions/runs/9893593560/job/27359418049?pr=9374#step:7:9300 can you have a look?

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jul 12, 2024

[SF] Trillian test result (tid-10806)
Environment: kvm-rocky8 (x2), Advanced Networking with Mgmt server r8
Total time taken: 50046 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9374-t10806-kvm-rocky8.zip
Smoke tests completed. 132 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

DaanHoogland
DaanHoogland approved these changes Jul 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

@weizhouapache weizhouapache changed the title server: apply network ACL even if there is no network ACLs in the ACL list server: apply network ACL even if there is no network ACLs rules in the ACL list Jul 22, 2024
@weizhouapache weizhouapache added this to the 4.19.2.0 milestone Aug 20, 2024
@kiranchavala kiranchavala self-assigned this Sep 23, 2024
kiranchavala
kiranchavala approved these changes Sep 24, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@kiranchavala kiranchavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

  1. Create a vpc network
  2. Create a tier with default_deny acl rule
  3. create a new acl rule with no rules
  4. Replace the acl rule for the tier with the acl rule created in step 3

Before fix


2024-09-24T05:59:27,953 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-36:[ctx-89b40bfe, job-46, ctx-65267911]) (logid:948cbd28) New network ACL is empty. Revoke existing rules before applying ACL
2024-09-24T05:59:27,957 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-36:[ctx-89b40bfe, job-46, ctx-65267911]) (logid:948cbd28) Updated network: 205 with Network ACL Id: 4, Applying ACL items
2024-09-24T05:59:27,963 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-36:[ctx-89b40bfe, job-46, ctx-65267911]) (logid:948cbd28) Applying NetworkACL for network: 205 with Network ACL service provider
2024-09-24T05:59:27,974 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-36:[ctx-89b40bfe, job-46, ctx-65267911]) (logid:948cbd28) Complete async job-46, jobStatus: SUCCEEDED, resultCode: 0, result: org.apache.cloudstack.api.response.SuccessResponse/null/{"success":"true"}

After fix

2024-09-24 05:53:06,552 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-29:ctx-77843697 job-36 ctx-87af95cc) (logid:c829616e) New network ACL is empty. Revoke existing rules before applying ACL
2024-09-24 05:53:06,562 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-29:ctx-77843697 job-36 ctx-87af95cc) (logid:c829616e) Releasing 2 Network ACL Items for network id=204
2024-09-24 05:53:06,564 DEBUG [c.c.n.v.NetworkACLManagerImpl] (API-Job-Executor-29:ctx-77843697 job-36 ctx-87af95cc) (logid:c829616e) Applying NetworkACL for network: 204 with Network ACL service provider
2024-09-24 05:53:06,586 DEBUG [o.a.c.f.j.i.AsyncJobManagerImpl] (API-Job-Executor-29:ctx-77843697 job-36 ctx-87af95cc) (logid:c829616e) Complete async job-36, jobStatus: SUCCEEDED, resultCode: 0, result: org.apache.cloudstack.api.response.SuccessResponse/null/{"success":"true"}

@DaanHoogland DaanHoogland merged commit 2398b5c into apache:4.19 Sep 24, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Oct 14, 2024
@weizhouapache @dhslove
server: apply network ACL even if there is no network ACLs in the ACL…
d5cb8b2 
… list (apache#9374)
weizhouapache added a commit to weizhouapache/cloudstack that referenced this pull request Jan 22, 2025
@weizhouapache
server: apply network ACL even if there is no network ACLs in the ACL…
a1daf60 
… list (apache#9374)
@weizhouapache weizhouapache mentioned this pull request Jan 22, 2025
Merged
14 tasks
@DaanHoogland DaanHoogland deleted the 4.19-apply-network-acl-no-rules branch January 22, 2025 15:17
DaanHoogland pushed a commit that referenced this pull request May 1, 2025
@weizhouapache
server: apply network ACL even if there is no network ACLs in the ACL…
fe77f5f 
… list (#9374) (#10241)
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Jun 19, 2025
@weizhouapache @dhslove
server: apply network ACL even if there is no network ACLs in the ACL…
4941d6e 
… list (apache#9374) (apache#10241)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes

@kiranchavala kiranchavala kiranchavala approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

@sureshanaparti sureshanaparti Awaiting requested review from sureshanaparti

Assignees

@kiranchavala kiranchavala

Labels

component:networking

Projects

None yet

Milestone

4.19.2

Development

Successfully merging this pull request may close these issues.

5 participants

@weizhouapache @blueorangutan @DaanHoogland @yadvr @kiranchavala