Skip to content
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.

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

daemon: Add --derive-masquerade-ip-addr-from-device opt #17230

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

brb
Copy link
Member

@brb brb commented Aug 24, 2021

The new option is used to specify a device which globally scoped IP addr
should be used for BPF-based masquerading.

This is a workaround for an environment which uses ECMP for outgoing
traffic and it has a dedicated device which IP addr should be used for
the masquerading. The workaround is relevant until
#17158 has been resolved (thus,
we hide the flag).

@brb brb added area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Aug 24, 2021
@brb brb requested review from a team August 24, 2021 09:15
@brb brb requested a review from a team as a code owner August 24, 2021 09:15
@brb brb force-pushed the pr/brb/masq-dev-ip-addr-selection branch from f95c18b to 7cca3ec Compare August 24, 2021 09:20
@brb
Copy link
Member Author

brb commented Aug 25, 2021

test-gke

@bmcustodio bmcustodio mentioned this pull request Aug 31, 2021
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments

pkg/node/address.go Show resolved Hide resolved
daemon/cmd/daemon_main.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member

If this is supposed to be "hidden" for now, does it make sense to mark this as "minor release change"? I'd expect release-note/misc?

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM but not sure I understand the motivation:

This is a workaround for an environment which uses ECMP for outgoing traffic and it has a dedicated device which IP addr should be used for the masquerading. The workaround is relevant until #17158 has been resolved (thus, we hide the flag).

#17158 describes a case where a device as multiple global scope IP addresses. This new flag still won't address that since we're only selecting the first global scope IP address of the passed device. What am I missing?

Also, what's the link with ECMP? Do you mean egress traffic from pods can egress the node on multiple native devices, but it should always be SNATed to the same IP address?

@brb brb added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Sep 6, 2021
@brb brb force-pushed the pr/brb/masq-dev-ip-addr-selection branch from 7cca3ec to 0ae690c Compare September 6, 2021 12:29
@brb
Copy link
Member Author

brb commented Sep 6, 2021

#17158 describes a case where a device as multiple global scope IP addresses. This new flag still won't address that since we're only selecting the first global scope IP address of the passed device. What am I missing?

Also, what's the link with ECMP? Do you mean egress traffic from pods can egress the node on multiple native devices, but it should always be SNATed to the same IP address?

The configuration I'm talking about is eth0 and eth1 with local scope IP addrs used for outgoing traffic, and then dummy0 which globally scoped IP addr should be used as src IP addr for the traffic. With #17158, the bpf_fib_lookup will determine the src IP addr based on the route which will be the IP addr of the dummy0.

@brb
Copy link
Member Author

brb commented Sep 6, 2021

test-net-next

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-K8s-1.16-net-next' has 1 failure but they might be new flake since it also hit 1 known flake: #17060 (94.00)

@brb
Copy link
Member Author

brb commented Sep 7, 2021

test-net-next

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.5 Sep 8, 2021
@brb brb force-pushed the pr/brb/masq-dev-ip-addr-selection branch from 0ae690c to dbeb2a6 Compare September 15, 2021 14:55
The new option is used to specify a device which globally scoped IP addr
should be used for BPF-based masquerading.

This is a workaround for an environment which uses ECMP for outgoing
traffic via multiple devices and it has a dedicated device which IP addr
should be used for the masquerading. The workaround is relevant until
#17158 has been resolved (thus,
we hide the flag).

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Sep 15, 2021

test-net-next

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-K8s-1.16-net-next' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: DNS entry of kafka-service is not ready after timeout

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.16-net-next so I can create a new GitHub issue to track it.

@brb
Copy link
Member Author

brb commented Sep 16, 2021

CI hit:

Considering that this is the fix for the customer and it got ACK-ed by the reviewers, marking as ready-to-merge.

@brb brb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 16, 2021
@christarazi christarazi merged commit d204d78 into master Sep 16, 2021
@christarazi christarazi deleted the pr/brb/masq-dev-ip-addr-selection branch September 16, 2021 20:36
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.5 Sep 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.5 Sep 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.5 Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.5
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

7 participants