Removed `--webhook` option #1497

zen-dog · 2020-05-07T20:35:52Z

Summary:
Now that KUDO moves towards a better support of multiple plans (e.g. kudoctl plan trigger command), the existing instance admission webhook becomes necessary to guarantee the plan execution consistency. More on KUDO admission controller in the documentation.

This PR removes the --webhook option and thus makes the instance admission webhook required. This is a breaking change since the users will have to either install cert-manager installed or use the --unsafe-self-signed-webhook-ca option when initializing KUDO. For existing installations, one would need to run kudo init to create the missing secret/webhook configuration.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com

Summary: Now that KUDO moves towards a better support of multiple plans (e.g. `kudoctl plan trigger` command), the existing instance admission webhook becomes necessary to guarantee the plan execution consistency. More on KUDO [admission controller](https://kudo.dev/docs/developing-operators/plans.html#admission-controllers) in the documentation. This PR removes the `--webhook` option and thus makes the instance admission webhook required. This is a breaking change since the users will have to either have [cert-manager](https://cert-manager.io/) installed or use the `--unsafe-self-signed-webhook-ca` option when initializing KUDO. For existing installations one would need to run [kudo init](https://kudo.dev/docs/cli.html#examples) to create missing secret/webhook configuration. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog · 2020-05-07T20:37:07Z

pkg/controller/instance/instance_controller.go

-	}
-
-	// ---------- 3. Check if we should start execution of new plan ----------
+	// ---------- 2. Check if we should start execution of new plan ----------


Webhook sets the finalizer on creation

zen-dog · 2020-05-07T20:38:09Z

pkg/controller/instance/instance_controller.go

@@ -214,9 +202,9 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", convert.StringValue(newExecutionPlan)))
 	}

-	// ---------- 4. If there's currently active plan, continue with the execution ----------
+	// ---------- 3. If there's currently active plan, continue with the execution ----------


I didn't want to complicate this PR but steps 2. and 3. can be merged now as the logic becomes considerably simpler. I'll do this in a followup.

zen-dog · 2020-05-07T20:39:06Z

pkg/controller/instance/instance_controller_test.go

-	close(stopMgr)
-	mgrStopped.Wait()
-
-	log.Print("And update the instance parameter value")


This test is not needed anymore as the webhook will decline Instance updates when the manager is down.

zen-dog · 2020-05-07T20:40:21Z

test/cert/ca.crt

@@ -0,0 +1,19 @@
+-----BEGIN CERTIFICATE-----


Below the server certificates (and the bundle just in case) used for integration tests

zen-dog · 2020-05-07T20:41:21Z

test/integration/immutable-client-ip/00-install-instance.yaml

@@ -48,3 +37,15 @@ spec:
        selector:
          instance: {{ .Name }}
          label: {{ .Params.LABEL }}
+


Fun fact: the order matters: Instance has to be applied after the OperatorVerson of the webhook will decline the CREATE request.

zen-dog · 2020-05-07T20:42:41Z

test/integration/racy-reconcile/00-install-instance.yaml

@@ -1,10 +0,0 @@
-apiVersion: kudo.dev/v1beta1


This test checks that you can install Instance before the OperatorVersion. This doesn't work anymore

test/integration/update-command/00-install.yaml

zen-dog · 2020-05-07T20:45:21Z

test/integration/update-command/03-update-param-value.yaml

-apiVersion: kudo.dev/v1beta1
-kind: TestStep
-commands:
-  - command: kubectl kudo update --instance update-command-instance -p "param2=othervalue"


This step would make the same (updating some param) as the previous one (02). I removed it.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog · 2020-05-07T20:52:22Z

Ignore failing integration tests. This kuttl/#77 PR will make it green again.

pkg/kudoctl/cmd/init_test.go

nfnt

This is looking great and removing quite a lot of "if"-s, awesome! Have some minor comments and nitpicks though.

cmd/manager/main.go

pkg/controller/instance/instance_controller.go

pkg/controller/instance/instance_controller_test.go

pkg/kudoctl/cmd/init_test.go

pkg/kudoctl/util/kudo/upgrade.go

test/integration/update-command/00-install.yaml

ANeumann82

lgtm, apart from the things @nfnt mentioned

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

cmd/manager/main.go

nfnt

Looking good! 🚢

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

This reverts commit 3f1d82d. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Summary: With [kudo/#1497](kudobuilder/kudo#1497) KUDO is making the instance admission webhook required. This PR adds the necessary manifests and certificate files to the tests. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog · 2020-05-11T18:55:57Z

Ok, so the reason e2e operators tests are failing is that we run them with a local ./bin/manager and not with the manager-as-pod like the other e2e tests. We need to change that before the e2e tests get green.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

as they are generated by `kudo init` or not needed anymore Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

hack/run-e2e-tests.sh

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog added 4 commits April 27, 2020 14:33

Merge branch 'master' into ad/required-webhook

18830cf

Merge branch 'master' into ad/required-webhook

9fa17e3

Making integration tests green

8c3ac8b

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog added the release/breaking-change This PR contains breaking changes and is marked in the release notes label May 7, 2020

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners May 7, 2020 20:35

zen-dog requested a review from ANeumann82 May 7, 2020 20:36

zen-dog commented May 7, 2020

View reviewed changes

test/integration/update-command/00-install.yaml Show resolved Hide resolved

zen-dog commented May 7, 2020

View reviewed changes

Making linter happy

646b0ad

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82 reviewed May 8, 2020

View reviewed changes

pkg/kudoctl/cmd/init_test.go Outdated Show resolved Hide resolved

nfnt requested changes May 8, 2020

View reviewed changes

ANeumann82 approved these changes May 8, 2020

View reviewed changes

zen-dog added 6 commits May 8, 2020 15:09

Temporary enabling skipDelete: true option for e2e tests

f7aed1b

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Removed a debugging remnant

9f3f81b

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Making KUDO_CERT_DIR default x-platform

adbddac

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Space at the end of kudo-e2e-test.yaml

fe0d307

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Review feedback part one

8d05566

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Fixed default KUDO_CERT_DIR path

374db1f

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

nfnt reviewed May 8, 2020

View reviewed changes

cmd/manager/main.go Show resolved Hide resolved

nfnt approved these changes May 8, 2020

View reviewed changes

zen-dog added 3 commits May 8, 2020 18:39

Merge branch 'master' into ad/required-webhook

e7bfb4b

Removed skipDelete: true e2e option

3f1d82d

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Revert "Removed skipDelete: true e2e option"

8f11643

This reverts commit 3f1d82d. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog force-pushed the ad/required-webhook branch 2 times, most recently from 9bdb541 to 8f11643 Compare May 8, 2020 20:18

zen-dog added 6 commits May 11, 2020 14:11

Temporary switch to patched kuttl version

2fe4ee2

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Trying to get e2e operator tests logs

f05df7c

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

and proper reports path

fb081b2

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

and moving logs one level up

b33fd6d

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

and with setting the proper --artifacts-dir /tmp/kudo-e2e-test

7d9641d

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

and with skipDelete: true for operator tests

7362dd9

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog mentioned this pull request May 11, 2020

Instance admission controller manifests kudobuilder/operators#259

Merged

run operators e2e tests agains operators/dev branch

206d7e9

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog added 5 commits May 18, 2020 18:37

Merge branch 'master' into ad/required-webhook

f91b405

Fixed TestIntegInitForCRDs

58d99e8

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Removed crdDir and manifestDirs from the e2e test configuration

4123776

as they are generated by `kudo init` or not needed anymore Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Running operators tests with the new kudo-test.yaml.tmpl

6b1ffd0

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

and with proper sed-ing

03e45b7

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

nfnt reviewed May 19, 2020

View reviewed changes

hack/run-e2e-tests.sh Outdated Show resolved Hide resolved

nfnt reviewed May 19, 2020

View reviewed changes

hack/run-e2e-tests.sh Outdated Show resolved Hide resolved

going back to operators master branch

14dca5f

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog merged commit a7b98bf into master May 19, 2020

zen-dog deleted the ad/required-webhook branch May 19, 2020 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed `--webhook` option #1497

Removed `--webhook` option #1497

zen-dog commented May 7, 2020

zen-dog May 7, 2020

zen-dog May 7, 2020

zen-dog May 7, 2020

zen-dog May 7, 2020 •

edited

Loading

zen-dog May 7, 2020

zen-dog May 7, 2020

zen-dog May 7, 2020 •

edited

Loading

zen-dog commented May 7, 2020 •

edited

Loading

nfnt left a comment

ANeumann82 left a comment

nfnt left a comment

zen-dog commented May 11, 2020

Removed --webhook option #1497

Removed --webhook option #1497

Conversation

zen-dog commented May 7, 2020

zen-dog May 7, 2020

Choose a reason for hiding this comment

zen-dog May 7, 2020

Choose a reason for hiding this comment

zen-dog May 7, 2020

Choose a reason for hiding this comment

zen-dog May 7, 2020 • edited Loading

Choose a reason for hiding this comment

zen-dog May 7, 2020

Choose a reason for hiding this comment

zen-dog May 7, 2020

Choose a reason for hiding this comment

zen-dog May 7, 2020 • edited Loading

Choose a reason for hiding this comment

zen-dog commented May 7, 2020 • edited Loading

nfnt left a comment

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

nfnt left a comment

Choose a reason for hiding this comment

zen-dog commented May 11, 2020

Removed `--webhook` option #1497

Removed `--webhook` option #1497

zen-dog May 7, 2020 •

edited

Loading

zen-dog May 7, 2020 •

edited

Loading

zen-dog commented May 7, 2020 •

edited

Loading