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: Update the docs for Helm mode Cilium CLI #26606

Merged
merged 1 commit into from Jul 7, 2023

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented Jul 3, 2023

  • Consistently use the --set flag.
  • Replace --helm-auto-gen-values with --dry-run-helm-values.
  • Set kubeProxyReplacement to "true" instead of "strict".

Ref: #26430
Fixes: cilium/cilium-cli#1624

@michi-covalent michi-covalent requested review from a team as code owners July 3, 2023 20:31
@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 Jul 3, 2023
@michi-covalent michi-covalent added the release-note/misc This PR makes changes that have no direct user impact. label Jul 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 3, 2023
@michi-covalent michi-covalent added the release-blocker/1.14 This issue will prevent the release of the next version of Cilium. label Jul 3, 2023
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

Thanks @michi-covalent for tackling this!

A couple of additional aspects:

  • I wonder if it could make sense to add a callout to the cli-download.rst (or somewhere else) to explicitly state that these guides require Cilium CLI v0.15 or subsequent, and users should upgrade it if appropriate. Otherwise users that already have the CLI installed might be puzzled about why it is not working as expected.
  • Similarly, I wonder if it could make sense to explicitly state that using the new CLI against an installation performed with older CLI versions is not supported, and can lead to errors/issues.
  • There seems to be still a couple of leftover occurrences of --helm-set in installation/k3s.rts and servicemesh/installation.rst.

Documentation/installation/k8s-install-migration.rst Outdated Show resolved Hide resolved
Documentation/operations/performance/tuning.rst Outdated Show resolved Hide resolved
Documentation/security/network/encryption-ipsec.rst Outdated Show resolved Hide resolved
Documentation/network/clustermesh/aks-clustermesh-prep.rst Outdated Show resolved Hide resolved
@michi-covalent
Copy link
Contributor Author

I wonder if it could make sense to add a callout to the cli-download.rst (or somewhere else) to explicitly state that these guides require Cilium CLI v0.15 or subsequent, and users should upgrade it if appropriate. Otherwise users that already have the CLI installed might be puzzled about why it is not working as expected.

Similarly, I wonder if it could make sense to explicitly state that using the new CLI against an installation performed with older CLI versions is not supported, and can lead to errors/issues.

yeah let's do that. i'll move this to draft, and add notes about cilium-cli version / incompatibility with the classic mode 📝

There seems to be still a couple of leftover occurrences of --helm-set in installation/k3s.rts and servicemesh/installation.rst.

updated 📝 🚀

@michi-covalent michi-covalent marked this pull request as draft July 5, 2023 04:07
@michi-covalent michi-covalent added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Jul 5, 2023
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.

Thank you, looks great! LGTM pending a couple of things:

  • @giorio94's comment on calling out compatibility concerns
  • I think there are some files and changes that we missed. These are some grep calls in Documentation/

kube proxy replacement, strict vs true. Some of these may not be related to the cilium-cli changes however and may not need to be changed:

grep -i '.*proxy.*replacement.*strict' -n --exclude-dir '_*' -R .
./network/kubernetes/ipam-multi-pool.rst:32:   * ``--set kubeProxyReplacement=strict``
./network/kubernetes/kubeproxy-free.rst:105:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:158:    KubeProxyReplacement:   Strict	[eth0 (Direct Routing), eth1]
./network/kubernetes/kubeproxy-free.rst:383:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:447:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:519:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:550:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:566:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:595:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:631:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:667:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:808:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:869:     --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:962:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:974:By specifying ``kubeProxyReplacement=strict`` the native hostPort support is
./network/kubernetes/kubeproxy-free.rst:996:        --set kubeProxyReplacement=strict \\
./network/kubernetes/kubeproxy-free.rst:1113:- ``kubeProxyReplacement=strict``: When using this option, it's highly recommended
./network/kubernetes/kubeproxy-free.rst:1149:  The following Helm setup below would be equivalent to ``kubeProxyReplacement=strict``
./network/kubernetes/kubeproxy-free.rst:1200:    KubeProxyReplacement:   Strict	[eth0 (DR)]
./network/kubernetes/kubeproxy-free.rst:1301:Cilium's eBPF kube-proxy replacement restricts access from outside (e.g. external
./network/kubernetes/kata.rst:81:   :ref:`kube-proxy replacement <kubeproxy-free>` in strict mode. These
./network/egress-gateway.rst:93:               --set kubeProxyReplacement=strict \\
./network/egress-gateway.rst:103:            kube-proxy-replacement: strict
./network/l2-announcements.rst:42:               --set kubeProxyReplacement=strict \\
./network/l2-announcements.rst:52:            kube-proxy-replacement: strict
./network/l2-announcements.rst:57:* Kube Proxy replacement mode must be enabled and set to ``strict`` mode, see :ref:`kubeproxy-free` for details.
./network/l2-announcements.rst:292:               --set kubeProxyReplacement=strict \\
./network/l2-announcements.rst:304:            kube-proxy-replacement: strict
./operations/upgrade.rst:208:      kubeProxyReplacement: "strict"
./operations/performance/tuning.rst:91:             --set kubeProxyReplacement=strict
./operations/performance/tuning.rst:148:             --set kubeProxyReplacement=strict
./operations/performance/tuning.rst:260:             --set kubeProxyReplacement=strict
./operations/performance/tuning.rst:308:             --set kubeProxyReplacement=strict
./operations/troubleshooting_servicemesh.rst:23: #. Validate that the ``kubeProxyReplacement`` is set to either partial or strict.
./operations/troubleshooting_servicemesh.rst:31:        KubeProxyReplacement:    Strict   [eth0 192.168.49.2]
./configuration/per-node-config.rst:65:nodes with ``io.cilium.migration/kube-proxy-replacement: strict``
./configuration/per-node-config.rst:79:        kubectl -n kube-system patch daemonset kube-proxy --patch '{"spec": {"template": {"spec": {"affinity": {"nodeAffinity": {"requiredDuringSchedulingIgnoredDuringExecution": {"nodeSelectorTerms": [{"matchExpressions": [{"key": "io.cilium.migration/kube-proxy-replacement", "operator": "NotIn", "values": ["strict"]}]}]}}}}}}}'
./configuration/per-node-config.rst:90:          name: kube-proxy-replacement-strict
./configuration/per-node-config.rst:94:              io.cilium.migration/kube-proxy-replacement: strict
./configuration/per-node-config.rst:96:            kube-proxy-replacement: strict
./configuration/per-node-config.rst:106:        kubectl label node $NODE --overwrite 'io.cilium.migration/kube-proxy-replacement=strict'
./configuration/per-node-config.rst:133:        cilium config set --restart=false kube-proxy-replacement strict
./configuration/per-node-config.rst:135:        kubectl -n kube-system delete ciliumnodeconfig kube-proxy-replacement-strict

--set over --helm-set:

grep -i 'helm-set' -n --exclude-dir '_*' -R .
./network/clustermesh/clustermesh.rst:56: * ``cilium install`` option ``--helm-set ipv4NativeRoutingCIDR=10.0.0.0/9``
./network/clustermesh/clustermesh.rst:107: * Cilium CLI install options (*helm* mode) ``--helm-set cluster.name`` and ``--helm-set cluster.id``

@michi-covalent
Copy link
Contributor Author

kube proxy replacement, strict vs true. Some of these may not be related to the cilium-cli changes however and may not need to be changed:

yeah let's deal with kpr values in a separate PR if necessary. summoning @brb to check if we need to update all the pages to use true instead of strict 🍅

grep -i 'helm-set' -n --exclude-dir '_*' -R .
./network/clustermesh/clustermesh.rst:56: * ``cilium install`` option ``--helm-set ipv4NativeRoutingCIDR=10.0.0.0/9``
./network/clustermesh/clustermesh.rst:107: * Cilium CLI install options (*helm* mode) ``--helm-set cluster.name`` and ``--helm-set cluster.id``

these are addressed in #26608

@michi-covalent michi-covalent force-pushed the pr/michi/helm-mode-gogogo branch 2 times, most recently from 43115ee to c1a0820 Compare July 5, 2023 21:16
@michi-covalent
Copy link
Contributor Author

/test

@michi-covalent michi-covalent marked this pull request as ready for review July 5, 2023 21:32
Copy link
Member

@giorio94 giorio94 left a comment

Choose a reason for hiding this comment

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

/lgtm

Documentation/installation/cli-download.rst Show resolved Hide resolved
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.

A couple of nits, but otherwise looks good! Since I am new to reviewing docs, let's also have another docs-structure team member take a look.

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks all in order to me once the feedback from Marco and Ryan is addressed 👍.

Skimming through the discussion: the other values for KPR should have been updated with #26577.

@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. area/cli Impacts the command line interface of any command in the repository. labels Jul 6, 2023
- Consistently use the --set flag.
- Replace --helm-auto-gen-values with --dry-run-helm-values.
- Set kubeProxyReplacement to "true" instead of "strict".
- Add a section in the upgrade guide.
- Add a warning in cilium-cli installation instructions to highlight
  that you need to upgrade cilium-cli to v0.15.0 or later.

Ref: #26430

Co-authored-by: Marco Iorio <marco.iorio@isovalent.com>
Co-authored-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
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.

LGTM! Thank you!

@michi-covalent
Copy link
Contributor Author

docs only change. marking it ready to merge

@michi-covalent michi-covalent added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 6, 2023
@borkmann borkmann merged commit ba52226 into main Jul 7, 2023
40 checks passed
@borkmann borkmann deleted the pr/michi/helm-mode-gogogo branch July 7, 2023 06:05
@jibi jibi mentioned this pull request Jul 10, 2023
19 tasks
@jibi jibi 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 Jul 10, 2023
@julianwiedmann julianwiedmann 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 Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Impacts the command line interface of any command in the repository. 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-blocker/1.14 This issue will prevent the release of the next version of Cilium. 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.

Update CLI documentation for Helm mode
9 participants