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

doc: Added doc for running ingress/GwAPI in host network mode #31839

Merged

Conversation

PhilipSchmid
Copy link
Contributor

@PhilipSchmid PhilipSchmid commented Apr 8, 2024

This PR adds documentation for #30840 and #31646.

  • Added documentation about how to expose the Cilium Ingress Controller / GwAPI Envoy listener on the host network
  • Fixed faulty Helm flag in Helm values comment
  • Unified GwAPI l7Proxy=true requirement to work everywhere with Helm flags, not agent flags.
  • Fixed RST title level inconsistency in the troubleshooting guide

ToDos:

Release note:

doc: Added doc for ingress/GwAPI host network mode

cc @mhofstetter, @networkop

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 8, 2024
Copy link
Contributor

@networkop networkop left a comment

Choose a reason for hiding this comment

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

left a few suggestions but overall lgtm

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/docs_envoy_hostnetwork branch 2 times, most recently from 6fb0a99 to 16e89ee Compare April 15, 2024 08:26
@mhofstetter mhofstetter added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-gateway-api feature/k8s-ingress release-note/misc This PR makes changes that have no direct user impact. dont-merge/blocked Another PR must be merged before this one. labels Apr 22, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 22, 2024
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/docs_envoy_hostnetwork branch 2 times, most recently from bced1f4 to 9abd528 Compare April 29, 2024 12:42
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/docs_envoy_hostnetwork branch from ad4f667 to ecf28b1 Compare April 30, 2024 06:19
@mhofstetter mhofstetter removed the dont-merge/blocked Another PR must be merged before this one. label Apr 30, 2024
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

Looks good to me - except one small change. But this is definitely ready for review (undraft) 🎉 Thanks @PhilipSchmid

side note: maybe squashing the commits into one (with an updated commit message)?

Documentation/network/servicemesh/ingress.rst Outdated Show resolved Hide resolved
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/docs_envoy_hostnetwork branch from 290c126 to 725871f Compare April 30, 2024 09:36
@PhilipSchmid PhilipSchmid marked this pull request as ready for review April 30, 2024 09:37
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Thanks ✔️

Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

This is great, thank you! I left a couple of small nits for you on grammar, wording and structure. Let me know what you think!

There are some suggestions which are applicable to both the Ingress and Gateway API sections. For brevity, I just left them in the Gateway API section.

@PhilipSchmid
Copy link
Contributor Author

Thanks, @learnitall, for your feedback! I addressed everything directly in c1b09de and will squash all commits as soon as we have all reviews in.

Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Great thank you! LGTM! I found one small grammar nit, but it's non-blocking.

Documentation/network/servicemesh/ingress.rst Outdated Show resolved Hide resolved
* Added documentation about how to expose the Cilium Ingress Controller / GwAPI Envoy listener on the host network
* Fixed faulty Helm flag in Helm values comment
* Unified GwAPI `l7Proxy=true` requirement to work everywhere with Helm flags, not agent flags.
* Fixed RST title level inconsistency in the troubleshooting guide

Signed-off-by: Philip Schmid <philip.schmid@isovalent.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/docs_envoy_hostnetwork branch from 5d347df to 9084828 Compare May 6, 2024 08:40
@PhilipSchmid
Copy link
Contributor Author

Squashed and rebased in the hope we can soon merge it 🤞.

@mhofstetter
Copy link
Member

/test

@mhofstetter
Copy link
Member

mhofstetter commented May 13, 2024

Checks passing and relevant reviews are in -> marking as ready-to-merge 🎉

@mhofstetter mhofstetter added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 13, 2024
@julianwiedmann
Copy link
Member

I see that #31646 was also backported to v1.15. Do we want some docs there as well?

@julianwiedmann julianwiedmann added this pull request to the merge queue May 14, 2024
@mhofstetter
Copy link
Member

mhofstetter commented May 14, 2024

I see that #31646 was also backported to v1.15. Do we want some docs there as well?

@julianwiedmann Thanks for mentioning this. But it's not necessary to backport this PR.

The author backport of the partially related PR only backported the actual fix (merge of envoy listeners) - and not the part that was related to the feature that gets documented with this PR.

See backport note:

HostNetwork support isn't available on v1.15, other smaller conflicts due to refactorings not being backported

Merged via the queue into cilium:main with commit 1f53d94 May 14, 2024
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/servicemesh GH issues or PRs regarding servicemesh feature/k8s-gateway-api feature/k8s-ingress 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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants