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

Add hostNetwork DNSPolicy support #2309

Closed
wants to merge 2 commits into from
Closed

Conversation

zzjin
Copy link
Contributor

@zzjin zzjin commented Dec 15, 2023

And add support listen port under 1024 (like 443).

What type of PR is this?
fix: fix new introduced hostNetwork cannot speak to xds bug.

May fix: #2310

@zzjin zzjin requested a review from a team as a code owner December 15, 2023 15:16
Copy link

🚀 Thank you for contributing to the Envoy Gateway project! 🚀

Before merging, please ensure to follow the process below:

  1. Requesting Reviews:
  • cc @envoyproxy/gateway-reviewers team for an initial review.
  • After the initial review, reviewers should request the @envoyproxy/gateway-maintainers team for further review.
  1. Review Approval:
  • Your PR needs to receive at least two approvals.
  • At least one approval must come from a member of the gateway-maintainers team.

NOTE: Once your PR is under review, please do not rebase and force push it. Otherwise, it will force your reviewers to review the PR from scratch rather than simply look at your latest changes.

What's more, you can help expedite the processing of your PR by
  • Ensuring you have self-reviewed your work according to the project's Contribution Guidelines.
  • If your PR addresses a specific issue, make sure to mention it in the PR description.
  • Respond promptly if there are any test failures or suggestions for improvements that we comment on.

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Plz fix the CI and make sure it is backport compatible.

containerPort := servicePortToContainerPort(servicePort.port)

containerPort := servicePort.port
if !(resources.EnvoyProxy != nil && resources.EnvoyProxy.Spec.Provider != nil &&
Copy link
Contributor

Choose a reason for hiding this comment

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

just out of curiosity, is there any better way to improve this looong hideous nested nil check?

Copy link
Member

Choose a reason for hiding this comment

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

@shawnh2
Copy link
Contributor

shawnh2 commented Dec 18, 2023

hi @zzjin, you can try make testdata to automatically generate the correct test data.

@arkodg
Copy link
Contributor

arkodg commented Dec 18, 2023

we shouldnt be changing the container port logic, all clients should access the gateway via the service

@Xunzhuo
Copy link
Member

Xunzhuo commented Dec 19, 2023

@arkodg Any better solutions for using EG in such scenarios:

  1. a kubernetes cluster without lb implementation
  2. expose envoyproxy with 80/443

In this scenario, EG is more like a real loadbalancer, it should be a common request.

@zzjin zzjin force-pushed the dev_hostnetwork branch 2 times, most recently from ee28e61 to a20f05c Compare December 19, 2023 18:58
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a723843) 64.67% compared to head (d2fb176) 64.73%.
Report is 113 commits behind head on main.

❗ Current head d2fb176 differs from pull request most recent head e4e662c. Consider uploading reports for the commit e4e662c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2309      +/-   ##
==========================================
+ Coverage   64.67%   64.73%   +0.06%     
==========================================
  Files         114      114              
  Lines       16618    16633      +15     
==========================================
+ Hits        10747    10767      +20     
+ Misses       5193     5188       -5     
  Partials      678      678              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arkodg
Copy link
Contributor

arkodg commented Dec 19, 2023

@Xunzhuo you can route to the service port (80/443) via External IP

@zzjin zzjin marked this pull request as draft December 20, 2023 06:31
@Xunzhuo
Copy link
Member

Xunzhuo commented Dec 20, 2023

@arkodg Can you elaborate it ?

@arkodg
Copy link
Contributor

arkodg commented Dec 20, 2023

@Xunzhuo
Copy link
Member

Xunzhuo commented Dec 21, 2023

Correct ... @arkodg I think I missed this, thanks for pointing out.

@Xunzhuo Xunzhuo changed the title Fix hostNetwork DNSPolicy. Add hostNetwork DNSPolicy support Dec 29, 2023
@Xunzhuo Xunzhuo marked this pull request as ready for review December 29, 2023 04:54
And add support listen port under 1024 (like 443).

Signed-off-by: zzjin <[email protected]>
@Xunzhuo Xunzhuo marked this pull request as draft January 8, 2024 02:26
Copy link

github-actions bot commented Feb 7, 2024

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Feb 7, 2024
@Xunzhuo
Copy link
Member

Xunzhuo commented Feb 23, 2024

close via #2374

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot listen host 80/443 when set hostNetwork=true
4 participants