Add basic Helm 3 support (with ListReleases only) #1359

SimonAlling · 2019-12-09T08:58:40Z

Description of the change

It implements ListReleases and ListAllReleases using the Helm 3 API.

Note: useHelm3 must be set to true in values.yaml for the changes introduced here to have any effect.

Benefits

We should get one step closer to Helm 3 support.

Possible drawbacks

There are no tests. (Hello, @absoludity!)

Applicable issues

This PR emerged from the discussion in #1309.

Additional information

We went with the name Kubeops this time (instead of Helmer).

@andresmgot

This commit doesn't import said package, but here is an example: import ( "fmt" "helm.sh/helm/v3/pkg/action" ) func Hello() { fmt.Printf("%v\n", new(action.Configuration)) } Most of the changes in this commit are actually made/suggested by @andresmgot.

Tiller Proxy is still used by default. Set useHelm3 to true in values.yaml to use Kubeops instead.

absoludity

LGTM, and I'll take a look at setting up tests so you can continue with the other ones.

Hmm, did you see the CI error? FAIL use-cases/default-deployment.js (71.032s). Not sure if it was an intermittent error, or whether that is reproduced locally? (I mean, nothing should be effected since CI is running without the useHelm3 flag, but haven't dug).

absoludity · 2019-12-09T10:45:18Z

chart/kubeapps/templates/kubeops-deployment.yaml

+      serviceAccountName: {{ template "kubeapps.kubeops.fullname" . }}
+      # Increase termination timeout to let remaining operations to finish before killing the pods
+      # This is because new releases/upgrades/deletions are synchronous operations
+      terminationGracePeriodSeconds: 300


hmm, I expected that this would only be required if the API equivalent of the --wait flag was used? Doesn't a new release return as soon as the yaml resources have been created?

I don't know tbh; this file is basically just tiller-proxy-deployment.yaml adapted for Kubeops.

background here: even if you don't use the --wait flag, the installation/upgrade/deletion of a chart may not return instantaneously if the chart has a hook. For example, in the case of Kubeapps, it's not until the initialRepos are populated (pre-install hook) when the rest of the chart is installed.

absoludity · 2019-12-09T10:47:37Z

chart/kubeapps/templates/kubeops-deployment.yaml

+        - name: POD_NAMESPACE
+          valueFrom:
+            fieldRef:
+              fieldPath: metadata.namespace


Not needed straight away, but keep in mind we'll want to include liveness/readiness probes too (I only just hooked up the ones for tiller-proxy in #1348 which were never added).

I see. Shouldn't be any problem.

absoludity · 2019-12-09T10:53:22Z

...ntroller/pkg/client/clientset/versioned/typed/apprepository/v1alpha1/apprepository_client.go

@@ -71,7 +70,7 @@ func setConfigDefaults(config *rest.Config) error {
 	gv := v1alpha1.SchemeGroupVersion
 	config.GroupVersion = &gv
 	config.APIPath = "/apis"
-	config.NegotiatedSerializer = serializer.DirectCodecFactory{CodecFactory: scheme.Codecs}
+	config.NegotiatedSerializer = scheme.Codecs.WithoutConversion()


I assume this and the related change below are just from package updates? Or otherwise, why the change?

Most of the modifications in that commit were suggested by @andresmgot so that we could import helm.sh/helm/v3 at all.

absoludity · 2019-12-09T11:03:23Z

pkg/agent/agent.go

+		Namespace:   r.Namespace,
+		Status:      r.Info.Status.String(),
+	}
+}


Great, I'll have a look at testing this and the handler tomorrow. Thanks!

SimonAlling · 2019-12-09T11:17:28Z

Hmm, did you see the CI error? FAIL use-cases/default-deployment.js (71.032s). Not sure if it was an intermittent error, or whether that is reproduced locally? (I mean, nothing should be effected since CI is running without the useHelm3 flag, but haven't dug).

Yep, I saw it. No idea why, really. This line seems to be the one failing:

await expect(page).toMatch("Ready", { timeout: 60000 });

absoludity · 2019-12-10T04:21:18Z

Hmm, did you see the CI error? FAIL use-cases/default-deployment.js (71.032s). Not sure if it was an intermittent error, or whether that is reproduced locally? (I mean, nothing should be effected since CI is running without the useHelm3 flag, but haven't dug).

Yep, I saw it. No idea why, really. This line seems to be the one failing:
await expect(page).toMatch("Ready", { timeout: 60000 });

Hrm, testing this locally, I see the following when I try a default deploy of aerospike (well, anything):

which would explain the failing test. I'll try to find if one of the updates has altered the token authentication which is done in tiller-proxy so you can move on.

I've done some sample tests you can merge for agent/ListReleases in SimonAlling#1 .

@absoludity

Bug detected by @absoludity's tests.

andresmgot

Thanks for the PR @SimonAlling! It mostly looks good to me, I only have a few comments. I am checking the issue that the CI raised, I suspect it's caused by the changes in the go.mod but will need to verify.

andresmgot · 2019-12-10T08:41:54Z

chart/kubeapps/templates/kubeops-deployment.yaml

+      serviceAccountName: {{ template "kubeapps.kubeops.fullname" . }}
+      # Increase termination timeout to let remaining operations to finish before killing the pods
+      # This is because new releases/upgrades/deletions are synchronous operations
+      terminationGracePeriodSeconds: 300


background here: even if you don't use the --wait flag, the installation/upgrade/deletion of a chart may not return instantaneously if the chart has a hook. For example, in the case of Kubeapps, it's not until the initialRepos are populated (pre-install hook) when the rest of the chart is installed.

chart/kubeapps/values.yaml

cmd/kubeops/Dockerfile

cmd/kubeops/main.go

andresmgot · 2019-12-10T08:52:25Z

cmd/kubeops/main.go

+	// Will panic below if an invalid driver type is provided.
+	driverType := defaultHelmDriver
+	if helmDriverArg != "" {
+		driverType = agent.ParseDriverType(helmDriverArg)
+	} else {
+		// CLI argument was not provided; check environment variable.
+		helmDriverEnv := os.Getenv(driverEnvVar)
+		if helmDriverEnv != "" {
+			driverType = agent.ParseDriverType(helmDriverEnv)
+		}
+	}


it's okay but why supporting the flag and the env var? In any case, I would rather use just the flag with a default value.

We did that because Helm did. I'll be happy to remove the env var if you prefer just the flag.

andresmgot · 2019-12-10T08:58:36Z

pkg/agent/agent.go

+	actionConfig.RESTClientGetter = nil     // TODO replace nil with meaningful value
+	actionConfig.KubeClient = kube.New(nil) // TODO replace nil with meaningful value


are these lines really necessary?

I guess it depends on your definition of "necessary". We probably thought it would be nice to be explicit. What would you recommend: keep or remove?

it's okay to leave it, I guess what I don't understand is the TODO note, why should it have a meaningful value? What's a meaningful value in this context?

I don't know; I still don't fully grasp how everything fits together in this fairly complex system. I thought nil was obviously not meaningful and would need to be replaced sooner or later.

andresmgot · 2019-12-10T08:59:40Z

pkg/agent/agent.go

+		// No (real) enums/ADTs in Go, so no static guarantee against this case.
+		panic("Invalid Helm driver type: " + driverType)


it's better to return an error than panicking, that way the caller can act in consequence.

In this specific case, I'm actually not sure about that. I wanted to have the first argument to agent.createStorage be of a sum type, so that I'd only need to handle exactly the cases represented by that sum type. There seems to be no sum types in Go, so said first argument instead has the quasi sum type DriverType. This at least means that values like os.Getenv("LOL")) don't typecheck:

cannot use os.Getenv("LOL") (type string) as type agent.DriverType in argument to handler.WithAgentConfig

However, values like "hello" apparently count as members of DriverType, but I found no way around that.

I think returning an error in createStorage would introduce a lot of (IMO) unnecessary complexity in agent.NewActionConfig and handler.WithAgentConfig. I also think a programmer will understand that they're not supposed to pass "hello", but an actual DriverType (e.g. agent.Secret).

At some point, you want to be able to say "I have a value of type T – I know it has been validated because it is a T." Parse, don't validate. Go doesn't seem to help us more than I've already made it do.

I'm much more inclined to make agent.ParseDriverType return an error instead of panicking, because, unlike createStorage, it promises that it can handle any string. Its current deceitfulness stems from my interpretation of @absoludity's suggestion that Kubeops should refuse to start if an invalid driver type has been specified. I'll be more than happy to return an error instead.

If I understand this correctly, you mean that driverType was validated before so this won't never panic, right? (because the driver was parsed before). If that's the case then it's fine, I just found it weird for a function in a library to panic.

Yeah, it is weird. But Go doesn't seem to let me express a good old sum type, so this was the best solution I could come up with. It is correct that createStorage should never panic, because driverType should really be a DriverType. The only way I know to violate this assumption is to pass a string literal as driverType.

andresmgot · 2019-12-10T09:12:20Z

pkg/chart/chart.go

@@ -320,7 +320,7 @@ func (c *Chart) InitNetClient(details *Details) (HTTPClient, error) {

 	defaultHeaders := http.Header{"User-Agent": []string{c.userAgent}}
 	if auth.Header != nil {
-		secret, err := c.kubeClient.Core().Secrets(namespace).Get(auth.Header.SecretKeyRef.Name, metav1.GetOptions{})
+		secret, err := c.kubeClient.CoreV1().Secrets(namespace).Get(auth.Header.SecretKeyRef.Name, metav1.GetOptions{})


why this change?

I don't know. I think you suggested it. (Do note that it's part of 1e1a3ba.)

I guess that was me trying things, you don't need this change

SimonAlling · 2019-12-10T10:43:24Z

I just tried deploying Aerospike as well, and I wasn't able to reproduce the issue. I trust you and CircleCI more than myself, though.

Having submitted this comment, I tried deploying Airflow (from stable) and ran into this error:

Sorry! Something went wrong.

rpc error: code = Unknown desc = [unable to recognize "": no matches for kind "Deployment" in version "extensions/v1beta1", unable to recognize "": no matches for kind "StatefulSet" in version "apps/v1beta2"]

But no other app I've tried has thrown any error.

SimonAlling · 2019-12-10T11:04:20Z

I think it would be interesting to run 1e1a3ba through CI, because it's the only commit on this branch that I'd reasonably expect to affect any existing functionality.

andresmgot · 2019-12-10T11:06:28Z

I was able to also reproduce the issue (I just built the tiller-proxy image in this branch). The issue is related to the upgrade of client-go. I am not sure why but here:

https://github.com/kubeapps/kubeapps/blob/master/pkg/auth/auth.go#L77

The response returned is empty so both allowed and denied are false:

Status:SubjectAccessReviewStatus{Allowed:false,Reason:,EvaluationError:,Denied:false,}

If you are not able to reproduce the issue, it may be related to an issue with kind (which is the dev environment in the CI).

andresmgot · 2019-12-10T12:00:15Z

If you are not able to reproduce the issue, it may be related to an issue with kind (which is the dev environment in the CI).

I am also able to reproduce it with minikube so I guess it's a different thing (btw, using Kubernetes 1.16)

vmware-tanzu#1359 (comment)

andresmgot · 2019-12-10T15:10:12Z

Finally found the issue (this one was a tough one). The problem is that the BearerTokenFile was set when creating the config in the auth module and now, it's taking precedence over the BearerToken. Because of that, it's using the default serviceaccount to validate all the actions. To fix the issue, add the line:

--- a/pkg/auth/auth.go
+++ b/pkg/auth/auth.go
@@ -105,6 +105,7 @@ func NewAuth(token string) (*UserAuth, error) {
        }
        // Overwrite default token
        config.BearerToken = token
+       config.BearerTokenFile = ""
        kubeClient, err := kubernetes.NewForConfig(config)
        if err != nil {
                return nil, err

here: https://github.com/kubeapps/kubeapps/blob/master/pkg/auth/auth.go#L107

@andresmgot

As suggested by @andresmgot: vmware-tanzu#1359 (comment)

andresmgot

It's okay for me to have a starting point. Leaving for @absoludity to do a final review. Thanks!

SimonAlling · 2019-12-10T15:53:25Z

It's okay for me to have a starting point. Leaving for @absoludity to do a final review. Thanks!

Thank you so much for helping me out with the debugging! 😃

absoludity

Excellent - great work digging on the token issue Andres. Thanks Simon - great to start getting this in master!

Related discussion: vmware-tanzu#1359 (comment)

SimonAlling added 2 commits December 9, 2019 09:23

Add basic Helm 3 support (with ListReleases only)

74137c5

Tiller Proxy is still used by default. Set useHelm3 to true in values.yaml to use Kubeops instead.

absoludity reviewed Dec 9, 2019

View reviewed changes

absoludity and others added 4 commits December 10, 2019 09:06

Add example test for ListReleases

a57736e

Filter by namespace in ListReleases

b8daff0

Bug detected by @absoludity's tests.

Take list limit as parameter in ListReleases

95cbfd5

Add listLimit test case

643059f

andresmgot reviewed Dec 10, 2019

View reviewed changes

Remove "necessary???" comments"

e8e6918

SimonAlling added 3 commits December 10, 2019 15:39

Rename agent to kubeops

be1763f

vmware-tanzu#1359 (comment)

Use real values for registry and repository

743b96a

vmware-tanzu#1359 (comment)

Remove HELM_DRIVER environment variable

117fcf1

vmware-tanzu#1359 (comment)

Fix issue with bearer token file

9766601

As suggested by @andresmgot: vmware-tanzu#1359 (comment)

andresmgot approved these changes Dec 10, 2019

View reviewed changes

absoludity approved these changes Dec 10, 2019

View reviewed changes

absoludity merged commit f6cd36c into vmware-tanzu:master Dec 10, 2019

SimonAlling added a commit to SimonAlling/kubeapps that referenced this pull request Dec 11, 2019

Don't panic in ParseDriverType

1fb2b6f

Related discussion: vmware-tanzu#1359 (comment)

SimonAlling mentioned this pull request Dec 11, 2019

Don't panic in ParseDriverType #1363

Merged

latiif mentioned this pull request Dec 12, 2019

Improve TestListReleases for /pkg/agent #1369

Merged

SimonAlling deleted the helm3-list-releases branch December 12, 2019 13:36

latiif mentioned this pull request Dec 18, 2019

Add Helm 3 CreateRelease #1399

Merged

latiif mentioned this pull request Jan 2, 2020

[kubeops] Implement framework for testing /kubeops/internal/handler #1408

Merged

SimonAlling mentioned this pull request Jan 22, 2020

Add Helm 3 support #1309

Closed

		actionConfig.RESTClientGetter = nil // TODO replace nil with meaningful value
		actionConfig.KubeClient = kube.New(nil) // TODO replace nil with meaningful value

		// No (real) enums/ADTs in Go, so no static guarantee against this case.
		panic("Invalid Helm driver type: " + driverType)

Add basic Helm 3 support (with ListReleases only) #1359

Add basic Helm 3 support (with ListReleases only) #1359

Conversation

SimonAlling commented Dec 9, 2019

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonAlling commented Dec 9, 2019

absoludity commented Dec 10, 2019

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonAlling commented Dec 10, 2019 • edited Loading

Sorry! Something went wrong.

SimonAlling commented Dec 10, 2019

andresmgot commented Dec 10, 2019

andresmgot commented Dec 10, 2019

andresmgot commented Dec 10, 2019

andresmgot left a comment

Choose a reason for hiding this comment

SimonAlling commented Dec 10, 2019

absoludity left a comment

Choose a reason for hiding this comment

SimonAlling commented Dec 10, 2019 •

edited

Loading