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

It's much too easy to accidentally trigger flux install #494

Closed
luisr-escobar opened this issue Oct 6, 2023 · 4 comments · Fixed by #498
Closed

It's much too easy to accidentally trigger flux install #494

luisr-escobar opened this issue Oct 6, 2023 · 4 comments · Fixed by #498

Comments

@luisr-escobar
Copy link

luisr-escobar commented Oct 6, 2023

Expected behaviour

After upgrading the flux cli with brew to v2.1.1 the gitops plugin stopped showing soruces/workloads with just an error showing "unable to load". After uninstalling and reinstalling the plugin, the prompt "enable gitops" appeared on the plugin UI screen. After clicking "enable gitops", it triggered the flux install command to run without any confirmation which caused CRD definitions to be irreversibly upgraded from v1beta2 to v1

This might be related to issue 489

Actual behaviour

If a cli upgrade makes the gitops plugin incompatible with the flux CRD api versions it should not prompt a user to "enable gitops" on the k8s cluster, instead it should ideally show an incompatibility error.

Steps to reproduce

  • Update flux cli using brew from v1.x.x to v2.x.x
  • The plugin is no longer able to show sources or workloads any further for the cluster using v1beta2 CRDs
  • Uninstall/Install GitOps Tools for Flux
  • The plugin screen in vs code shows the option to "enable gitops"

Versions

kubectl version: v1.27.2
Flux version: v2.1.1
Git version: 2.39.3
Extension version: v0.25.4
VSCode version: 1.83.0
Operating System (OS) and its version: MacOS Ventura 13.6 (arm64)

@juozasg
Copy link
Collaborator

juozasg commented Oct 13, 2023

@luisr-escobar Thank you very much for the bug report!

I was unsuccessful trying to reproduce the issue. I installed flux v0.41 from Homebrew and I used it to create a v1beta2 GitRepository. I upgraded to flux v2.1.1 using Homebrew. I am still seeing v1beta GitRepository listed in Sources view. As expected, I can't perform operations that require flux CLI because of version incompatability but the resources are listed.

The extension does not use flux CLI to view flux resources on the cluster. It uses kubectl get GitRepository.source.toolkit.fluxcd.io or the javascript client API. The API is built from kubectl api-resources -o wide query. We map short names like GitRepository to full API endpoints we use to query objects. Load errors are shown when these queries fail or timeout.

"Enable GitOps" option is shown when no flux controllers are found. Flux controllers are listed using kubectl get deployment --namespace=flux-system -o json

Seeing the code I wonder if this problem is triggered when flux is installed in a non-default namespace? In that case we should have setting to override the namespace.

@kingdonb
Copy link
Collaborator

I think we should rewrite our tutorials and remove "Enable GitOps" - or at least hide it behind some warnings.

The flux install command should never be run against a production cluster, and we make it very easy to accidentally trigger that without understanding what possible implications there are. CRD upgrades are irreversible. We should make clearing this issue a priority.

On the other hand, simply having permission to run flux install on production clusters could be considered a misconfiguration. I'd rather not let that be an excuse for leaving the state as it is, where a simple detection error can result in users being unaware they are one click away from messing up their cluster's inbuilt Flux CRD.

The detection could fail in any one of numerous cases, I'm not sure which one would have triggered. We've seen strange instances of Flux controllers for example when Azure AKS Flux is installed, though those particular issues have been addressed, Microsoft Azure does not have a monopoly on the Flux clones market and we may find other pathological behavior that could be prevented by adding one more step between "Enable GitOps" and the irreversible application of flux install.

It is another issue that flux install itself does not provide these guard-rails internally, but we should certainly provide guard-rails in the extension if it's meant to be able to serve as a Flux user's entire first interaction with GitOps and Flux.

@kingdonb kingdonb changed the title Flux does not detect flux installation after upgrading flux cli with brew GitOps Tools extension makes it much too easy to accidentally trigger flux install Oct 25, 2023
@kingdonb kingdonb changed the title GitOps Tools extension makes it much too easy to accidentally trigger flux install It's much too easy to accidentally trigger flux install Oct 25, 2023
@luisr-escobar
Copy link
Author

luisr-escobar commented Oct 26, 2023

Thanks a lot @juozasg @kingdonb for looking into the issue. We have flux installed in the flux-system namespace and since that incident (fortunately was on our dev environment) haven't seen or been able to replicate the issue so I'm not sure if reloading vs code or the plugin helped address the issue. I think having a guardrail that at a minimum brings up a screen confirming if you would like to install flux on the target k8s cluster would be ideal.

@kingdonb
Copy link
Collaborator

kingdonb commented Nov 1, 2023

We are addressing this for the next release, and Flux upstream has merged a PR to address it as well:

In future releases of Flux, when you run flux bootstrap or flux install you will get a prompt if you're about to override an installation of Flux that is not already managed that way.

And in the edge release of our extension as of yesterday, there is a prompt in the extension as well, since "Enable GitOps" isn't necessarily clear about what it's going to do, you'll see a prompt that makes it clear flux install is what it's going to do.

Keeping this issue open until that lands in a mainline release 🙇 🙏

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 a pull request may close this issue.

3 participants