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

helm: avoid generating ConfigMapList #21750

Merged

Conversation

kaworu
Copy link
Member

@kaworu kaworu commented Oct 17, 2022

According to the Kubernetes documentation, List objects should not appear in requests.

See also prometheus-operator/kube-prometheus#1735

@kaworu kaworu added kind/cleanup This includes no functional changes. release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels Oct 17, 2022
@kaworu kaworu requested review from a team as code owners October 17, 2022 09:42
@kaworu
Copy link
Member Author

kaworu commented Oct 17, 2022

This intend to fix a helm lint warning, before this patch we have

% helm lint --debug --with-subcharts --values ./install/kubernetes/cilium/values.yaml ./install/kubernetes/cilium
==> Linting ./install/kubernetes/cilium
[WARNING] templates/hubble/dashboards-configmap.yaml: object name does not conform to Kubernetes naming requirements: "": metadata.name: Invalid value: "": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

1 chart(s) linted, 0 chart(s) failed

and after the patch:

% helm lint --debug --with-subcharts --values ./install/kubernetes/cilium/values.yaml ./install/kubernetes/cilium
==> Linting ./install/kubernetes/cilium

1 chart(s) linted, 0 chart(s) failed

@kaworu kaworu requested a review from chancez October 17, 2022 09:46
@chancez
Copy link
Contributor

chancez commented Oct 17, 2022

According to the Kubernetes documentation, List objects should not appear in requests.

I don't see anywhere in the documentation that it says that specifically. Unless you mean this note:

Note:
Keep in mind that the Kubernetes API does not have a kind named List.

kind: List is a client-side, internal implementation detail for processing collections that might be of different kinds of object. Avoid depending on kind: List in automation or other code.

Which I believe is talking specifically about kind: List, not kind: FooList (the prior isn't a real API type, the latter is: kubectl explain podlist vs kubectl explain list)

Which leads me to: is this PR actually necessary? NVM, I see that the helm lint fails, annoyingly, this should be valid, but I see the motivation at least.

Copy link
Contributor

@chancez chancez left a comment

Choose a reason for hiding this comment

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

Functionally this looks fine. We lost the if $files check, but that should be fine since we range over $files which will produce nothing if $files is empty.

@kaworu
Copy link
Member Author

kaworu commented Oct 17, 2022

[…] We lost the if $files check, but that should be fine since we range over $files which will produce nothing if $files is empty

It wasn't lost, it was removed 😉

@kaworu kaworu force-pushed the pr/kaworu/helm/do-not-generate-configmaplist branch from 1ff8d08 to 52f2670 Compare October 18, 2022 10:33
According to the Kubernetes documentation[1], List objects should not
appear in requests.

See also prometheus-operator/kube-prometheus#1735

[1]: https://kubernetes.io/docs/reference/using-api/api-concepts/#collections

Signed-off-by: Alexandre Perrin <alex@isovalent.com>
@kaworu
Copy link
Member Author

kaworu commented Oct 18, 2022

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sKafkaPolicyTest Kafka Policy Tests KafkaPolicies

Failure Output

FAIL: Found 1 k8s-app=cilium logs matching list of errors that must be investigated:

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

@kaworu
Copy link
Member Author

kaworu commented Oct 20, 2022

/test-runtime

EDIT: previous run here, error was

[13:44:00] FAIL: ipsec_linux_test.go:122: IPSecSuitePrivileged.TestUpsertIPSecEndpoint
[13:44:00]
[13:44:00] ipsec_linux_test.go:146:
[13:44:00]     cleanIPSecStatesAndPolicies(c)
[13:44:00] ipsec_linux_test.go:206:
[13:44:00]     c.Fatalf("Can't delete XFRM state: %v", err)
[13:44:00] ... Error: Can't delete XFRM state: no such process
[13:44:00]
[13:44:00] level=error msg="Error while getting xfrm stats" error="error due to some reason" subsys=ipsec
[13:44:00] level=error msg="Error while getting xfrm stats" error="error due to some reason" subsys=ipsec
[13:44:00] level=error msg="Error while getting xfrm stats" error="error due to some reason" subsys=ipsec
[13:44:00] OOPS: 6 passed, 1 FAILED

Re-running to see whether it is a flake or not.

@kaworu
Copy link
Member Author

kaworu commented Oct 20, 2022

/test-1.16-4.9

EDIT: previous run here, error

FAIL: Found 1 k8s-app=cilium logs matching list of errors that must be investigated:
level=error

@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 Oct 20, 2022
@aanm aanm merged commit 0687885 into cilium:master Oct 21, 2022
@kaworu kaworu deleted the pr/kaworu/helm/do-not-generate-configmaplist branch October 24, 2022 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience kind/cleanup This includes no functional changes. 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

7 participants