Skip to content

Preventing port 53 being added as lb rule when dns service is availab… #4401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Spaceman1984 wants to merge 2 commits into from
Closed

Preventing port 53 being added as lb rule when dns service is availab… #4401

Spaceman1984 wants to merge 2 commits into from

Conversation

@Spaceman1984
Copy link
Contributor

@Spaceman1984 Spaceman1984 commented Oct 13, 2020

…le in network offering

Description

Throwing an error when port 53 is added to a load balancer when DNS is available on the network service offering.

Fixes: #4285

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)

Screenshots (if appropriate):

How Has This Been Tested?

Try to add port 53 as a public port as a load balancer rule, an error will be returned.

@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Oct 13, 2020

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Oct 13, 2020

@Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Oct 13, 2020

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2165

@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Oct 13, 2020

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Oct 13, 2020

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

@blueorangutan
Copy link

blueorangutan commented Oct 13, 2020

Trillian test result (tid-2928)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 36946 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4401-t2928-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 83 look OK, 2 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 315.19 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 303.30 test_vpc_redundant.py
test_hostha_kvm_host_fencing Error 99.19 test_hostha_kvm.py

@Spaceman1984 Spaceman1984 reopened this Oct 14, 2020
@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Oct 14, 2020

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Oct 14, 2020

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

weizhouapache
weizhouapache reviewed Oct 14, 2020
View reviewed changes
server/src/main/java/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java Outdated
IpAddress systemIp = null;
NetworkOffering off = _entityMgr.findById(NetworkOffering.class, network.getNetworkOfferingId());

if (srcPortStart == 53) {
Copy link
Member

@weizhouapache weizhouapache Oct 14, 2020

Choose a reason for hiding this comment

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

@Spaceman1984
(1) the issue happens only if lb uses source nat IP. so IP needs to be checked
(2) what about port 8081 which is used for haproxy stats ?

Copy link
Contributor Author

@Spaceman1984 Spaceman1984 Oct 14, 2020

Choose a reason for hiding this comment

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

I'll look into it @weizhouapache.

Copy link
Contributor Author

@Spaceman1984 Spaceman1984 Oct 14, 2020

Choose a reason for hiding this comment

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

I tested port 8081 and it threw an error, seems like there is already some code somewhere that checks for certain ports, I'll see if I should move my code there as well.

Copy link
Contributor Author

@Spaceman1984 Spaceman1984 Oct 14, 2020

Choose a reason for hiding this comment

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

I think the code is fine where it is, I've added a check for Source NAT.

@Spaceman1984 Spaceman1984 marked this pull request as draft October 14, 2020 08:34
@Spaceman1984 Spaceman1984 marked this pull request as ready for review October 14, 2020 10:39
@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Oct 14, 2020

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Oct 14, 2020

@Spaceman1984 a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Oct 14, 2020

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2176

@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Oct 14, 2020

@blueorangutan test

@blueorangutan
Copy link

blueorangutan commented Oct 14, 2020

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

@blueorangutan
Copy link

blueorangutan commented Oct 14, 2020

Trillian test result (tid-2941)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35646 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4401-t2941-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_volumes.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_hostha_kvm_host_fencing Error 168.08 test_hostha_kvm.py

@PaulAngus PaulAngus added this to the 4.15.0.0 milestone Oct 29, 2020
weizhouapache
weizhouapache approved these changes Nov 2, 2020
View reviewed changes
Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

lgtm
would it be better to fix it in 4.14 as well ?

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Nov 4, 2020

@Spaceman1984 can you rebase on 4.14?

@DaanHoogland DaanHoogland modified the milestones: 4.15.0.0, 4.14.1.0 Nov 4, 2020
@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Nov 4, 2020

Sure @DaanHoogland, I'll try to get to it later today.

@Spaceman1984
Copy link
Contributor Author

Spaceman1984 commented Nov 5, 2020

Closing in favor of #4442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@weizhouapache weizhouapache weizhouapache approved these changes

@DaanHoogland DaanHoogland Awaiting requested review from DaanHoogland

Assignees

No one assigned

Labels

type:bug

Projects

None yet

Milestone

4.14.1.0

Development

Successfully merging this pull request may close these issues.

Set 53/111 LoadBalancer rule may kill haproxy and cause loadbalancer down completely.

5 participants

@Spaceman1984 @blueorangutan @DaanHoogland @weizhouapache @PaulAngus