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

MINIKUBE_ADDONS environment variable ignored #11171

Closed
davecardwell opened this issue Apr 23, 2021 · 11 comments · Fixed by #11469
Closed

MINIKUBE_ADDONS environment variable ignored #11171

davecardwell opened this issue Apr 23, 2021 · 11 comments · Fixed by #11469
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.

Comments

@davecardwell
Copy link

minikube start does not support the MINIKUBE_ADDONS environment variable as a proxy for the --addons argument as it does with other options.

Steps to reproduce the issue:

  1. MINIKUBE_PROFILE=this-works MINIKUBE_ADDONS=registry minikube start
  2. MINIKUBE_PROFILE=this-works minikube addons list
  3. Note that MINIKUBE_PROFILE has taken effect (the profile is set to this-works), but MINIKUBE_ADDONS has not (the registry addon has not been enabled).

Full output of failed command:

$ MINIKUBE_PROFILE=this-works MINIKUBE_ADDONS=registry minikube start

😄  [this-works] minikube v1.19.0 on Darwin 10.15.7
    ▪ MINIKUBE_ADDONS=registry
    ▪ MINIKUBE_PROFILE=this-works
✨  Automatically selected the hyperkit driver. Other choices: virtualbox, ssh
👍  Starting control plane node this-works in cluster this-works
🔥  Creating hyperkit VM (CPUs=2, Memory=4000MB, Disk=20000MB) ...
🐳  Preparing Kubernetes v1.20.2 on Docker 20.10.4 ...
    ▪ Generating certificates and keys ...
    ▪ Booting up control plane ...
    ▪ Configuring RBAC rules ...
🔎  Verifying Kubernetes components...
    ▪ Using image gcr.io/k8s-minikube/storage-provisioner:v5
🌟  Enabled addons: storage-provisioner, default-storageclass
🏄  Done! kubectl is now configured to use "this-works" cluster and "default" namespace by default


$ MINIKUBE_PROFILE=this-works minikube addons list

|-----------------------------|------------|--------------|
|         ADDON NAME          |  PROFILE   |    STATUS    |
|-----------------------------|------------|--------------|
| ambassador                  | this-works | disabled     |
| auto-pause                  | this-works | disabled     |
| csi-hostpath-driver         | this-works | disabled     |
| dashboard                   | this-works | disabled     |
| default-storageclass        | this-works | enabled ✅   |
| efk                         | this-works | disabled     |
| freshpod                    | this-works | disabled     |
| gcp-auth                    | this-works | disabled     |
| gvisor                      | this-works | disabled     |
| helm-tiller                 | this-works | disabled     |
| ingress                     | this-works | disabled     |
| ingress-dns                 | this-works | disabled     |
| istio                       | this-works | disabled     |
| istio-provisioner           | this-works | disabled     |
| kubevirt                    | this-works | disabled     |
| logviewer                   | this-works | disabled     |
| metallb                     | this-works | disabled     |
| metrics-server              | this-works | disabled     |
| nvidia-driver-installer     | this-works | disabled     |
| nvidia-gpu-device-plugin    | this-works | disabled     |
| olm                         | this-works | disabled     |
| pod-security-policy         | this-works | disabled     |
| registry                    | this-works | disabled     |
| registry-aliases            | this-works | disabled     |
| registry-creds              | this-works | disabled     |
| storage-provisioner         | this-works | enabled ✅   |
| storage-provisioner-gluster | this-works | disabled     |
| volumesnapshots             | this-works | disabled     |
|-----------------------------|------------|--------------|
💡  To see addons list for other profiles use: `minikube addons -p name list
@medyagh
Copy link
Member

medyagh commented Apr 25, 2021

@davecardwell than you for creating this sue, not all our flags are configurable by ENV but this should be easy to add support for it. (Since we are using cobra)

Would you be interested to contribute this as a PR to minikube ?

@ilya-zuyev ilya-zuyev added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. labels Apr 28, 2021
@davecardwell
Copy link
Author

@medyagh I did try to take a quick look prior to reporting, but I’m not a Go programmer so didn’t get beyond looking for any obvious logic issues.

I noticed that unlike most of the options, --addons goes straight into &config.AddonList from k8s.io/minikube/pkg/minikube/config. It is then not made part of the ClusterConfig like most of the other flags are here. I assumed something going awry due to these differences, but didn’t look any further.

I will try to make time to set up the code and write a test case at least to see if I can figure it out. If anybody else with Go experience wants to jump in before then though I’d be fine with that! ;)

@govargo
Copy link
Contributor

govargo commented Apr 29, 2021

Hi @davecardwell

Thank you for creating this issue.
I would like to say for this feature.

As you said, the addons flag is through at here.

startCmd.Flags().StringSliceVar(&config.AddonList, "addons", nil, "Enable addons. see `minikube addons list` for a list of valid addon names.")

And the config.AddonList is used at here.
// enable addons, both old and new!
if starter.ExistingAddons != nil {
if viper.GetBool("force") {
addons.Force = true
}
wg.Add(1)
go addons.Start(&wg, starter.Cfg, starter.ExistingAddons, config.AddonList)
}

So I think if you set some code about environment MINIKUBE_ADDONS to cmd/minikube/cmd/start_flags.go or
pkg/minikube/node/start.go, minikube can set its value to config.AddonList.

https://ordina-jworks.github.io/development/2018/10/20/make-your-own-cli-with-golang-and-cobra.html#using-environment-variables-instead-of-flags

I'm looking forward to your codes. If you have any questions, please let us know.

@davecardwell
Copy link
Author

@govargo Thank you for the additional context. I think the main part I was unsure of was why the viper.AutomaticEnv() stuff was not working for MINIKUBE_ADDONS as it seemed to be for the rest of the start options:

viper.SetEnvPrefix(minikubeEnvPrefix)
// Replaces '-' in flags with '_' in env variables
// e.g. iso-url => $ENVPREFIX_ISO_URL
viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_"))
viper.AutomaticEnv()

I will try to investigate further soon.

@bisakhmondal
Copy link

Hi all, I dig into the problem and it turns out that that the problem is with the viper from spf13. We are trying to capture a slice but it seems viper is unable to do so.

  1. passing as a flag : single item : works ✔️
    image

  2. passing as a flag : multiple items : does not work ❌
    image

  3. passing and environment variable: ❌

I think the issue is with StringSliceVar.

@medyagh
Copy link
Member

medyagh commented May 12, 2021

Hi all, I dig into the problem and it turns out that that the problem is with the viper from spf13. We are trying to capture a slice but it seems viper is unable to do so.

  1. passing as a flag : single item : works ✔️
    image
  2. passing as a flag : multiple items : does not work ❌
    image
  3. passing and environment variable: ❌

I think the issue is with StringSliceVar.

@bisakhmondal seems like you found a bug ! would u like to make a PR to fix it ?

@spowelljr spowelljr added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label May 17, 2021
@tangledbytes
Copy link
Contributor

@medyagh can I work on this issue? // @spowelljr

@spowelljr
Copy link
Member

@Utkarsh-pro For sure! Just comment /assign and the issue will be assigned to you.

@tangledbytes
Copy link
Contributor

Thanks @spowelljr!

/assign

@DimitrijeManic
Copy link

@Utkarsh-pro Do you plan on making it possible to set addons in the config or just ENV variables?

@tangledbytes
Copy link
Contributor

@DimitrijeManic, PR #11469 only adds support for setting addons using environmental variables (in addition to --addons flag). I am not sure if increasing the scope of the issue would be appropriate.

However, I'd be more than happy to add support for config too if it's a requirement and is accepted by the admins 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants