chore(*) make kumactl install control plane leverage the Helm Charts #989

nickolaev · 2020-08-21T16:26:40Z

Summary

Make the kumactl instal control-plane generate the manifests based in rendering the Helm Charts.

Documentation

Internal

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev · 2020-08-26T08:42:10Z

@austince please check this one

austince

overall looks great, a few nits, ideas, and suggestions

austince · 2020-08-26T14:17:03Z

app/kumactl/cmd/install/testdata/install-control-plane.cni-enabled.golden.yaml

@@ -5472,7 +5513,7 @@ spec:
      terminationGracePeriodSeconds: 5
      containers:
        - name: install-cni
-          image: lobkovilya/install-cni:0.0.1
+          image: kong-docker-kuma-docker.bintray.io/lobkovilya/install-cni:0.0.1


I think this will be fixed with the "kuma.formatImage" comment below

austince · 2020-08-26T14:19:17Z

test/framework/constants.go

+	//kumaCPImage       = kumaImageRegistry + "/" + kumaCPImageRepo
 	kumaDPImageRepo   = "kuma-dp"
 	kumaDPImage       = kumaImageRegistry + "/" + kumaDPImageRepo
 	kumaInitImageRepo = "kuma-init"
-	kumaInitImage     = kumaImageRegistry + "/" + kumaInitImageRepo
+	//kumaInitImage     = kumaImageRegistry + "/" + kumaInitImageRepo


Should these comments be removed?

deployments/charts/kuma/templates/cni-daemonset.yaml

austince · 2020-08-26T17:06:08Z

app/kumactl/cmd/install/render_helm_files.go

+
+			// strip Helm Chart specific labels
+			for _, label := range stripLabels {
+				re := regexp.MustCompile("(?m)[\r\n]+^.*" + label + ".*$")


nit - might want to compile these all ahead of time instead of for each file.

func labelRegex(label string) *regexp.Regexp { return regexp.MustCompile("(?m)[\r\n]+^.*" + label + ".*$") } var stripLabelsRegexps = []*regexp.Regexp{ labelRegex("app.kubernetes.io/managed-by"), labelRegex("helm.sh/chart"), labelRegex("app.kubernetes.io/version"), }

austince · 2020-08-26T17:07:54Z

app/kumactl/cmd/install/render_helm_files.go

+	for _, t := range loadedTemplates {
+		if !strings.HasPrefix(t.Name, "templates/pre-") {
+			loadedChart.Templates = append(loadedChart.Templates, &chart.File{
+				Name: t.Name,
+				Data: t.Data,
+			})
+		}


This makes a lot of sense, though we might want to document the pre- convention for hooks. What about post hooks?

austince · 2020-08-26T17:08:45Z

app/kumactl/cmd/install/render_helm_files.go

+		value := v.FieldByName(name).Interface()
+
+		splitName := strings.Split(name, "_")
+		len := len(splitName)


Suggested change

len := len(splitName)

nameLen := len(splitName)

nit to avoid conflict w/ len() builtin

austince · 2020-08-26T17:15:28Z

app/kumactl/cmd/install/render_helm_files.go

+		for i := 1; i < len-1; i++ {
+			n := splitName[i]
+
+			if _, ok := root[n]; !ok {
+				root[n] = map[string]interface{}{}
+			}
+			root = root[n].(map[string]interface{})
+		}
+		root[splitName[len-1]] = value


I find this map generation a bit hard to follow. Not surehow difficult something like this would be to implement, but would be nicer to have the arg-to-helm-value mapping tagged on the struct fields instead of using reflection.

type InstallControlPlaneArgs struct { controlPlaneImagePullPolicy string `helm:"controlPlane.image.pullPolicy"` // ...

Co-authored-by: Austin Cawley-Edwards <austin.cawley@gmail.com>

…ore/kumactl_with_helm

Co-authored-by: Austin Cawley-Edwards <austin.cawley@gmail.com> Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz · 2020-09-04T10:46:28Z

app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml

@@ -5499,7 +5548,7 @@ webhooks:
          - meshes
    sideEffects: None
  - name: owner-reference.kuma-admission.kuma.io
-    failurePolicy: Fail
+    failurePolicy: Ignore


I think this should stay as Fail. This helps us set owner references to resources. Without this, if we add TrafficPermission to some Mesh and we delete this some Mesh, TrafficPermission won't be deleted

Oh this probably means the Helm charts are wrong.

jakubdyszkiewicz · 2020-09-04T11:03:33Z

app/kumactl/cmd/install/render_helm_files.go

+}
+
+func renderHelmFiles(templates []data.File, args interface{}) ([]data.File, error) {
+	loadedChart, err := loadCharts(templates)


nit: loadedChart, err := loadCharts(templates) -> chart, err := loadChart(templates)

jakubdyszkiewicz · 2020-09-04T11:05:34Z

deployments/charts/kuma/README.md

+## Note to Chart developers
+
+The charts are used internally in `kumactl install`, therefore the following rules apply when developing new chat features:
+ * all templates that start with `pre-` and `post-` are omitted when processing in `kumactl install`  


Maybe add a note that you need to run make generate... to have consistent HELM and kumactl install?

Makes sense, but I think there is still this verification part of make test

go.mod

…helm

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…helm

austince

looks great to me too!

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev force-pushed the chore/kumactl_with_helm branch 2 times, most recently from 82036d4 to 1553876 Compare August 24, 2020 13:34

Nikolay Nikolaev added 12 commits August 24, 2020 16:45

chore(*) make kumactl install control-plane use Helm Charts

871f633

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(helm) extra chars in cp-webhooks-and-secrets

072d586

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(kumactl) tls arguments naming

847a231

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(kumactl) namespace resource for Helm charts

7575517

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(kumactl) generate Values map without the first name

e71236b

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(kumact) make one test pass

2eb554b

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(kumactl) make CNI test pass

f2e867f

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(kumactl) order helm files when serializing

4386948

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(kumactl) install global cp test

772a47a

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(kumactl) fix remote install test

68b852d

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(kumactl) fix override install test

be6c744

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(kumactl) fix completion tests

0b14db1

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev force-pushed the chore/kumactl_with_helm branch from 1553876 to 0b14db1 Compare August 25, 2020 20:12

fix(kumactl) fix --use-node-port

5a54ece

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev marked this pull request as ready for review August 26, 2020 05:51

nickolaev requested a review from a team as a code owner August 26, 2020 05:51

fix(kumactl) strip Helm specific labels

04ee5f7

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev force-pushed the chore/kumactl_with_helm branch from d52ac67 to 04ee5f7 Compare August 26, 2020 08:08

austince suggested changes Aug 26, 2020

View reviewed changes

Nikolay Nikolaev and others added 5 commits August 27, 2020 18:46

Update deployments/charts/kuma/templates/cni-daemonset.yaml

1a0fa19

Co-authored-by: Austin Cawley-Edwards <austin.cawley@gmail.com>

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

e62c282

Merge remote-tracking branch 'origin/chore/kumactl_with_helm' into ch…

81d833c

…ore/kumactl_with_helm

fix(kumactl) review remarks

8e805ff

Co-authored-by: Austin Cawley-Edwards <austin.cawley@gmail.com> Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(kumactl) re-generate from the templates

85c6acf

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz reviewed Sep 4, 2020

View reviewed changes

Nikolay Nikolaev added 2 commits September 14, 2020 12:16

Merge remote-tracking branch 'origin/master' into chore/kumactl_with_…

809df0c

…helm

fix(*) after merge

0dd0c9a

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Nikolay Nikolaev added 2 commits September 14, 2020 16:55

fix(*) review remarks

4194e19

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) get rid of the old templates

ed267b4

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz approved these changes Sep 15, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into chore/kumactl_with_…

d1eac46

…helm

austince approved these changes Sep 15, 2020

View reviewed changes

nickolaev merged commit ef62c04 into master Sep 15, 2020

nickolaev mentioned this pull request Sep 15, 2020

Merge kumactl install ingress in the main kumactl install control-plane #1020

Closed

nickolaev deleted the chore/kumactl_with_helm branch September 17, 2020 05:45

nickolaev mentioned this pull request Sep 17, 2020

fix(*) drop k8s 1.13 support #1026

Merged

1 task

nickolaev pushed a commit that referenced this pull request Oct 7, 2020

chore(*) cleanup cni templates after #989

e071802

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev mentioned this pull request Oct 7, 2020

chore(*) cleanup unneeded CNI templates and the relevant code #1053

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(*) make kumactl install control plane leverage the Helm Charts #989

chore(*) make kumactl install control plane leverage the Helm Charts #989

nickolaev commented Aug 21, 2020

nickolaev commented Aug 26, 2020

austince left a comment

austince Aug 26, 2020

austince Aug 26, 2020

austince Aug 26, 2020

austince Aug 26, 2020

austince Aug 26, 2020

austince Aug 26, 2020

jakubdyszkiewicz Sep 4, 2020

nickolaev Sep 4, 2020

jakubdyszkiewicz Sep 4, 2020

jakubdyszkiewicz Sep 4, 2020

nickolaev Sep 4, 2020

austince left a comment

chore(*) make kumactl install control plane leverage the Helm Charts #989

chore(*) make kumactl install control plane leverage the Helm Charts #989

Conversation

nickolaev commented Aug 21, 2020

Summary

Documentation

nickolaev commented Aug 26, 2020

austince 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

austince left a comment

Choose a reason for hiding this comment