Helm Refactoring #13259

seanmwinn · 2020-09-22T21:13:20Z

Signed-off-by: Sean Winn sean@isovalent.com

Full refactor of helm charts:

Re-structure helm charts into a single chart for cilium.
Configure sane default values at the chart level for each component, removing the need for all global values previously implemented.
Maintain upgrade compatibility with prior versions of cilium.

Helm charts have been fully re-structured to a single chart for cilium with no dependency on sub-charts. More than 170 global values have been properly scoped to the cilium chart with sane defaults defined. Users upgrading from prior versions of cilium should be sure to read the upgrade guide for specific instructions.

joestringer

I didn't do a deep review yet since I'm still mulling over the best way to efficiently do so for such a large change. I did however note a few points below where it seems like the charts are not taking into account changes made into the master branch of the Cilium tree, such as hubble-relay TLS functionality and likely also the "local redirect policy" RBAC (amongst other things).

One reassuring point would be if you run make -C install/kubernetes, does it produce any diff against the quick-install / experimental-install YAMLs? If yes, why? (If this command breaks, then well that could do with some debugging and I'm happy to help debug why, feel free to reach out).

Step 2 of your PR description, "Maintain upgrade compatibility" suggests that upgrade should continue to work. Two questions from my side on this:

Do we need any changes to the upgrade guide to account for this?
Does the upgrade CI test still pass?

(I can run the CI to get feedback on the second point)

install/kubernetes/cilium/charts/hubble-relay/Chart.yaml

install/kubernetes/cilium/charts/hubble-relay/README.md

install/kubernetes/cilium/charts/agent/templates/NOTES.txt

install/kubernetes/cilium/charts/hubble-relay/values.yaml

.github/workflows/smoke-test-ipv6.yaml

tgraf

The end result of this looks awesome. Much cleaner.

The PR description states that compatibility is maintained. How? I assume that existing values will be ignored and users will have to convert old helm values to new helm values. Is that correct? Do we need a script or a guide on how to convert old values to new values?

Documentation/install/upgrade.rst

christarazi

Looks fantastic overall! Very small comments to address below.

install/kubernetes/Makefile

install/kubernetes/cilium/templates/cilium-operator-deployment.yaml

vendor/github.com/fsnotify/fsnotify/.travis.yml

aanm

How of curiosity, how will helm decide if we end up with the same option in 2 different sub-charts?

also, the quick-install.yaml file is a lot different from the current root. Ideally the generated file should be the same, or similar to the existing one.

Documentation/concepts/ebpf/_static/kubernetes_iptables.svg

install/kubernetes/cilium/templates/cilium-operator-podsecuritypolicy.yaml

install/kubernetes/cilium/templates/cilium-agent-podsecuritypolichy.yaml

install/kubernetes/cilium/charts/hubble-relay/values.yaml

tgraf · 2020-09-29T10:04:32Z

I've reviewed this PR a second time to make sure I understand what it changes. Based on the comments in the meeting on Monday, I was under the impression that this would also remove the need for the compatibility values which, if I understand this PR correctly, are still in-tact and unchanged after this PR.

The main change this PR achieves seems to be to merge all agent-related sub-charts into a single parent chart to avoid global variables. The benefit from this is that a single Values file can control all aspects of the agent.

The cost of this change is that all Helm value names are changing which means a hard breakage unless we take action to convert existing Values files. If I understand it correctly, helm upgrade will break hard unless we do something about it.

It seems vital that we understand what we are asking the user to do in the event of an upgrade in order to migrate to this new model. I'm not sure I understand what we are about to ask users.

seanmwinn · 2020-09-29T14:40:51Z

@tgraf your understanding of the upgrade being broken by this change is not entirely accurate. I have been using helm upgrade to migrate from existing 1.7x and 1.8x installations to ones using this helm chart as a basis of my own testing. The upgradeCompatibility flag is removed from the values file, and any leftover pieces in the config map have zero effect. I still have a little bit of cleanup to do there, apparently, but upgrades totally work with small changes on the user's part which can be easily documented.

The upgrade guide has the start of what will become the upgrade directions, but I have been working toward getting all of the tests to pass as a primary focus, and they have been secondary and are still not complete. I'm expecting to complete the last pieces today/tomorrow now that I got all the base tests passing as of last night. I will need to also see what CI produces next. I'm about to kick it off.

My offer from the Monday call stands to demonstrate how this works in an upgrade scenario. I've tested it thoroughly, and I'm confident it will not break upgrades for users, however, they will need to follow the upgrade guide to be successful. The other piece which I think still needs some clarification across the team is how to set default values for each released version of a chart.

seanmwinn · 2020-09-29T15:02:49Z

test-me-please

seanmwinn · 2020-09-29T22:48:10Z

test-me-please

seanmwinn · 2020-09-30T18:18:57Z

test-me-please

seanmwinn · 2020-09-30T18:24:52Z

test-me-please

seanmwinn · 2020-09-30T19:29:50Z

test-me-please

seanmwinn · 2020-09-30T20:44:04Z

test-me-please

seanmwinn · 2020-09-30T20:47:56Z

test-me-please

It looks like this file was wrongly committed as part of the Helm refactoring PR (#13259). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

PR #11577 added checks to ensure that the following conditions are met: - Hubble UI requires Hubble Relay - Hubble Relay requires Hubble These checks were pruned as part of the Helm refactoring PR (#13259) so this commit adds them back. This ensures that invalid Hubble settings such as enabling UI without enable Relay are caught early to avoid confusion. Fixes: #13537 Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

PR #13259 move helm chart files around and the CODEOWNERS file was not updated accordingly. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

PR #11577 added checks to ensure that the following conditions are met: - Hubble UI requires Hubble Relay - Hubble Relay requires Hubble These checks were pruned as part of the Helm refactoring PR (#13259) so this commit adds them back. This ensures that invalid Hubble settings such as enabling UI without enable Relay are caught early to avoid confusion. Fixes: #13537 Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

It looks like this file was wrongly committed as part of the Helm refactoring PR (#13259). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

PR #13259 made the service account configuration a top level value (`.Values.serviceAccounts`). This commit removes the now unused service accounts under `hubble.ui.serviceAccount` and `hubble.relay.serviceAccount`, which are supposed to be configured via `serviceAccounts.ui` and `serviceAccounts.relay`. In addition, also fixes the service account value in the Hubble UI cluster role and cluster role binding. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>

nebulon42 · 2021-03-07T12:28:05Z

install/kubernetes/cilium/values.yaml

-    runPath: "/var/run/cilium"
+# nativeRoutingCIDR allows to explicitly specify the CIDR for native routing. This
+# value corresponds to the configured cluster-cidr.
+# nativeRoutingCIDR:


@seanmwinn Here "nativeRoutingCIDR" is commented out. Was that an intended change?

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 22, 2020

seanmwinn added the release-note/major This PR introduces major new functionality to Cilium. label Sep 22, 2020

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 22, 2020

joestringer reviewed Sep 23, 2020

View reviewed changes

aanm mentioned this pull request Sep 24, 2020

helm: configurable nodeSelector and tolerations for all charts #13267

Merged

aanm requested changes Sep 24, 2020

View reviewed changes

.github/workflows/smoke-test-ipv6.yaml Outdated Show resolved Hide resolved

seanmwinn marked this pull request as ready for review September 25, 2020 22:40

seanmwinn requested review from a team as code owners September 25, 2020 22:40

seanmwinn requested review from a team September 25, 2020 22:40

seanmwinn requested a review from a team as a code owner September 25, 2020 22:40

tgraf requested changes Sep 28, 2020

View reviewed changes

Documentation/install/upgrade.rst Outdated Show resolved Hide resolved

christarazi requested changes Sep 28, 2020

View reviewed changes

install/kubernetes/Makefile Outdated Show resolved Hide resolved

install/kubernetes/cilium/templates/cilium-operator-deployment.yaml Show resolved Hide resolved

vendor/github.com/fsnotify/fsnotify/.travis.yml Outdated Show resolved Hide resolved

aanm reviewed Sep 28, 2020

View reviewed changes

Documentation/concepts/ebpf/_static/kubernetes_iptables.svg Outdated Show resolved Hide resolved

aanm requested changes Sep 28, 2020

View reviewed changes

This was referenced Sep 28, 2020

enable hubble by default #13311

Closed

helm: Add extraConfig in configmap template #13317

Merged

seanmwinn requested a review from a team as a code owner September 29, 2020 21:57

rolinh added the area/helm Impacts helm charts and user deployment experience label Sep 30, 2020

nebril approved these changes Oct 12, 2020

View reviewed changes

joestringer merged commit e9cb43c into cilium:master Oct 12, 2020

This was referenced Oct 12, 2020

Prepare for release v1.9.0-rc1 #13518

Merged

Nodeinit Daemonset no longer pre-pulls Cilium image #13521

Closed

Fix install/kubernetes update-versions make target #13523

Merged

rolinh mentioned this pull request Oct 13, 2020

Helm install gives hubble-relay-client-certs not found for hubble-relay pod #13537

Closed

rolinh added a commit that referenced this pull request Oct 13, 2020

helm: remove random value file

1f22d57

It looks like this file was wrongly committed as part of the Helm refactoring PR (#13259). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

rolinh mentioned this pull request Oct 13, 2020

helm: remove random value file #13538

Merged

rolinh mentioned this pull request Oct 13, 2020

helm: bring back hubble dependencies validation #13539

Merged

This was referenced Oct 13, 2020

CODEOWNERS: fix owner assignment for hubble related helm charts #13540

Merged

helm: remove unused var in make quick-install target #13541

Merged

tklauser pushed a commit that referenced this pull request Oct 14, 2020

helm: remove random value file

4dde09f

It looks like this file was wrongly committed as part of the Helm refactoring PR (#13259). Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>

sayboras mentioned this pull request Oct 14, 2020

helm: Add check for prometheus service monitoring CRDs #13549

Merged

pchaigno mentioned this pull request Oct 14, 2020

docs: Update CI documentation following Helm refactoring #13561

Merged

sayboras mentioned this pull request Oct 15, 2020

helm: auto generate README.md #13583

Closed

gandro mentioned this pull request Oct 15, 2020

helm: Remove unused serviceAccount values #13585

Merged

pchaigno mentioned this pull request Oct 28, 2020

test: Unquarantine K8sUpdates under GKE #13793

Merged

pchaigno added release-blocker/1.9 and removed priority/release-blocker labels Jan 26, 2021

mvisonneau mentioned this pull request Feb 12, 2021

helm: PodSecurityPolicy management regression #14963

Closed

pchaigno mentioned this pull request Feb 26, 2021

Helm: Add support for exposing runtime configuration flags via extraArgs per-container #10502

Closed

nebulon42 reviewed Mar 7, 2021

View reviewed changes

joestringer mentioned this pull request Jul 13, 2021

Restructure helm chart into components #16795

Merged

sayboras mentioned this pull request Apr 16, 2023

Helm: Implement imagePullSecrets to support private repositories #11961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Helm Refactoring #13259

Helm Refactoring #13259

seanmwinn commented Sep 22, 2020 •

edited

Loading

joestringer left a comment •

edited

Loading

tgraf left a comment

christarazi left a comment

aanm left a comment •

edited

Loading

tgraf commented Sep 29, 2020 •

edited

Loading

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

nebulon42 Mar 7, 2021

Helm Refactoring #13259

Helm Refactoring #13259

Conversation

seanmwinn commented Sep 22, 2020 • edited Loading

joestringer left a comment • edited Loading

Choose a reason for hiding this comment

tgraf left a comment

Choose a reason for hiding this comment

christarazi left a comment

Choose a reason for hiding this comment

aanm left a comment • edited Loading

Choose a reason for hiding this comment

tgraf commented Sep 29, 2020 • edited Loading

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 29, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

seanmwinn commented Sep 30, 2020

nebulon42 Mar 7, 2021

Choose a reason for hiding this comment

seanmwinn commented Sep 22, 2020 •

edited

Loading

joestringer left a comment •

edited

Loading

aanm left a comment •

edited

Loading

tgraf commented Sep 29, 2020 •

edited

Loading