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

docs: Clean up mentions to KPR=partial|strict #27314

Merged
merged 3 commits into from Aug 23, 2023
Merged

docs: Clean up mentions to KPR=partial|strict #27314

merged 3 commits into from Aug 23, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Aug 7, 2023 •
edited

Follow-up to #27222.
Similar to #26577.

  • docs: Replace mentions to KPR strict mode for per-node-config docs
  • docs: Replace deprecated "kubeProxyReplacement=strict" with "...=true"
  • docs: Update references to legacy kube-proxy replacement modes

Requirement on NodePort only in Documentation/network/servicemesh/l7-traffic-management.rst and Documentation/operations/troubleshooting_servicemesh.rst to be confirmed.

docs: Clean up references to deprecated modes "strict" and "partial" for kube-proxy replacement feature flag

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Aug 7, 2023
@qmonnet qmonnet requested review from brb and sayboras August 7, 2023 10:17
@qmonnet qmonnet requested review from a team as code owners August 7, 2023 10:17
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.1 Aug 7, 2023
@qmonnet
Copy link
Member Author

qmonnet commented Aug 7, 2023

/test

@sayboras
Copy link
Member

sayboras commented Aug 7, 2023

Requirement on NodePort only in Documentation/network/servicemesh/l7-traffic-management.rst and Documentation/operations/troubleshooting_servicemesh.rst to be confirmed.

Let me do a couple of tests locally, and get back to you on the above.

lmb
lmb approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@lmb lmb left a comment

Choose a reason for hiding this comment

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

EGW changes SGTM.

brb
brb approved these changes Aug 7, 2023
View reviewed changes
Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Thanks!

lambdanis
lambdanis reviewed Aug 7, 2023
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment •
edited

Choose a reason for hiding this comment

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

One general comment I have is that it's tricky when a Helm value is "true"/"false", as a string not boolean, like it's the case for kubeProxyReplacement now. I think all helm upgrade commands need to be adjusted to treat this value as a string, but I haven't actually run them, so ignore me if I'm misremembering my helm.

(docs structure LGTM)

Documentation/network/egress-gateway.rst
@@ -93,7 +93,7 @@ The egress gateway feature and all the requirements can be enabled as follow:
--reuse-values \\
--set egressGateway.enabled=true \\
--set bpf.masquerade=true \\
--set kubeProxyReplacement=strict \\
--set kubeProxyReplacement=true \\
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--set kubeProxyReplacement=true \\
--set-string kubeProxyReplacement=true \\

I haven't tried it, but IIRC Helm CLI would treat true as a boolean, while the value is string.

Documentation/network/kubernetes/ipam-multi-pool.rst Show resolved Hide resolved
Documentation/network/kubernetes/kata.rst Show resolved Hide resolved
Documentation/network/l2-announcements.rst Show resolved Hide resolved
Documentation/network/l2-announcements.rst Show resolved Hide resolved
Documentation/operations/performance/tuning.rst Show resolved Hide resolved
Documentation/operations/performance/tuning.rst Show resolved Hide resolved
Documentation/operations/performance/tuning.rst Show resolved Hide resolved
Documentation/operations/performance/tuning.rst Show resolved Hide resolved
@qmonnet
Copy link
Member Author

qmonnet commented Aug 8, 2023

One general comment I have is that it's tricky when a Helm value is "true"/"false", as a string not boolean, like it's the case for kubeProxyReplacement now. I think all helm upgrade commands need to be adjusted to treat this value as a string, but I haven't actually run them, so ignore me if I'm misremembering my helm.

The string vs. boolean consideration is also my understanding, but I think we're already handling everything in the charts. We enquote the value for the KPR flag: kube-proxy-replacement: {{ $kubeProxyReplacement | quote }}, and all values are passed through toString for the validation steps (see commit c2c9121, re-applied as d2b63e3).

I did test on my side with --set kubeProxyReplacement=true, and KPR turns on just as expected.

(docs structure LGTM)

Thanks for the review!

@qmonnet
docs: Replace mentions to KPR strict mode for per-node-config docs
db5f83d 
The "strict" and "partial" mode for kube-proxy replacement (KPR) were
deprecated in Cilium 1.14, in favour of the feature being enabled (the
flag is set to "true" and all related options are turned on) or disabled
(leaving users free to pick the options they want).

Let's update the documentation for per-node configuration accordingly
and use "true" instead of "strict".

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Replace deprecated "kubeProxyReplacement=strict" with "...=true"
8f09c51 
The "strict" and "partial" mode for kube-proxy replacement (KPR) were
deprecated in Cilium 1.14, in favour of the feature being enabled (the
flag is set to "true" and all related options are turned on) or disabled
(leaving users free to pick the options they want).

This commit is a simple update for the cases where "strict" can be
replaced with "true" in the documentation.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Update references to legacy kube-proxy replacement modes
d64a5a0 
The "strict" and "partial" mode for kube-proxy replacement (KPR) were
deprecated in Cilium 1.14, in favour of the feature being enabled (the
flag is set to "true" and all related options are turned on) or disabled
(leaving users free to pick the options they want).

This commit updates some references to the legacy modes, and update the
surrouinding documentation accordingly.

For L7 traffic management, we can narrow the minimal requirements
(NodePort rather than full KPR set), and also remove the requirement on
Kubernetes version 1.19+, now that 1.19 is the minimal version supported
by Cilium anyway.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
Copy link
Member Author

qmonnet commented Aug 8, 2023 •
edited

Merge conflict with 1cb2210
/test

@qmonnet qmonnet requested a review from lambdanis August 8, 2023 11:23
@lambdanis
Copy link
Contributor

The string vs. boolean consideration is also my understanding, but I think we're already handling everything in the charts. We enquote the value for the KPR flag: kube-proxy-replacement: {{ $kubeProxyReplacement | quote }}, and all values are passed through toString for the validation steps (see commit c2c9121, re-applied as d2b63e3).

I did test on my side with --set kubeProxyReplacement=true, and KPR turns on just as expected.

I see, thank you!

@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
@sayboras
Copy link
Member

Let me do a couple of tests locally, and get back to you on the above.

CEC is working as expected with nodePort.enabled=true, and kubeProxyReplacement=fase. Sorry for late reply.

sayboras
sayboras approved these changes Aug 19, 2023
View reviewed changes
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 ✔️

@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 Aug 23, 2023
@borkmann borkmann merged commit 163e575 into cilium:main Aug 23, 2023
55 checks passed
@tklauser tklauser mentioned this pull request Aug 24, 2023
Merged
9 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 24, 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
@qmonnet qmonnet deleted the pr/docs-kpr-strict-update branch August 29, 2023 11:25
@michi-covalent michi-covalent mentioned this pull request Sep 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brb brb brb approved these changes

@lmb lmb lmb approved these changes

@lambdanis lambdanis lambdanis approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@sayboras sayboras sayboras approved these changes

Assignees
No one assigned
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. 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
No open projects
1.14.2
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@qmonnet @sayboras @lambdanis @brb @lmb @tommyp1ckles @tklauser @borkmann @joestringer