Skip to content

resolves issues Multiple Public Subnets static NAT - #4236

Closed
havengit wants to merge 1 commit into
apache:masterfrom
havengit:master
Closed

resolves issues Multiple Public Subnets static NAT #4236
havengit wants to merge 1 commit into
apache:masterfrom
havengit:master

Conversation

@havengit

@havengithavengit commented Jul 31, 2020

Copy link
Copy Markdown
Contributor

Description

This PR resolves 1 issues Multiple Public Subnets static NAT issue
#3361 VR Issues with Multiple Public Subnets
The network has Multiple Public Subnets, enable a static NAT for VM with public IP, If the public IP subnets is different to default SNAT public IP subnets , the VM SNAT IP is default SNAT IP.

For example
In isolate network
Public IP:
192.168.3.3 netmask 255.255.255.0 (default source NAT IP)
192.168.4.3 netmask 255.255.255.0
VM IP:
10.10.1.100

Enable static NAT for 192.168.4.3 -->10.10.1.100
In VM ,get the public IP with commond : curl ip.sb

incorrect:
source nat is 192.168.3.3
correct:
source nat is 192.168.4.3

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?

@havengithavengit changed the title Update configure.pyresolves issues Multiple Public Subnets static NAT Jul 31, 2020
@weizhouapache

Copy link
Copy Markdown
Member

as we have discussed, this issue is caused by commit 82d94a8

we need to find a way to fix both this issue and the issue described in #3604.
maybe add public device name in the rules ?

@DaanHoogland

Copy link
Copy Markdown
Contributor

should we add
Fixes #4234
to the description of this PR, @havengit ?

@DaanHoogland

Copy link
Copy Markdown
Contributor

kick travis in the

@yadvr

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Packaging result: ✔centos7 ✔debian. JID-1750

@weizhouapache

Copy link
Copy Markdown
Member

as I said , this might cause the issue in #3604

@yadvr

Copy link
Copy Markdown
Member

request for comments - @DaanHoogland@andrijapanicsb@PaulAngus

@DaanHoogland

Copy link
Copy Markdown
Contributor

@weizhouapache this would re-introduce the second removed rule from #3604 , only on another location. What is the consequence of the MARK, in your opinion?

"-A PREROUTING -d %s/32 -m state --state NEW -j MARK --set-xmark %s/0xffffffff" %
(rule["public_ip"], hex(100 + int(device[len("eth"):])))])
self.fw.append(["mangle", "front",
"-A PREROUTING -s %s/32 -m state --state NEW -i eth0 -j MARK --set-xmark %s/0xffffffff" %

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@havengit@div8cn
I have tested this pr. The issue described in #3604 is not back.

However in this rule, it should not be "eth0".
for isolated network, the guest nic is eth0. but for vpc, eth0 is nic for link local ip.

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
link/ether 0e:00:a9:fe:65:77 brd ff:ff:ff:ff:ff:ff
inet 169.254.101.119/16 brd 169.254.255.255 scope global eth0
valid_lft forever preferred_lft forever

so in my point of view, this pr fixes the issue in isolated networks, but does not fix the issue in vpc. could you please confirm ? @havengit@div8cn

I have tested this pr but without "-i eth0", then issue #3604 is back.

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.

ok, so we should be more intelligent about the IF to add in the rule, tnx @weizhouapache

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@DaanHoogland I have confirmed with @div8cn that the issue still exist with vpc.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@DaanHoogland yes, This PR only fixes the issue in isolated networks , Weizhou's PR was much more effective #4484

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

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

@DaanHoogland

Copy link
Copy Markdown
Contributor

@blueorangutan test

@apacheapache deleted a comment from blueorangutanOct 21, 2020
@apacheapache deleted a comment from blueorangutanOct 21, 2020
@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

Trillian test result (tid-3041)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31439 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4236-t3041-kvm-centos7.zip
Smoke tests completed. 85 look OK, 0 have error(s)
Only failed tests results shown below:

TestResultTime (s)Test File

@DaanHoogland

Copy link
Copy Markdown
Contributor

@weizhouapache @rhtyd @andrijapanicsb, lgtm lgt-you?

@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache @rhtyd @andrijapanicsb, lgtm lgt-you?

@DaanHoogland
niet goed :-D
see #4236 (comment)

@andrijapanicsb

Copy link
Copy Markdown
Contributor

see comment thread @DaanHoogland

@DaanHoogland

Copy link
Copy Markdown
Contributor

@weizhouapache @rhtyd @andrijapanicsb, lgtm lgt-you?

@DaanHoogland
niet goed :-D
see #4236 (comment)

ok, @weizhouapache I didn't get it, just to verify, this solves it for isolated networks but we don't accept it because the issue remains for VPCs, right?
should we close this one?

@weizhouapache

Copy link
Copy Markdown
Member

@weizhouapache @rhtyd @andrijapanicsb, lgtm lgt-you?

@DaanHoogland
niet goed :-D
see #4236 (comment)

ok, @weizhouapache I didn't get it, just to verify, this solves it for isolated networks but we don't accept it because the issue remains for VPCs, right?
@DaanHoogland yes, the issue still remains for VPCs.

should we close this one?
@DaanHoogland hmmmm...

@DaanHoogland

Copy link
Copy Markdown
Contributor

should we close this one?
@DaanHoogland hmmmm...

i'll take that as a nee

@DaanHoogland

Copy link
Copy Markdown
Contributor

@PaulAngus as per @weizhouapache 's comment, this has a 👎 . He is working on a more solid fix as far as I understand. If you want to RC this week, this should go to 4.15.1

@yadvr

Copy link
Copy Markdown
Member

Looks like Wei has sent another PR that would handle some cases, @DaanHoogland@PaulAngus I'm not sure if this issue is handled well in Wei's new PR but given the objection let's move to next milestone?

@havengit

Copy link
Copy Markdown
ContributorAuthor

Weizhou's PR was much more effective #4484

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@havengit@weizhouapache@DaanHoogland@yadvr@blueorangutan@andrijapanicsb@PaulAngus@svenvogel