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

Add Helm-based install / uninstall commands #1475

Merged
merged 2 commits into from
Mar 31, 2023

Conversation

michi-covalent
Copy link
Contributor

@michi-covalent michi-covalent commented Mar 26, 2023

please review commit by commit. there are 2 commits:

  • refactoring a bit to support helm-based install / uninstall commands.
  • implement install / uninstall commands.

let's implement other commands in separate PRs to keep the PR size reasonable.

ref: #1396

@michi-covalent michi-covalent temporarily deployed to ci March 26, 2023 21:55 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 26, 2023 22:09 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 26, 2023 22:23 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 27, 2023 20:47 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 27, 2023 23:34 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 27, 2023 23:42 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 27, 2023 23:49 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 27, 2023 23:54 — with GitHub Actions Inactive
- Don't fail the installation validation if --encryption flag is not
  specified. It should behave the same way as if the flag is set to
  "disabled".
- Return the resolved Helm chart from ResolveHelmChartVersion so that
  Helm can use the chart later.
- Expose RESTClientGetter. Helm client needs it.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent temporarily deployed to ci March 28, 2023 00:38 — with GitHub Actions Inactive
@michi-covalent michi-covalent temporarily deployed to ci March 28, 2023 00:58 — with GitHub Actions Inactive
@michi-covalent michi-covalent changed the title test install/uninstall Add Helm-based install / uninstall commands Mar 28, 2023
@michi-covalent michi-covalent marked this pull request as ready for review March 28, 2023 15:43
@michi-covalent michi-covalent requested review from a team as code owners March 28, 2023 15:43
@tklauser tklauser requested a review from a team March 30, 2023 14:02
defaults/defaults.go Outdated Show resolved Hide resolved
install/install.go Outdated Show resolved Hide resolved
internal/cli/cmd/install.go Outdated Show resolved Hide resolved
@michi-covalent
Copy link
Contributor Author

thank you for your review @aanm & @tklauser ! tobias, all of your suggestions make sense. i'm updating the PR now 🚀

@michi-covalent michi-covalent temporarily deployed to ci March 30, 2023 14:38 — with GitHub Actions Inactive
This is an alternative implementation to manage Cilium installations.
It uses Helm actions to manage Cilium instead of generating Kubernetes
resources from the Helm chart and managing them using the Kubernetes
APIs directly.

These commands are disabled by default. Set CILIUM_CLI_MODE environment
variable to "helm" to enable these commands.

Ref: #1396

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@michi-covalent michi-covalent temporarily deployed to ci March 30, 2023 14:57 — with GitHub Actions Inactive
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

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

This first stage LGTM, some questions below.

@@ -22,7 +22,13 @@ jobs:
installation-and-connectivity:
runs-on: ubuntu-22.04
timeout-minutes: 40
strategy:
matrix:
mode: ["classic", "helm"]
Copy link
Member

Choose a reason for hiding this comment

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

Am I correct in that later on we will drop the classic mode and only have Helm available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we'll gradually transition to the helm mode, and then deprecate the classic mode ✨

- name: Install Cilium with IPsec Encryption
if: ${{ matrix.mode == 'helm' }}
run: |
kubectl create -n kube-system secret generic cilium-ipsec-keys \
Copy link
Member

Choose a reason for hiding this comment

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

Can't we still have this handled by the CLI even in Helm mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we could. i do want to have a discussion first though and come up with a general guideline on how cilium-cli should handle resources that are not part of the helm chart.

on the one hand, these flags are very convenient. on the other hand, i think it's a very slippery slope. we might end up adding more and more logic to cilium-cli without carefully considering:

  • what resources should be managed by cilium, and what resources should be managed by the user.
  • if cilium should be managing the resource, why it's not part of the helm chart.

.github/workflows/kind.yaml Show resolved Hide resolved
@michi-covalent michi-covalent merged commit 3760fd7 into master Mar 31, 2023
@michi-covalent michi-covalent deleted the pr/michi/helminstall branch March 31, 2023 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants