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

docs: Clean up prerequisites for the Ingress Controller #27222

Merged
merged 1 commit into from Aug 8, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Aug 2, 2023

This commit brings various minor improvements to the intro and prerequisites for the Ingress Controller documentation:

  • Add a link to Kubernetes' documentation for Ingress, given that we never define or explain what Kubernetes Ingress is
  • Instruct users to "enable" kube-proxy replacement, given that the values "strict" or "partial" are deprecated.
  • Use the Helm value instead of the agent flag for the L7 proxy configuration, simply to remain consistent with the item above (KPR).
  • Remove the requirement on Kubernetes 1.19+, given that 1.19 is now the minimum version supported by Cilium.

NOTE: kubeProxyReplacement=true is equivalent to the legacy strict mode, but not to the partial mode. It's likely that the full KPR is not necessary for running the Ingress Controller, but I wasn't able to find the exact subset of options that are required. @sayboras (you authored this requirement in the first place), do you know?

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. feature/k8s-ingress labels Aug 2, 2023
@qmonnet qmonnet requested a review from sayboras August 2, 2023 16:17
@qmonnet qmonnet requested review from a team as code owners August 2, 2023 16:17
@qmonnet
Copy link
Member Author

qmonnet commented Aug 2, 2023

/test

Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@qmonnet This is awesome. ✨ Minor formatting tweak, otherwise LGTM

Documentation/network/servicemesh/ingress.rst Outdated Show resolved Hide resolved
@qmonnet
Copy link
Member Author

qmonnet commented Aug 3, 2023

/test

@qmonnet qmonnet added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Aug 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.1 Aug 3, 2023
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.

NOTE: kubeProxyReplacement=true is equivalent to the legacy strict mode, but not to the partial mode. It's likely that the full KPR is not necessary for running the Ingress Controller, but I wasn't able to find the exact subset of options that are required

Actually, partial along might not be enough, we also need enable-node-port to complement.

Documentation/network/servicemesh/ingress.rst Show resolved Hide resolved
@qmonnet qmonnet added the dont-merge/blocked Another PR must be merged before this one. label Aug 3, 2023
@qmonnet
Copy link
Member Author

qmonnet commented Aug 3, 2023

Actually, partial along might not be enough, we also need enable-node-port to complement.

@sayboras "Partial" does not exist anymore, we have "enabled" (formerly "strict") or disabled ("pick your options"). My question in this case, is, is it enough to have KPR "disabled" and enable-node-port enabled for the Ingress Controller to work? Or is there something else that needs to be enabled?

In other words, what's the minimal requirement for Ingress support?

@sayboras
Copy link
Member

sayboras commented Aug 3, 2023

"Partial" does not exist anymore, we have "enabled" (formerly "strict") or disabled ("pick your options"). My question in this case, is, is it enough to have KPR "disabled" and enable-node-port enabled for the Ingress Controller to work? Or is there something else that needs to be enabled?

Ah right, let me do a couple of tests locally and get back to you on this 👍

@qmonnet qmonnet added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed dont-merge/blocked Another PR must be merged before this one. labels Aug 3, 2023
@sayboras
Copy link
Member

sayboras commented Aug 6, 2023

👋 sorry for the late reply, seems like the combination of kpr=false and nodePort.enabled=true is working for Ingress (except direct traffic to Ingress LB service nodeport). I have created #27304 to add coverage in CI so that we don't break this behavior unintentionally.

Can you help to update the docs if possible, otherwise, I can send the follow-up PR?

$ ksysg cm cilium-config -o json | zgrep -E "replacement|enable-node-port" | head -3
        "enable-node-port": "true",
        "kube-proxy-replacement": "false",
        "kube-proxy-replacement-healthz-bind-address": "",

$ lb=$(kubectl get ingress basic-ingress -o jsonpath='{.status.loadBalancer.ingress[0].ip}')

$ curl -k http://$lb/details/1           
{"id":1,"author":"William Shakespeare","year":1595,"type":"paperback","pages":200,"publisher":"PublisherA","language":"English","ISBN-10":"1234567890","ISBN-13":"123-1234567890"}

This commit brings various minor improvements to the intro and
prerequisites for the Ingress Controller documentation:

- Add a link to Kubernetes' documentation for Ingress, given that we
  never define or explain what Kubernetes Ingress is
- Instruct users to "enable" kube-proxy replacement, given that the
  values "strict" or "partial" are deprecated.
- Mention NodePort as an alternative to kube-proxy replacement, givent
  that this is the only feature from KPR which is actually required for
  Ingress.
- Use the Helm value instead of the agent flag for the L7 proxy
  configuration, simply to remain consistent with the item above (KPR).
- Remove the requirement on Kubernetes 1.19+, given that 1.19 is now the
  minimum version supported by Cilium.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet requested a review from sayboras August 7, 2023 09:00
@qmonnet qmonnet removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Aug 7, 2023
@qmonnet
Copy link
Member Author

qmonnet commented Aug 7, 2023

/test

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 and LGTM ✅

@qmonnet
Copy link
Member Author

qmonnet commented Aug 7, 2023

@sayboras: Thanks! Grepping for strict or partial in the docs, I see similar requirements on “[KPR] set to either partial or strict” in Documentation/servicemesh/l7-traffic-management.rst and Documentation/operations/troubleshooting_servicemesh.rst. Do you know if NodePort is the actual feature that we need there as well?

[Edit: see directly PR linked below.]

@qmonnet qmonnet dismissed zacharysarah’s stale review August 8, 2023 11:41

Review addressed, “otherwise LGTM”

@qmonnet qmonnet 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 Aug 8, 2023
@qmonnet qmonnet merged commit e2ae22e into cilium:main Aug 8, 2023
55 checks passed
@qmonnet qmonnet deleted the pr/ingress-doc branch August 8, 2023 11:43
@nebril nebril added this to Needs backport from main in 1.14.2 Aug 10, 2023
@nebril nebril removed this from Needs backport from main in 1.14.1 Aug 10, 2023
@tklauser tklauser mentioned this pull request Aug 22, 2023
22 tasks
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 22, 2023
@joestringer joestringer added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Aug 25, 2023
@joestringer joestringer moved this from Needs backport from main to Backport done to v1.14 in 1.14.2 Aug 25, 2023
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. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. feature/k8s-ingress release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.14.2
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

5 participants