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

cli: add microservice upgrades behind hidden flags #729

Merged
merged 11 commits into from
Dec 19, 2022
Merged

Conversation

derpsteb
Copy link
Member

@derpsteb derpsteb commented Dec 6, 2022

Proposed change(s)

Introduce functionality to upgrade microservices in a running cluster as per update.rfc. Currently an upgrade can be triggered by running constellation upgrade execute --helm. This is only temporary until we restructure the upgrade command according to the RFC. I am also open to change the temporary solution.

--keepValues is disabled for cert-manager and cilium as their docs explicitly warn against using the flag. Reason being that newly introduced values are not picked up when using --keepValues. Therefore I went with the recommended way of fetching the existing values from the cluster and merging them with the loaded values from disk. MergeMaps will prioritize the values from the second argument, see the respective unit test. Calls to MergeMaps should always have the cluster values in the second argument.

Helm does not support updating CRDs as outlined here. There is some discussion on the topic. In the end we will have to decide how to handle CRDs on a case by case basis. We must take extra care of updating CRDs when Constellation users could rely on the installed CRDs.
CRDs updates are managed like the following:

  • cert-manager: the upstream chart uses helm's templates folder to manager them. They intentionally side-step the crds folder in order to have full control of the CRDs. We follow that approach. Constellation users will have to take care that their cert-manager installation is on the same version as Constellation's. This will be described in the docs (ticket is created).
  • cilium: cilium lets it's operator handle CRD installation. Therefore we don't need to worry about those. The cilium upgrade docs also don't mention a need to handle CRDs manually.
  • constellation-operators: during installation helm will automatically install the CRDs included in the crds folder. CRD updates have to be done manually. The current implementation is effectively running kubectl apply -f ./crds/ on the respective folders.
  • constellation-services: no CRDs included.

We may want to include an upgrade rollback command, but won't implement it now. If helm declares an update as successful, but release does not work as expected the docs will help by suggesting relevant helm-rollback commands.

Related issue

Checklist

@derpsteb derpsteb added this to the v2.4.0 milestone Dec 6, 2022
@netlify
Copy link

netlify bot commented Dec 6, 2022

Deploy Preview for constellation-docs ready!

Name Link
🔨 Latest commit d9d284a
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/63a01833acea2e00082b9d60
😎 Deploy Preview https://deploy-preview-729--constellation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@daniel-weisse daniel-weisse left a comment

Choose a reason for hiding this comment

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

I think the place for helm upgrades is fine for now.
The whole upgrade structure should be implemented with v2.4.
If we don't manage to get it done until then we can always hide/remove the flag for the release or think of a more user friendly solution

cli/internal/helm/README.md Outdated Show resolved Hide resolved
cli/internal/helm/charts/cilium/values.yaml Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
cli/internal/helm/client_test.go Outdated Show resolved Hide resolved
cli/internal/helm/client_test.go Outdated Show resolved Hide resolved
cli/internal/helm/client_test.go Outdated Show resolved Hide resolved
internal/deploy/helm/helm_test.go Outdated Show resolved Hide resolved
@derpsteb derpsteb force-pushed the feat/helm-upgrade branch 2 times, most recently from 983da3b to 1f5ef90 Compare December 7, 2022 07:29
@derpsteb
Copy link
Member Author

derpsteb commented Dec 9, 2022

This is currently missing automatic creation of CRD and CR backups. However, to keep PRs smaller I would merge this without backups.

Only merge after 2.3 release.

@derpsteb derpsteb marked this pull request as ready for review December 9, 2022 09:27
cli/internal/cloudcmd/upgrade.go Outdated Show resolved Hide resolved
cli/internal/cmd/upgradeexecute.go Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
cli/internal/helm/client.go Outdated Show resolved Hide resolved
@daniel-weisse
Copy link
Member

Looks good to me.
We should decide on how to handle the helm upgrade flags and then we can merge this 👍

Run with:
constellation upgrade execute --helm.
This will only upgrade the helm charts. No config is needed.
* Use strechr/testify
* Extend parseCRDs test
* Increase Timeout
* Use correct error messages
* Add license header
* Add periods to comments
* hide helm flags
* expand help text
* remove unnecessary  comments
@derpsteb derpsteb merged commit efcd033 into main Dec 19, 2022
@derpsteb derpsteb deleted the feat/helm-upgrade branch December 19, 2022 15:52
@derpsteb derpsteb changed the title MVP for microservice upgrades cli: add microservice upgrades behind hidden flags Jan 11, 2023
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.

2 participants