First step of AddPackageRepository - Package Repository API for Helm #4723

castelblanque · 2022-05-17T21:45:17Z

Description of the change

Adds a basic implementation of AddPackageRepository method from package repository API for the Helm plugin (#3496). With basic I mean:

No Docker credentials supported yet
No TLS config with secret ref (it is, though, the TLS CA)
No custom fields for Helm repos (e.g. template for cronjobs)

Package repositories for Helm plugin can be created now with the common repository API definition, e.g.

{
  "context": { "namespace": "example-ns" },
  "name": "example-repo",
  "type": "helm",
  "namespace_scoped": true,
  "url": "https://charts.bitnami.com/bitnami"
}

The above example will create a repository in the namespace example-ns.

Benefits

Repositories can be created meeting the new Package Repository API definition.

Possible drawbacks

None that I know. Outcome of this implementation should be the same AppRepository CRD created by the already existing API.
Don't see a problem with both APIs coexisting.

Applicable issues

Partially implements #4662.

Additional information

Following the example above, the one below will have the repo created in the global namespace, even if there is a namespace specified:

{
  "context": { "namespace": "example-ns" },
  "name": "example-repo",
  "type": "helm",
  "namespace_scoped": false,
  "url": "https://charts.bitnami.com/bitnami"
}

That does not seem very consistent to me, should we return failure in this case?

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

netlify · 2022-05-17T21:45:25Z

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	`cff7999`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/628f914d7fbfe00008a04f16

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

absoludity · 2022-05-25T00:59:47Z

Hey @castelblanque . Did you check with @dlaloue-vmware about reviewing each other work? I can jump in and review if needed, but I think since you're both working on similar aspects for different plugins, it'd be a great chance to review each others and learn together?

castelblanque · 2022-05-25T06:48:01Z

You are totally right @absoludity. Will do!

dlaloue-vmware · 2022-05-26T05:08:35Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go

@@ -1024,3 +996,75 @@ func (s *Server) GetInstalledPackageResourceRefs(ctx context.Context, request *c
 		}, nil
 	}
 }
+
+func (s *Server) AddPackageRepository(ctx context.Context, request *corev1.AddPackageRepositoryRequest) (*corev1.AddPackageRepositoryResponse, error) {
+	log.Infof("+helm AddPackageRepository [%v]", request)


logging the whole request may leak credentials. it would be better to abstract only what is necessary (see how it is done in the method CreateInstalledPackage)

It is a line copied from Flux plugin here.
If it is just to locate the operation in the logs, I would say that name and URL should suffice.
@gfichtenholt what do you think?

I needed more than name and URL when I was working on flux as some point. I debugged and found and fixed the problem, whatever it was, then and this just kind of remained. I think @dlaloue-vmware is right, could you fix it and also fix one in flux as well if you have the bandwidth. Thanks in advance

dlaloue-vmware · 2022-05-26T05:11:35Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go

+		return nil, status.Errorf(codes.InvalidArgument, "no package repository Name provided")
+	}
+	name := types.NamespacedName{Name: request.Name}
+	if request.GetNamespaceScoped() {


i think we have agreed to validate that the namespace and scope were consistent (e.g. if scope is global but namespace is not the global namespace, then it should be a validation error)

Ok, added the check.

dlaloue-vmware · 2022-05-26T05:12:51Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/server.go

+	if request == nil {
+		return nil, status.Errorf(codes.InvalidArgument, "no request provided")
+	}
+	if request.Context == nil || request.Context.Namespace == "" {


this is just a suggestion, but if the namespace is not provided, we could default it to the global namespace.
this makes it easier for users as the global namespace is not required to be known

Yep, it makes sense. Will add it.

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories.go

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque · 2022-05-26T14:46:22Z

@dlaloue-vmware Thanks for the review!

antgamdia

Thanks for the changes! LGTM
I wanted to get into the UI revamp to refresh my knowledge regarding repos before looking at the PR.

Minor side coment:

Have a look at remove un-needed .../plugin/... from URL paths in flux2.proto #4708 (comment) where Greg commented about the file: cmd/apprepository-controller/vendor/k8s.io/code-generator

antgamdia · 2022-05-27T08:03:20Z

cmd/kubeapps-apis/docs/kubeapps-apis.swagger.json

+    {
+      "name": "HelmRepositoriesService"
+    },


No action required in this PR, but just mentioning it for all us to be aware: the openapi.yaml file (which is the doc used for the interactive REST API portal) will get out of sync.

Also, but unrelated: I don't know why my generator is different (meaning worse) than yours haha.

antgamdia · 2022-05-27T08:10:21Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_auth.go

+}
+
+// this func is only used with kubeapps-managed secrets
+func newSecretFromTlsConfigAndAuth(repoName types.NamespacedName,


Didn't we have a shared /pkg/ folder share code across plugins?
I'm not sure, but I'd rather remove any dup code if possible, no?
(mentioning this bc I saw a similar fn in the flux plg)

Some code here is copied from Greg's Flux.
Most of the times due to Flux-specific structs it isn't possible to make it generic. I tried at the beginning but had to undo down the road.
We could do some refactoring in a later iteration if needed.

antgamdia · 2022-05-27T08:15:10Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/globals_vars_test.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+var (


Correct me if I'm wrong here, @absoludity , but I'd rather say we weren't so fans of globals in the tests, preferring instead each test case being defining (and customizing with a given set of test-case properties) the object.

So, what about replacing those globals with just functions creating each struct based upon a set of params (where you foresee the variability, I mean).

Ok, I didn't know about that decision. I've used functions to generate structs at some points, but didn't want to convert tests in another set of complex code 😄

Not so much a decision as a recommendation - global vars create shared state that is then used by different tests. Can lead to one test updating that shared state causing hard to debug failures in other tests etc. See https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables for a good discussion.

antgamdia · 2022-05-27T08:18:18Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/repositories_test.go

+		{
+			name:       "returns error if invalid cluster",
+			request:    &corev1.AddPackageRepositoryRequest{Context: &corev1.Context{Cluster: "wrongCluster", Namespace: "foo"}},
+			statusCode: codes.Unimplemented,


Why unimplemented if invalid cluster?

Mmm yes, probably InvalidArgument makes more sense. Thanks!

antgamdia · 2022-05-27T08:31:32Z

cmd/kubeapps-apis/plugins/helm/packages/v1alpha1/testdata/cert/ca-csr.json

@@ -0,0 +1,8 @@
+{
+  "CN": "fluxv2plugin-testdata-ssl-svc.default.svc.cluster.local CA",


nitpick (same in the rest of ocurrences)

Suggested change

"CN": "fluxv2plugin-testdata-ssl-svc.default.svc.cluster.local CA",

"CN": "helmplugin-testdata-ssl-svc.default.svc.cluster.local CA",

Will change it in the following PR that is based in this branch, if you don't mind.

Rafa Castelblanque added 6 commits May 13, 2022 16:04

First step for AddPackageRepository

849c1dd

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

More tests OK and fixed functionality for AddPackageRepository

c38bc44

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Helm Repository API small fix to params

8977746

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Added and fixed some tests for AddPackageRepository

043e483

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Added test for empty secrets

4b022d7

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Fix for Plugin config test

a9b3e72

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Rafa Castelblanque added 6 commits May 17, 2022 23:47

Cleanup in comments

fba6380

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into 4662-addpackagerepoHelm-2

c5d5f21

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge updated from main

ae86ab2

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Secrets check for AddPackageRepo Helm tests

fc94b62

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into 4662-addpackagerepoHelm-2

22e8fc3

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Proto code generated

ccb6a60

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque marked this pull request as ready for review May 20, 2022 16:49

Rafa Castelblanque added 5 commits May 20, 2022 18:55

Added check for OCI repos

f8ab1e8

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into 4662-addpackagerepoHelm-2

695fdc0

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Merge branch 'main' into 4662-addpackagerepoHelm-2

3027a96

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Adjusted Helm plugin config

9a29807

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

Run buf generate

d6e94df

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque requested review from antgamdia and absoludity May 24, 2022 14:18

Added missing copyright headers

12dc3ba

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque requested review from dlaloue-vmware and removed request for absoludity May 25, 2022 06:48

absoludity mentioned this pull request May 26, 2022

Carvelrepos #4788

Merged

dlaloue-vmware reviewed May 26, 2022

View reviewed changes

Code review changes

cff7999

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>

castelblanque requested a review from dlaloue-vmware May 26, 2022 14:46

antgamdia approved these changes May 27, 2022

View reviewed changes

castelblanque merged commit 11e279a into main May 27, 2022

castelblanque deleted the 4662-addpackagerepoHelm-2 branch May 27, 2022 16:34

castelblanque mentioned this pull request May 27, 2022

Second step of package repos API - Package Repository API for Helm #4796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First step of AddPackageRepository - Package Repository API for Helm #4723

First step of AddPackageRepository - Package Repository API for Helm #4723

castelblanque commented May 17, 2022

netlify bot commented May 17, 2022 •

edited

Loading

absoludity commented May 25, 2022

castelblanque commented May 25, 2022

dlaloue-vmware May 26, 2022

castelblanque May 26, 2022

gfichtenholt May 26, 2022

dlaloue-vmware May 26, 2022

castelblanque May 26, 2022

dlaloue-vmware May 26, 2022

castelblanque May 26, 2022

castelblanque commented May 26, 2022

antgamdia left a comment

antgamdia May 27, 2022

antgamdia May 27, 2022

castelblanque May 27, 2022

antgamdia May 27, 2022

castelblanque May 27, 2022

absoludity May 30, 2022

antgamdia May 27, 2022

castelblanque May 27, 2022

antgamdia May 27, 2022

castelblanque May 27, 2022

		@@ -0,0 +1,8 @@
		{
		"CN": "fluxv2plugin-testdata-ssl-svc.default.svc.cluster.local CA",

	"CN": "fluxv2plugin-testdata-ssl-svc.default.svc.cluster.local CA",
	"CN": "helmplugin-testdata-ssl-svc.default.svc.cluster.local CA",

First step of AddPackageRepository - Package Repository API for Helm #4723

First step of AddPackageRepository - Package Repository API for Helm #4723

Conversation

castelblanque commented May 17, 2022

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

netlify bot commented May 17, 2022 • edited Loading

✅ Deploy Preview for kubeapps-dev canceled.

absoludity commented May 25, 2022

castelblanque commented May 25, 2022

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

castelblanque commented May 26, 2022

antgamdia 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

netlify bot commented May 17, 2022 •

edited

Loading