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

[regression] Cannot control which chart versions are displayed #6020

Closed
absoludity opened this issue Feb 23, 2023 · 2 comments · Fixed by #6022
Closed

[regression] Cannot control which chart versions are displayed #6020

absoludity opened this issue Feb 23, 2023 · 2 comments · Fixed by #6022
Assignees
Labels
kind/bug An issue that reports a defect in an existing feature

Comments

@absoludity
Copy link
Contributor

In the past we had solved the problem of how to display a useful set of versions to users in #3588, but the issue appears to have regressed and needs investigation:

From @linhvuntu #3588 (comment)

So I tried to create a brand new kubeapps with Helm Package version 12.2.4 / App Version 2.6.3, and with the config of {10 10 10} and the logs are as below

I0223 04:40:53.660198       1 root.go:36] "kubeapps-apis has been configured with serverOptions" serverOptions={Port:50051 PluginDirs:[/plugins/helm-packages /plugins/resources] ClustersConfigPath:/config/clusters.conf PluginConfigPath:/config/kubeapps-apis/plugi │ns.conf PinnipedProxyURL:http://kubeapps-internal-pinniped-proxy.kubeapps:3333 PinnipedProxyCACert: GlobalHelmReposNamespace:kubeapps-v3 UnsafeLocalDevKubeconfig:false QPS:50 Burst:100}                                                                               │
I0223 04:40:53.941472       1 server.go:101] +helm NewServer(globalPackagingCluster: [default], globalPackagingNamespace: [kubeapps-v3], pluginConfigPath: [/config/kubeapps-apis/plugins.conf]                                                                         
I0223 04:40:53.941671       1 server.go:112] +helm using custom config: [{{10 10 10} 300 }]                                                                                                                                                                             
I0223 04:40:54.029400       1 server.go:124] +helm NewServer effective globalPackagingNamespace: [kubeapps-v3]                                                                                                                                                          
I0223 04:40:54.030557       1 plugins.go:152] "Successfully registered plugin" pluginPath="/plugins/helm-packages/helm-packages-v1alpha1-plugin.so"                                                                                                                     │
I0223 04:40:54.332092       1 server.go:119] +resources using custom config: [{{ }}]

So it seems updated successfully, but when checking the Package Version dropdown list, it still display in with the setting of {3 3 3}

The Helm package I'm checking is the public ingress-nginx chart from Kubernetes here: https://github.com/kubernetes/ingress-nginx/tree/main/charts/ingress-nginx

When I check the this chart versions from terminal, it shows full list of versions as below

helm search repo ingress-nginx/ingress-nginx -l

NAME                       	CHART VERSION	APP VERSION	DESCRIPTION
ingress-nginx/ingress-nginx	4.5.2        	1.6.4      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.5.0        	1.6.3      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.4.2        	1.5.1      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.4.1         	1.5.2      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.4.0        	1.5.1      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.3.0        	1.4.0      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.5        	1.3.1      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.4        	1.3.1      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.3        	1.3.0      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.2        	1.3.0      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.1         	1.3.0      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.2.0        	1.3.0      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.1.4         	1.2.1      	Ingress controller for Kubernetes using NGINX a...
ingress-nginx/ingress-nginx	4.1.3         	1.2.1      	Ingress controller for Kubernetes using NGINX a...

... and more ..

But from the Package Version dropdown list in the Kubeapps Dashboards, it show only

4.5.2 
4.5.0
4.4.2
4.4.1
4.4.0
4.3.0
3.41.0
3.40.0
3.39.0
.. and more ..

For some specific reason, we need to use the Chart version 4.2.5 but it is not displayed.

Do I need to config anything else at the kubeappsController or elsewhere? I have cleared cache, use another laptop to check but it's still the same

@absoludity absoludity added the kind/bug An issue that reports a defect in an existing feature label Feb 23, 2023
@absoludity absoludity self-assigned this Feb 23, 2023
@linhvuntu
Copy link

thanks in advance :)

@absoludity
Copy link
Contributor Author

thanks in advance :)

Thank you for demoing the issue. So I've reproduced this, and fixed it locally but want to write a test to ensure it's covered (so it can't regress again).

For those interested, the issue was a change that unintentionally loads the config in an inner scope:

// If no config is provided, we default to the existing values for backwards
// compatibility.
pluginConfig := common.NewDefaultPluginConfig()
if pluginConfigPath != "" {
pluginConfig, err := common.ParsePluginConfig(pluginConfigPath)
if err != nil {
log.Fatalf("%s", err)
}
log.Infof("+helm using custom config: [%v]", *pluginConfig)
} else {
log.Info("+helm using default config since pluginConfigPath is empty")
}

So

  • on line 106 a variable pluginConfig is created
  • on line 108 two new variables, pluginConfig and err are created (:=) in the inner scope
  • on line 112 we helpfully print out the parsed config
  • at line 115 the variables from the inner scope are no longer used, and the variable from line 106 remains empty.

Case of an extra colon :/ and our issue for not testing that properly (I'll do that tomorrow and submit the fix).

absoludity added a commit that referenced this issue Feb 24, 2023
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
In [#5339 we inadvertently
switched](https://github.com/vmware-tanzu/kubeapps/pull/5339/files#diff-da1b1406426f28c45f4b63a5ae7547e9c29747e118bf70df6a4694d575710f94R104)
from an [assignment](https://go.dev/ref/spec#Assignment_statements) to a
[short variable
declaration](https://go.dev/ref/spec#Short_variable_declarations):

```diff
-		pluginConfig, err = common.ParsePluginConfig(pluginConfigPath)
+		pluginConfig, err := common.ParsePluginConfig(pluginConfigPath)
```

which meant that, in context, a *new* `pluginConfig` var was created in
that scope, so that the actual `pluginConfig` var from the outside scope
remained empty.

### Benefits

Plugin config works again for the Helm plugin.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- fixes #6020 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->
I spent some time trying to write a test for this, but I could either
extract the 5 lines to a separate function and test that or test the
`NewServer` function itself. The latter is very difficult because it
instantiates a bunch of network requests, and the former seems redundant
since we already extract the functionality to the
`common.ParsePluginConfig()` function... it's really just a golang
gotcha that is important to be aware of: `a := foo` is equivalent to
`var a atype \n a = foo` and so is always creating a new variable in
that scope.

Signed-off-by: Michael Nelson <minelson@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug An issue that reports a defect in an existing feature
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants