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.

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

Fix minor bug where the previous Cilium proxy port was not reused #27634

Merged
merged 3 commits into from Sep 10, 2023
Merged

Fix minor bug where the previous Cilium proxy port was not reused #27634

merged 3 commits into from Sep 10, 2023

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Aug 22, 2023 •
edited

  • proxy, daemon: Extract common types to separate types package
  • dnsproxy: Extract and export ipFamily functions
  • iptables, dnsproxy: Fix regex matching for bind addr

Validated by doing manual testing of restarting Cilium and observing that it picks up the previous proxy port.

Last commit for convenience:

iptables, dnsproxy: Fix regex matching for bind addr

In a previous change 1, the bind address for the proxy changed from
0.0.0.0 to localhost. This broke restoring the old proxy port and caused
Cilium to always allocate a new proxy port.

Fix it by changing the regex string in the case that the iptables rule
is related to the DNS proxy. If the name passed in is not for the DNS
proxy, then default to the previous behavior of "0.0.0.0" and "::".

Found by code inspection.

Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only")
Fixes: #25309

Signed-off-by: Chris Tarazi chris@isovalent.com

@christarazi christarazi added kind/bug This is a bug in the Cilium logic. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Aug 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 22, 2023
@christarazi christarazi added area/daemon Impacts operation of the Cilium daemon. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/fqdn Affects the FQDN policies feature labels Aug 22, 2023
@christarazi christarazi changed the title pr/christarazi/fix reuse old proxy port Fix minor bug where the previous Cilium proxy port was not reused Aug 22, 2023
@christarazi christarazi marked this pull request as ready for review August 22, 2023 19:16
@christarazi christarazi requested review from a team as code owners August 22, 2023 19:16
@christarazi
Copy link
Member Author

cc @mhofstetter

@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch from 42c4208 to e93256f Compare August 22, 2023 19:18
@christarazi
Copy link
Member Author

/test

@christarazi christarazi added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Aug 22, 2023
@christarazi
Copy link
Member Author

Import cycle errors, back to draft.

@christarazi christarazi marked this pull request as draft August 22, 2023 19:29
@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch from e93256f to 0697409 Compare August 22, 2023 20:28
@christarazi
Copy link
Member Author

/test

@christarazi christarazi marked this pull request as ready for review August 22, 2023 20:34
doniacld
doniacld approved these changes Aug 23, 2023
View reviewed changes
Copy link
Contributor

@doniacld doniacld left a comment •
edited

Choose a reason for hiding this comment

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

Only one minor comment otherwise looks good! Good catch!

@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch 2 times, most recently from 7b09fc5 to a53b393 Compare August 24, 2023 19:39
@christarazi
Copy link
Member Author

/test

@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch from a53b393 to b302031 Compare August 29, 2023 00:14
@christarazi
Copy link
Member Author

/test

@christarazi
Copy link
Member Author

@jrajahalme ping!

jrajahalme
jrajahalme requested changes Sep 4, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme left a comment

Choose a reason for hiding this comment

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

Will need to retain the old forms for the upgrade case.

pkg/datapath/iptables/iptables.go Outdated Show resolved Hide resolved
@christarazi
proxy, daemon: Extract common types to separate types package
8251909 
In the upcoming commits, the DNS proxy code and the datapath iptables
code need to share a few variables. This commit is necessary to prevent
an import cycle.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
dnsproxy: Extract and export ipFamily functions
9902d62 
This will be useful for the upcoming commit to use in the datapath /
iptables packages for extracting shared state from iptables rules
related to the DNS proxy.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch from b302031 to 1cdfd1d Compare September 5, 2023 21:21
@christarazi
iptables, dnsproxy: Fix regex matching for bind addr
1a7d8d8 
In a previous change [1], the bind address for the proxy changed from
0.0.0.0 to localhost. This broke restoring the old proxy port and caused
Cilium to always allocate a new proxy port.

Fix it by changing the regex string to include the new bind address as
well as the previously used "0.0.0.0" and "::", for
backwards-compatibility reasons on upgrade.

Found by code inspection.

[1]: cilium#25309

Fixes: 5304088 ("dnsproxy: bind dns proxy to localhost only")
Fixes: cilium#25309

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/fix-reuse-old-proxy-port branch from 1cdfd1d to 1a7d8d8 Compare September 5, 2023 21:39
@christarazi
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 8, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.2 Sep 8, 2023
@michi-covalent michi-covalent added this to Needs backport from main in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.2 Sep 9, 2023
@youngnick youngnick merged commit 1f8e015 into cilium:main Sep 10, 2023
61 checks passed
@christarazi christarazi deleted the pr/christarazi/fix-reuse-old-proxy-port branch September 11, 2023 18:29
@gandro
Copy link
Member

gandro commented Sep 12, 2023

There are quite a few conflicts against v1.14. Most look trivial, but there are a few that look more complicated. I'm marking this backport/author.

@gandro gandro added the backport/author The backport will be carried out by the author of the PR. label Sep 12, 2023
@gandro gandro mentioned this pull request Sep 12, 2023
Merged
15 tasks
@christarazi christarazi mentioned this pull request Sep 12, 2023
Merged
1 task
@christarazi christarazi added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Sep 12, 2023
@jrajahalme jrajahalme mentioned this pull request Oct 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldelossa ldelossa ldelossa approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

@doniacld doniacld doniacld approved these changes

@giorio94 giorio94 giorio94 approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. area/fqdn Affects the FQDN policies feature area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. backport/author The backport will be carried out by the author of the PR. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
No open projects
1.14.3
Needs backport from main
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@christarazi @gandro @ldelossa @jrajahalme @doniacld @giorio94 @youngnick