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 Sidecar egress TCP listener doesn't use auto allocated address #41311

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Fix Sidecar egress TCP listener doesn't use auto allocated address #41311

merged 3 commits into from
Oct 12, 2022

Conversation

l8huang
Copy link
Contributor

@l8huang l8huang commented Oct 7, 2022 •
edited
Loading

When proxy metadata DNSCapture and DNSAutoAllocate are enabled, if Sidecar egress listener for TCP service is defined with port number, the generated sidecar outbound listener doesn't bind to auto allocated address(240.240.x.x).

  • Ambient

  • Configuration Infrastructure

  • Docs

  • Installation

  • Networking

  • Performance and Scalability

  • Policies and Telemetry

  • Security

  • Test and Release

  • User Experience

  • Developer Infrastructure

  • Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

@l8huang l8huang requested a review from a team as a code owner October 7, 2022 21:48
@istio-policy-bot istio-policy-bot added area/networking release-notes-none Indicates a PR that does not require release notes. labels Oct 7, 2022
@google-cla
Copy link

google-cla bot commented Oct 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 7, 2022
@l8huang l8huang force-pushed the fix-dns branch 3 times, most recently from 47c9ff4 to 10de308 Compare October 7, 2022 22:58
l8huang added 2 commits October 7, 2022 23:00
@l8huang
Fix Sidecar egress TCP listener doesn't use auto allocated address wh…
a17d645 
…en Port is specified

When proxy metadata DNSCapture and DNSAutoAllocate are enabled, if Sidecar egress listener
for TCP service is defined with port number, the generated sidecar outbound listener
doesn't bind to auto allocated address(240.240.x.x).
@l8huang
actual fix code
0d591f4
@l8huang l8huang mentioned this pull request Oct 7, 2022
Closed
@zhlsunshine
Copy link
Contributor

Hi @l8huang, it seems that you moved the part of logic for else branch into if branch in buildSidecarOutboundListeners, I do not think it's proper for us to reverse them just for auto allocated address.

@l8huang
Copy link
Contributor Author

l8huang commented Oct 9, 2022

@zhlsunshine I didn't move any part from else to if, I lifted the common part to the outside of the if ... else ... block, so they could have same behavior -- outbound listener can bind to auto allocated IP no matter port is specified explicitly or not.

hzxuzhonghu
hzxuzhonghu reviewed Oct 9, 2022
View reviewed changes
pilot/pkg/networking/core/v1alpha3/listener.go Outdated Show resolved Hide resolved
pilot/pkg/networking/core/v1alpha3/listener_test.go
&model.Port{
Name: "tcp",
Port: tcpPort,
Protocol: protocol.TCP,
Copy link
Member

Choose a reason for hiding this comment

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

Please also add case for http port

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP uses hosts to multiplex traffic, it doesn't have problem with bind to 0.0.0.0; I propose covering HTTP case with another PR if it's required.

@zhlsunshine
Copy link
Contributor

@zhlsunshine I didn't move any part from else to if, I lifted the common part to the outside of the if ... else ... block, so they could have same behavior -- outbound listener can bind to auto allocated IP no matter port is specified explicitly or not.

Hi @l8huang, I mean the specifications for bind = egressListener.IstioListener.Bind are different in buildSidecarOutboundListeners

  • The condition is if egressListener.IstioListener != nil && egressListener.IstioListener.Port != nil in if branch
  • The condition is if egressListener.IstioListener != nil && egressListener.IstioListener.Bind != "" { in else branch

But the conditions are totally the same as if egressListener.IstioListener != nil && egressListener.IstioListener.Bind != "" { after your PR

@l8huang
Copy link
Contributor Author

l8huang commented Oct 9, 2022

@zhlsunshine That change actually fixes the issue which supposed to be fixed by this PR.

@zhlsunshine
Copy link
Contributor

@zhlsunshine That change actually fixes the issue which supposed to be fixed by this PR.

@l8huang Yeah, so I think it's the concern.

@l8huang
Copy link
Contributor Author

l8huang commented Oct 10, 2022

@hzxuzhonghu @zhlsunshine Thanks for your comments, is the PR good to merge?

hzxuzhonghu
hzxuzhonghu approved these changes Oct 12, 2022
View reviewed changes
Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

LGTM

@istio-testing istio-testing merged commit b8876c8 into istio:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

Assignees
No one assigned
Labels
area/networking release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@l8huang @zhlsunshine @hzxuzhonghu @istio-testing @istio-policy-bot