helm: document ipv{4,6}NativeRoutingCIDR option missing in helm reference #21195

vincentmli · 2022-09-03T16:59:40Z

helm options that are commented out in install/kubernetes/cilium/values.yaml.tmpl
are missing documentation in Documentation/helm-values.rst and in helm reference
this is reported in a few GH issues, for example:
#21107

This appears to be known helm-doc bug and the bug has not been resolved, a related
PR norwoodj/helm-docs#99 that seems has not completely resloved
the issue.

before the helm-doc bug is root caused, uncomment these helm options as workaround.

This PR is to document ipv4, ipv6 native routing cidr helm option

Signed-off-by: Vincent Li v.li@f5.com

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

Fixes: #21107

document ipv4/ipv6 native routing cidr helm option missing in Documentation and helm reference

vincentmli · 2022-09-04T19:19:34Z

not sure why the Smoke test / conformance-test (pull_request) failed. could not find sysdump file to check

timed out waiting for the condition on pods/cilium-n88sn
timed out waiting for the condition on pods/cilium-wb5fr
timed out waiting for the condition on pods/coredns-f9fd979d6-5drnv
timed out waiting for the condition on pods/coredns-f9fd979d6-pmdnl
timed out waiting for the condition on pods/hubble-relay-6c67669fcd-h44t7
pod/kube-scheduler-chart-testing-control-plane condition met
Error: Process completed with exit code 1.

sayboras

Thanks a lot for your PR, I have couple of comments below.

One of them will help with the below smoke test failure you mentioned. I would also suggest taking a quick look in code to confirm the default value in helm and implementation are the same, sometimes we didn't pay much attention in helm especially it's commented.

install/kubernetes/cilium/values.yaml.tmpl

squeed · 2022-09-05T13:15:39Z

This is a huge change, and we need to track down every variable to see if the provided sample is, in fact, the default, or just an example.

For each variable, we need to determine if an empty value can be provided (and thus consumed appropriately).

I know this is a lot of work, but as it stands, this PR isn't safe to merge.

vincentmli · 2022-09-05T14:16:42Z

This is a huge change, and we need to track down every variable to see if the provided sample is, in fact, the default, or just an example.

For each variable, we need to determine if an empty value can be provided (and thus consumed appropriately).

I know this is a lot of work, but as it stands, this PR isn't safe to merge.

@squeed How about splitting the singe huge change one commit in multiple commits/PRs, for examples, the two or three options I have tested and verified to work in one commit/PR, and the ones that I am not sure and has not tested/verified in following-up commits/PRs ?

squeed · 2022-09-05T14:44:01Z

@squeed How about splitting the singe huge change one commit in multiple commits/PRs,

Absolutely, piecemeal is the way to go here. And needless to say, thanks for looking in to this!

gandro · 2022-09-19T16:41:57Z

Broken docs lint is fixed by #21360. Waiting on @qmonnet's review, then this should be good to merge (I don't see a point in running full CI, since it does not affect the generated Helm manifest).

qmonnet · 2022-09-20T13:43:56Z

Documentation Updates CI check failure should be addressed with 5ea92fa. Following Sebastian's reasoning, we're good to go, I'm marking as ready to merge.

pchaigno · 2022-09-21T09:26:16Z

@gandro @qmonnet But how do we know this is correct if we never ran the documentation workflow successfully?

qmonnet · 2022-09-21T09:43:45Z

I'd expect the CI check to have reported other errors too if there was any, but right, I was somewhat on the fence as well. @vincentmli Can you rebase and update the PR please?

…ence helm options that are commented out in install/kubernetes/cilium/values.yaml.tmpl are missing documentation in Documentation/helm-values.rst and in helm reference this is reported in a few GH issues, for example: cilium#21107 This appears to be known helm-doc bug and the bug has not been resolved, a related PR norwoodj/helm-docs#99 that seems has not completely resloved the issue. before the helm-doc bug is root caused, uncomment these helm options as workaround. Since Documentation/helm-values.rst install/kubernetes/cilium/README.md are auto generated, it is difficult to avoid rebase/squash conflicts in local branch and upstream master branch, so introduce the helm option in smaller changes so it easy to review and rebase/squash and merge. Fixes: cilium#21334 cilium#21107 Signed-off-by: Vincent Li <v.li@f5.com>

vincentmli · 2022-09-21T13:27:06Z

I'd expect the CI check to have reported other errors too if there was any, but right, I was somewhat on the fence as well. @vincentmli Can you rebase and update the PR please?

done

pchaigno · 2022-09-21T15:07:08Z

Thanks!

This is follow-up of cilium#21195 to continue to document missing helm options. Signed-off-by: Vincent Li <v.li@f5.com>

This is follow-up of #21195 to continue to document missing helm options. Signed-off-by: Vincent Li <v.li@f5.com>

ieth0 · 2022-10-12T08:59:31Z

As this PR doesn't include a new feature, is there any chance it could be backported to the next release of v1.12? @vincentmli

add missing bpf.hostLegacyRouting, bpf.tproxy, bpf.vlanBypass in helm reference and helm documents. remove bpf.lbBypassFIBLookup, hostRouting when original commented helm option is uncommented, the helm template generated manifest will include the new uncommented helm option even helm template does not specify the new uncommented option, this behavior could affect unknown effect, see cilium#21195 (comment) For bool type helm option, gandro suggested (not (eq nil $value)) check, but it didn't work. see cilium#21195 (comment) Eventually gandro suggested the {{- if (not (kindIs "invalid" .Values.bpf.tproxy)) }} trick found in Masterminds/sprig#53 (comment) see: cilium#21195 (comment) cilium#21195 (comment) Signed-off-by: Vincent Li <v.li@f5.com>

add missing bpf.hostLegacyRouting, bpf.tproxy, bpf.vlanBypass in helm reference and helm documents. remove bpf.lbBypassFIBLookup, hostRouting when original commented helm option is uncommented, the helm template generated manifest will include the new uncommented helm option even helm template does not specify the new uncommented option, this behavior could affect unknown effect, see #21195 (comment) For bool type helm option, gandro suggested (not (eq nil $value)) check, but it didn't work. see #21195 (comment) Eventually gandro suggested the {{- if (not (kindIs "invalid" .Values.bpf.tproxy)) }} trick found in Masterminds/sprig#53 (comment) see: #21195 (comment) #21195 (comment) Signed-off-by: Vincent Li <v.li@f5.com>

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 Sep 3, 2022

vincentmli force-pushed the vli-helm branch 2 times, most recently from e39b4e2 to 4b56988 Compare September 3, 2022 18:00

vincentmli marked this pull request as ready for review September 3, 2022 18:07

vincentmli requested review from a team as code owners September 3, 2022 18:07

vincentmli requested review from squeed, nbusseneau and qmonnet September 3, 2022 18:07

vincentmli force-pushed the vli-helm branch 2 times, most recently from 86f04d3 to 938cd07 Compare September 3, 2022 18:14

vincentmli changed the title ~~helm: document helm option missing in Documentation~~ helm: document helm option missing in helm reference Sep 3, 2022

vincentmli force-pushed the vli-helm branch from 938cd07 to e3391f5 Compare September 3, 2022 18:16

sayboras added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 4, 2022

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 Sep 4, 2022

vincentmli force-pushed the vli-helm branch from e3391f5 to 09c11e5 Compare September 4, 2022 18:28

sayboras reviewed Sep 5, 2022

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

gandro self-requested a review September 5, 2022 09:16

qmonnet requested changes Sep 5, 2022

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

squeed reviewed Sep 5, 2022

View reviewed changes

install/kubernetes/cilium/values.yaml.tmpl Outdated Show resolved Hide resolved

vincentmli force-pushed the vli-helm branch from 09c11e5 to 3b43427 Compare September 5, 2022 15:28

gandro requested a review from qmonnet September 19, 2022 09:06

vincentmli force-pushed the vli-helm branch from e4162b5 to 20a9123 Compare September 19, 2022 14:06

qmonnet removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 20, 2022

qmonnet approved these changes Sep 20, 2022

View reviewed changes

qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 20, 2022

pchaigno removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 21, 2022

vincentmli force-pushed the vli-helm branch from 20a9123 to 1e3d3ae Compare September 21, 2022 13:25

pchaigno merged commit 649d393 into cilium:master Sep 21, 2022

vincentmli deleted the vli-helm branch October 7, 2022 15:54

vincentmli added a commit to vincentmli/cilium that referenced this pull request Oct 7, 2022

helm: Document missing bpf ctTcpMax/ctAnyMax/natMax/neighMax

3c77661

This is follow-up of cilium#21195 to continue to document missing helm options. Signed-off-by: Vincent Li <v.li@f5.com>

vincentmli mentioned this pull request Oct 7, 2022

helm: Document missing bpf ctTcpMax ctAnyMax natMax neighMax #21627

Merged

7 tasks

sayboras pushed a commit that referenced this pull request Oct 10, 2022

helm: Document missing bpf ctTcpMax/ctAnyMax/natMax/neighMax

f3b9ef3

This is follow-up of #21195 to continue to document missing helm options. Signed-off-by: Vincent Li <v.li@f5.com>

vincentmli mentioned this pull request Oct 11, 2022

helm: document missing bpf helm option #21650

Merged

7 tasks

vincentmli mentioned this pull request Oct 21, 2022

helm: Document missing k8sService kubeConfigPath bpf.mapDynamicSizeRatio #21817

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

helm: document ipv{4,6}NativeRoutingCIDR option missing in helm reference #21195

helm: document ipv{4,6}NativeRoutingCIDR option missing in helm reference #21195

vincentmli commented Sep 3, 2022 •

edited

Loading

vincentmli commented Sep 4, 2022

sayboras left a comment

squeed commented Sep 5, 2022

vincentmli commented Sep 5, 2022 •

edited

Loading

squeed commented Sep 5, 2022

gandro commented Sep 19, 2022 •

edited

Loading

qmonnet commented Sep 20, 2022

pchaigno commented Sep 21, 2022

qmonnet commented Sep 21, 2022

vincentmli commented Sep 21, 2022

pchaigno commented Sep 21, 2022

ieth0 commented Oct 12, 2022

helm: document ipv{4,6}NativeRoutingCIDR option missing in helm reference #21195

helm: document ipv{4,6}NativeRoutingCIDR option missing in helm reference #21195

Conversation

vincentmli commented Sep 3, 2022 • edited Loading

vincentmli commented Sep 4, 2022

sayboras left a comment

Choose a reason for hiding this comment

squeed commented Sep 5, 2022

vincentmli commented Sep 5, 2022 • edited Loading

squeed commented Sep 5, 2022

gandro commented Sep 19, 2022 • edited Loading

qmonnet commented Sep 20, 2022

pchaigno commented Sep 21, 2022

qmonnet commented Sep 21, 2022

vincentmli commented Sep 21, 2022

pchaigno commented Sep 21, 2022

ieth0 commented Oct 12, 2022

vincentmli commented Sep 3, 2022 •

edited

Loading

vincentmli commented Sep 5, 2022 •

edited

Loading

gandro commented Sep 19, 2022 •

edited

Loading