FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add new parameter to createLoadBalancerRule API by JoaoJandre · Pull Request #6460 · apache/cloudstack · GitHub

  1. apache
  2. cloudstack

Repository navigation

Add new parameter to createLoadBalancerRule API - #6460

Merged
DaanHoogland merged 3 commits into
apache:mainfrom
scclouds:LB-Configuration-for-VPCs
Aug 8, 2022
ConversationCommits (3)ChecksFiles changed
Merged

Add new parameter to createLoadBalancerRule API#6460
DaanHoogland merged 3 commits into
apache:mainfrom
scclouds:LB-Configuration-for-VPCs

Conversation

JoaoJandre commented Jun 15, 2022

Copy link
Copy Markdown
Contributor

Description

ACL rules are currently not enforced when a Load Balancer is used. When created, ACL rules are implemented in the VR in the FORWARD chain. In order for the LB to work only through the configured IPs, it is necessary to configure the rule in the INPUT chain. However, since all traffic into the VR is being allowed, only forwarding is blocked to the unconfigured IPs.

To work around this situation, it is possible to configure LB rules via createLoadBalancerRule API. However, the cidrlist parameter used to configure the CIDRs that can access the load balancer, had been deprecated in favor of the firewall rule implementation, which is not intuitive and does not support VRs in VPCs. Therefore, the parameter was reimplemented to allow users to create rules in the Load Balancer backend that restrict the access according to the source IP.

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)

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?

New unit tests were created. Alongside that, the changes were manually tested in a local lab. By using the createLoadBalancerRule API with the cidrlist parameter to restrict network traffic.

sonarqubecloud Bot commented Jun 15, 2022

Copy link
Copy Markdown

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

18.5% Coverage
0.0% Duplication

weizhouapache commented Jun 20, 2022

Copy link
Copy Markdown
Member

@JoaoJandre
there are many parameters supported by haproxy but not supported in cloudstack.
I suggest you to collaborate with @ravening on #5799

JoaoJandre commented Jun 20, 2022

Copy link
Copy Markdown
Contributor Author

@JoaoJandre there are many parameters supported by haproxy but not supported in cloudstack. I suggest you to collaborate with @ravening on #5799

I would rather move on with this PR only, as it is re-adding a feature that was deprecated/removed without a proper alternative. This PR is small, concise, unit tested, and ready to be used in production. The alternative you pointed out is a huge PR that looks a bit too complicated. Therefore, I would rather move on with this small addition to restore the API workings.
Anyways, I can note that PR here, and if we ever need such a featureset, we might be able to help there.

weizhouapache commented Jun 21, 2022

Copy link
Copy Markdown
Member

@JoaoJandre there are many parameters supported by haproxy but not supported in cloudstack. I suggest you to collaborate with @ravening on #5799

I would rather move on with this PR only, as it is re-adding a feature that was deprecated/removed without a proper alternative. This PR is small, concise, unit tested, and ready to be used in production. The alternative you pointed out is a huge PR that looks a bit too complicated. Therefore, I would rather move on with this small addition to restore the API workings. Anyways, I can note that PR here, and if we ever need such a featureset, we might be able to help there.

@JoaoJandre
I understand your want to have this feature in main branch as soon as possible.
however, in my opinion, it is not a good idea to add such minor haproxy setting to the load_balancing_rules table (and VO/TO). If we support more haproxy settings by this way, the table/VO/TO will be very big.

#5799 has milestone "4.18.0.0". Once it is merged, it will be much easier to add more haproxy setting like the one you added in this PR.

GutoVeronezi commented Jun 21, 2022

Copy link
Copy Markdown
Contributor

@weizhouapache, PR #5799 has being developed through 2 years from now (originally developed in PR #4141). It passed through 4.15, 4.16, and 4.17, and we do not have guarantee that will be merged in 4.18. Also, there is a lot of changes in #5799, which would require a good documentation and a lot of tests. Therefore, I would rather go with these small changes instead of blocking the feature for something we do not have guarantee.

github-actions Bot commented Jun 27, 2022

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

weizhouapache commented Jun 27, 2022

Copy link
Copy Markdown
Member

@weizhouapache, PR #5799 has being developed through 2 years from now (originally developed in PR #4141). It passed through 4.15, 4.16, and 4.17, and we do not have guarantee that will be merged in 4.18. Also, there is a lot of changes in #5799, which would require a good documentation and a lot of tests. Therefore, I would rather go with these small changes instead of blocking the feature for something we do not have guarantee.

@GutoVeronezi @JoaoJandre
as far as I know, #5799 is not beeing developed, tt has been deployed in their production for around two years.
It is ready for review and test. The reason it is not merged is, nobody in cloudstack community has reviewed and tested it.
I hope you guys can review/test it and collaborate with @ravening @soreana on merging #5799 and changes for cidrlist into 4.18

RodrigoDLopez commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

based on tests performed in our labs and code reviews. Lgtm.

soreana commented Jun 27, 2022

Copy link
Copy Markdown
Member

@weizhouapache
I have a suggestion for #5799. We can test it gradually, like what we did when we developed it. We can have a branch named HAproxy-LoadBalancer in the community. I will port all the changes from #5799 to HAproxy-LoadBalancer one by one. When satisfied with its quality, we can merge it to the main branch. That way, it would be far easier for community members to test it.

This feature could be part of it, too.

weizhouapache commented Jun 27, 2022

Copy link
Copy Markdown
Member

@weizhouapache I have a suggestion for #5799. We can test it gradually, like what we did when we developed it. We can have a branch named HAproxy-LoadBalancer in the community. I will port all the changes from #5799 to HAproxy-LoadBalancer one by one. When satisfied with its quality, we can merge it to the main branch. That way, it would be far easier for community members to test it.

This feature could be part of it, too.

@soreana
Each PR can be merged only if there are at least 2 approvals and all smoke tests pass.
Most users use official release. If someone want to test a feature, they can build and test the specific branch, it is not needed to merge it into main.

JoaoJandre commented Aug 3, 2022

Copy link
Copy Markdown
Contributor Author

Hey @DaanHoogland could you review and run the tests?

DaanHoogland commented Aug 4, 2022

Copy link
Copy Markdown
Contributor

@blueorangutan package

blueorangutan commented Aug 4, 2022

Copy link
Copy Markdown

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

blueorangutan commented Aug 4, 2022

Copy link
Copy Markdown

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3900

DaanHoogland commented Aug 4, 2022

Copy link
Copy Markdown
Contributor

@blueorangutan test

blueorangutan commented Aug 4, 2022

Copy link
Copy Markdown

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland approved these changes Aug 4, 2022
View reviewed changes

DaanHoogland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

clgtm needs testing

blueorangutan commented Aug 4, 2022

Copy link
Copy Markdown

Trillian test result (tid-4618)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 37051 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6460-t4618-kvm-centos7.zip
Smoke tests completed. 101 look OK, 0 have errors
Only failed tests results shown below:

Test Result Time (s) Test File

acs-robot commented Aug 5, 2022

Copy link
Copy Markdown

Found UI changes, kicking a new UI QA build
@blueorangutan ui

blueorangutan commented Aug 5, 2022

Copy link
Copy Markdown

@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

blueorangutan commented Aug 5, 2022

Copy link
Copy Markdown

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6460 (SL-JID-2086)

sonarqubecloud Bot commented Aug 5, 2022 •
edited
Loading

Copy link
Copy Markdown

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

18.5% Coverage
0.0% Duplication

DaanHoogland merged commit 9c63c39 into apache:main Aug 8, 2022
neogismm pushed a commit to neogismm/cloudstack that referenced this pull request Sep 5, 2022
Add new parameter to createLoadBalancerRule API (apache#6460)
caf553a 
* Add new parameter to createLoadBalancerRule API

* address review

Co-authored-by: João Paraquetti <joao@scclouds.com.br>
JoaoJandre mentioned this pull request Nov 3, 2022
Merged
2 of 7 tasks
GutoVeronezi mentioned this pull request Jun 10, 2024
Closed
GutoVeronezi mentioned this pull request Jun 28, 2024
Closed
weizhouapache mentioned this pull request Jun 7, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DaanHoogland DaanHoogland approved these changes
+1 more reviewer
RodrigoDLopez RodrigoDLopez approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:api component:networking

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL