vpc: set traffic type of private gateway IP to Public to fix keepalived misconfiguration by ustcweizhou · Pull Request #3851 · apache/cloudstack

ustcweizhou · 2020-01-29T13:29:15Z

Description

In VPC, the private gateway has traffic type as Guest not Public.
This leads to keepalived misconfiguration in VPC with redundant VRs.

There are some commits (eg 65cb222 and f4f9b3a) for this issue, however the issue still exists in 4.13 and 4.14.

Simply setting the traffic type of ip address (of private gateway) to Public fixes the issue.

Fixes: #3402

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?

Before change,

In VPC master VR (redundant), IP of private gateway is added in keepalived, and gateway of private gateway is also added to ethX which is obviously wrong.
nw_type of ethX is 'guest' in /etc/cloudstack/ips.json

root@r-181-VM:~# grep virtual_ipaddress /etc/keepalived/keepalived.conf -A5
virtual_ipaddress {
10.11.112.254/24 brd 10.11.112.255 dev eth6
192.168.0.2/27 brd 192.168.0.31 dev eth5
192.168.0.129/26 brd 192.168.0.191 dev eth4
192.168.0.65/26 brd 192.168.0.127 dev eth3
}
8: eth6: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
link/ether 1e:00:65:00:01:ef brd ff:ff:ff:ff:ff:ff
inet 10.11.112.10/24 brd 10.11.112.255 scope global eth6
valid_lft forever preferred_lft forever
inet 10.11.112.254/24 brd 10.11.112.255 scope global secondary eth6
valid_lft forever preferred_lft forever
"eth6": [
{
"add": true,
"broadcast": "10.11.112.255",
"cidr": "10.11.112.10/24",
"device": "eth6",
"first_i_p": false,
"gateway": "10.11.112.254",
"netmask": "255.255.255.0",
"network": "10.11.112.0/24",
"new_nic": false,
"nic_dev_id": 6,
"nw_type": "guest",
"one_to_one_nat": false,
"public_ip": "10.11.112.10",
"size": "24",
"source_nat": false,
"vif_mac_address": "1e:00:65:00:01:ef"
}
],

After change, private gateway IP is not added in keepalived, gateway is not added to ethX.
nw_type of ethX is 'public' in /etc/cloudstack/ips.json

root@r-187-VM:~# grep virtual_ipaddress /etc/keepalived/keepalived.conf -A5
virtual_ipaddress {
192.168.0.2/27 brd 192.168.0.31 dev eth5
192.168.0.129/26 brd 192.168.0.191 dev eth4
192.168.0.65/26 brd 192.168.0.127 dev eth3
}
8: eth6: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
link/ether 1e:00:f2:00:01:ef brd ff:ff:ff:ff:ff:ff
inet 10.11.112.10/24 brd 10.11.112.255 scope global eth6
valid_lft forever preferred_lft forever
"eth6": [
{
"add": true,
"broadcast": "10.11.112.255",
"cidr": "10.11.112.10/24",
"device": "eth6",
"first_i_p": false,
"gateway": "10.11.112.254",
"netmask": "255.255.255.0",
"network": "10.11.112.0/24",
"new_nic": false,
"nic_dev_id": 6,
"nw_type": "public",
"one_to_one_nat": false,
"public_ip": "10.11.112.10",
"size": "24",
"source_nat": false,
"vif_mac_address": "1e:00:f2:00:01:ef"
}
],

…ed misconfiguration

DaanHoogland · 2020-01-30T16:47:37Z

code lgtm, I do not oversee the logic from an operations point of view. I think extensive test is needed for this one.
Can you comment @weizhouapache ?
What I mean is, is there some reliance on the vpc-pg being of type guest elsewhere in the code?
And do we need to have keepalive guard on a private gateway in general?
tnx

GabrielBrascher · 2020-01-30T18:31:26Z

+ private TrafficType getNetworkTrafficType(Network network) {
+ final VpcGatewayVO gateway = _vpcGatewayDao.getVpcGatewayByNetworkId(network.getId());
+ if (gateway != null) {
+ s_logger.debug("network " + network.getId() + " is a vpc private gateway, set traffic type to Public");


@ustcweizhou what do you think of adding also the network name? Sometimes it annoys me to see a lot of IDs on the log and then go after a few selects to understand what is really going on.
Having the network ID and name would be great for debugging :-)

@GabrielBrascher added a new commit per your comment.

weizhouapache · 2020-01-30T19:40:48Z

code lgtm, I do not oversee the logic from an operations point of view. I think extensive test is needed for this one.
Can you comment @weizhouapache ?
What I mean is, is there some reliance on the vpc-pg being of type guest elsewhere in the code?

@DaanHoogland the network type is intended to changed to public in commit
f4f9b3a

however, it does not work, for unknown reasons.

And do we need to have keepalive guard on a private gateway in general?

definitely no.

The network corresponding to a private gateway is "Guest" network in database.
But in keepalived configuration, we should not take it into consideration because it is not a vpc tier.

tnx

DaanHoogland · 2020-01-31T07:29:54Z

...

Can you comment @weizhouapache ?
...
And do we need to have keepalive guard on a private gateway in general?
definitely no.

meaning if a vpd on the pg fails we don't care?

The network corresponding to a private gateway is "Guest" network in database.
But in keepalived configuration, we should not take it into consideration because it is not a vpc tier.

Ok, I get that part, and than shouldn't we mark it public in the DB, or give it its own type? The idea of treating it as guest internally and as public on the VR somehow doesn't feel right.

This is by no means a disqualification of your fix, but a question about the design. Maybe, in the future we can implement a more permanent design??

weizhouapache · 2020-01-31T08:20:32Z

...
Can you comment @weizhouapache ?
...
And do we need to have keepalive guard on a private gateway in general?
definitely no.
meaning if a vpd on the pg fails we don't care?

@DaanHoogland the private gateway IP is configured in our VRs (Up in master VR and Down in backup VR). The gateway of private gateway is not configured in our VRs, it might be configured on some switches (of cloudstack platform or other platforms), similar as gateway of public IPs.

The network corresponding to a private gateway is "Guest" network in database.
But in keepalived configuration, we should not take it into consideration because it is not a vpc tier.
Ok, I get that part, and than shouldn't we mark it public in the DB, or give it its own type? The idea of treating it as guest internally and as public on the VR somehow doesn't feel right.
This is by no means a disqualification of your fix, but a question about the design. Maybe, in the future we can implement a more permanent design??

@DaanHoogland ideally there is a network type (eg PrivateGateway) defined for private gateway.
However, it seems to be a big change in java code and python code (in vr), which might bring other issues.
I prefer to fix an issue in easier and more stable way.

DaanHoogland · 2020-01-31T10:41:13Z

@blueorangutan test

blueorangutan · 2020-01-31T10:41:30Z

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

blueorangutan · 2020-01-31T20:07:19Z

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

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	215.77	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	223.86	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	316.50	test_privategw_acl.py

weizhouapache · 2020-02-03T08:20:18Z

@DaanHoogland @rhtyd the failed tests seem not to be related to this PR.
I have created an issue for it: #3859

yadvr · 2020-02-03T09:11:30Z

Let me rekick against vmware
@blueorangutan package

blueorangutan · 2020-02-03T09:11:42Z

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

blueorangutan · 2020-02-03T09:35:48Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-738

DaanHoogland · 2020-02-03T12:54:08Z

Any particular versions you are interested in @rhtyd ? or just
@blueorangutan test centos7 vmware-65u2

blueorangutan · 2020-02-03T12:54:43Z

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

blueorangutan · 2020-02-03T22:33:49Z

Trillian test result (tid-879)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33006 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3851-t879-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 76 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	287.13	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	273.46	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	429.85	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Error`	512.52	test_privategw_acl.py

GabrielBrascher

Thanks for the updates @ustcweizhou, code LGTM.

yadvr · 2020-02-06T05:52:56Z

@DaanHoogland @andrijapanicsb we'll need some manual testing to confirm the fix.
@blueorangutan package

blueorangutan · 2020-02-06T05:53:57Z

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

yadvr

LGTM, did not test it though. Does it require any changes in VR code?

yadvr · 2020-02-06T06:13:11Z

@ustcweizhou @weizhouapache cc @DaanHoogland @andrijapanicsb - for reference there was an old fix which basically introduced a new network type for private gateway, please refer https://github.com/apache/cloudstack/pull/2128/files

blueorangutan · 2020-02-06T06:17:32Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-762

yadvr · 2020-02-06T06:20:50Z

@blueorangutan test

blueorangutan · 2020-02-06T06:20:56Z

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

weizhouapache · 2020-02-06T06:29:22Z

@ustcweizhou @weizhouapache cc @DaanHoogland @andrijapanicsb - for reference there was an old fix which basically introduced a new network type for private gateway, please refer https://github.com/apache/cloudstack/pull/2128/files

@rhtyd I have read it before. Apparently it is not a full implementation. Many more changes in java code and python code for virtual router are needed

weizhouapache · 2020-02-06T06:37:08Z

LGTM, did not test it though. Does it require any changes in VR code?

no, the private gateway will be handled as a public ip in virtual router.

* 4.13: vr: add missing rule for port forwarding rule in vpc (#3857) vpc: set traffic type of private gateway IP to Public to fix ke… (#3851)

blueorangutan · 2020-02-07T00:41:14Z

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

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	253.14	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	258.43	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	384.27	test_privategw_acl.py

…che#3851)

vpc: set traffic type of private gateway IP to Public to fix keepaliv…
a7c9c52
…ed misconfiguration

svenvogel added component:vpc type:bug component:virtual-router labels Jan 29, 2020

DaanHoogland added this to the 4.13.1.0 milestone Jan 29, 2020

DaanHoogland requested review from andrijapanicsb and yadvr January 29, 2020 13:43

weizhouapache requested a review from svenvogel January 29, 2020 13:45

weizhouapache closed this Jan 30, 2020

weizhouapache reopened this Jan 30, 2020

apache deleted a comment from yadvr Jan 30, 2020

apache deleted a comment from blueorangutan Jan 30, 2020

GabrielBrascher reviewed Jan 30, 2020
View reviewed changes

Add network name in debugging log
15ed0d9

yadvr closed this Jan 31, 2020

yadvr reopened this Jan 31, 2020

apache deleted a comment from blueorangutan Jan 31, 2020

apache deleted a comment from yadvr Feb 3, 2020

apache deleted a comment from blueorangutan Feb 3, 2020

DaanHoogland approved these changes Feb 3, 2020
View reviewed changes

GabrielBrascher approved these changes Feb 4, 2020
View reviewed changes

yadvr approved these changes Feb 6, 2020
View reviewed changes

DaanHoogland merged commit a9a1737 into apache:4.13Feb 6, 2020

DaanHoogland added a commit that referenced this pull request Feb 6, 2020
Merge release branch 4.13 to master
10482da
* 4.13: vr: add missing rule for port forwarding rule in vpc (#3857) vpc: set traffic type of private gateway IP to Public to fix ke… (#3851)

ustcweizhou added a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
vpc: set traffic type of private gateway IP to Public to fix ke… (apa…
9954615
…che#3851)

ustcweizhou mentioned this pull request Nov 19, 2020
VPC: fix some issues related to multiple public IP ranges and private gateway #4484
Merged
10 tasks

Uh oh!

Conversation

ustcweizhou commented Jan 29, 2020

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

DaanHoogland commented Jan 30, 2020

Uh oh!

GabrielBrascherJan 30, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapacheJan 30, 2020

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Jan 30, 2020

Uh oh!

DaanHoogland commented Jan 31, 2020

Uh oh!

weizhouapache commented Jan 31, 2020

Uh oh!

DaanHoogland commented Jan 31, 2020

Uh oh!

blueorangutan commented Jan 31, 2020

Uh oh!

blueorangutan commented Jan 31, 2020

Uh oh!

weizhouapache commented Feb 3, 2020

Uh oh!

yadvr commented Feb 3, 2020

Uh oh!

blueorangutan commented Feb 3, 2020

Uh oh!

blueorangutan commented Feb 3, 2020

Uh oh!

DaanHoogland commented Feb 3, 2020

Uh oh!

blueorangutan commented Feb 3, 2020

Uh oh!

blueorangutan commented Feb 3, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

yadvr commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 6, 2020

Uh oh!

weizhouapache commented Feb 6, 2020

Uh oh!

weizhouapache commented Feb 6, 2020

Uh oh!

blueorangutan commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants