Verify that cert manager is installed #1186

ANeumann82 · 2019-12-16T14:05:10Z

What this PR does / why we need it:

Verify that cert manager is installed before installing with web hook
Add code to allow pre-checks before starting any installation

Fixes #1130

ANeumann82 · 2019-12-16T14:13:14Z

Draft because still missing some more tests

kensipe

commits not signed off

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed CRD checks for cert-manager Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-01-21T11:57:49Z

@kensipe Could you please review again?

gerred

@kensipe Will you take a deep pass here? There's a few things that I think we should talk about in line with standard code review comments / Brian Ketelsen (as Ashley MacNamara ;)) things here like "New". I think where you're going is good @ANeumann82 for validating the cert manager and adding preflight checks, but we just all need to get on the same page for styling.

Additionally, instead of having Description, I recommend embedding the Stringer interface. You can replace Description() string with Stringer and embed the interface and get a lot more power.

pkg/kudoctl/kudoinit/types.go

pkg/kudoctl/kudoinit/prereq/webhook.go

kensipe

Description should NOT be used for something that is well defined in Go . (which is the Stringer interface)
in crds.go, there is a NewInitializer func and an Initializer struct with associated methods... I'm not convinced we should have these as outlined in inlined comments.. however if we are to continue on this path it seems more go like to have a package named initializer with a func that is New() resulting in calling code calling initializer.New() to communicate the full message. This also encapsulates the init struct behavior.

There seems to be a lot of code that is written in another style (perhaps Java). with lots of structure. Perhaps we could pair on this?

kensipe · 2020-01-22T19:31:56Z

pkg/kudoctl/kudoinit/crd/crds.go

@@ -36,6 +36,10 @@ func NewInitializer() Initializer {
 	}
 }

+func (c Initializer) Description() string {


totally agree this should be Stringer interface...

I'm not exactly sure how we got to the "Initializerstruct in the first place. TheInitStepalso feels completely off. An interface would be something you would use more than once and would be reduced in size. If this was aStepI might be able to see it... but anInitStep`. How many and how many kinds of InitSteps are expected?

pkg/kudoctl/kudoinit/result.go

pkg/kudoctl/kudoinit/types.go

kensipe · 2020-01-22T20:15:47Z

I would encourage adding in the needed "Verify that cert manager is installed before installing with web hook" without a complete debatable restructuring of all things init.

ANeumann82 · 2020-01-22T20:41:15Z

Are you generally opposed to refactoring the init code, or just in the context of adding this feature?

Some thoughts on the structure (which I hope is generally not a bad thing, it is a bit much though, I agree on that)

There are multiple init steps:
- CRDs
- PreReqNamespace
- PreReqServiceAccount & RoleBinding
- PreReqWebHook
- Manager

Each of these Steps should be able to:

Verify that it is correctly installed ( or is outdated, looking at the KUDO upgrade)
Check that it can be installed (i.e. webhook needs cert-manager)
- Some of the checks here are only warnings, as we can't fully validate the prerequisite, for example the cert-manager may run in a different namespace.
Install (or upgrade from the existing state)
Output/Return the installed resources as YAML

The Initializer should actually be the only interface there. The InitStep inside the prereqs is kind of duplicated and does the same thing, and should be flattened into the list from above.

With regards to the Result: I totally agree that copying the code is a bad thing(TM), although I have no idea to make it reusable without generics. But i'm very open to have a different pattern here.

Stringer interface makes a lot of sense, didn't know about that one.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Refactored Step interface Renamed Step interface methods Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

More renaming Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-01-30T13:31:43Z

Reworked this quite a bit:

I decided to stay with the Result return type, as I do want to verify the installation, and want warnings for the preInstall verification as well
The actual Installation returns simple golang errors
Moved Result into pkg/kudoctl/verify and removed the duplicate code
Renamed InitStep to Step
Split up the Step interface into multiple smaller ones and used composition. Was a bit unsure about this, but it does make the different responsibilities more clear

There is one more refactoring that I would like to make, in the kudoinit/prereqs package, but I'll leave that for another PR.

gerred

Looks mostly good to me now. The verify errors code could be made a lot more idiomatic, but I think would potentially look like a larger restructuring. I'm 👍 on this now and maybe we shape it up over the next few passes over this code.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 · 2020-02-13T13:10:35Z

I've just tried to rebasing this to fix the DCO things in the early commits. It did... not go well. So for this PR i'd probably manually override the DCO check

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed initStep order Use new strategy for AsYamlManifests as well Only run webhook prechecks if webhooks are actually enabled Move pre install checks into correct funcs, now that they exist Changed PreInstallCheck to use Result with Errors and Warnings Added unit tests for prerequisite validation Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Added unit tests for prerequisite validation Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Added check for running cert-manager deployment with warning Fixed CRD checks for cert-manager Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Replaced "Description" with Stringer interface Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Use health.isHealthy to check cert-manager deployment Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Renamed and organized interface Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Unified Result into pkg/kudoctl/verify Refactored Step interface Renamed Step interface methods Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Cleanup, removed unused code More renaming Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Adjusted use Result by new Verifiers Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Extracted some constants Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

kensipe · 2020-02-27T18:59:00Z

the simple solution to the DCO issue is to squash the commits... they get squash anyway on merge (by our convention), so if we are ready to merge it makes sense.
I rebased to latest master with a squash on all commits. Now to review.

kensipe

I'm just against the verify.Result approach... it completely makes sense for verification processes where a report of warnings and errors is provided to an end user... it does not make sense for standard error flow (of an action like init)
If allowed it would cause confusion as to when to use standard go error handling vs this approach through the application.

…kudobuilder/kudo into an/verify-cert-manager-installation

ANeumann82 · 2020-02-27T19:44:08Z

Thanks for the squash and the comment. I do get the issue you have with that, and I hoped that the current state does take care of it:

The verify.Result is only used in the InstallVerify interface: This will contain methods that will only do verification and will not do any modifications.
For the actual Init action, there is a simple error result, as I do agree that the verify.Result shouldn't be used for the standard error flow.

There is just the thing that the init process should have - in my opionion - a verification step, that provides a list of warnings and errors before doing any potentially irreversable actions on the cluster. And I feel for this verification step the verify.Result is a good fit.

Can we agree on something like "verify.Result should only be used in non-modifying methods"?

If you have any constructive ideas how to handle this, please let me know. I can write some ideas that I have:

Get rid of the verify.Result in the PreInstallVerify and return an error that contains either warnings or errors wrapped in a golang error type. Might be an option, but a custom error would get very close to the existing verify.Result`
Don't return warnings, only return the first error: Could work too, but limits us severly, in my opinion.
Separate the verification and the actual installation/upgrade more (than just a different method/interface): Would be quite hard, as a lot of the same code is used by both, but might be possible.
...

removed error interface from Result Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

kensipe

/lgtm

thanks for the pair coding review! I like the changes we made and you convinced me that the results are useful

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

* Added check that cert-manager is installed if webhooks are enabled * Refactoring for the init process Signed-off-by: Andreas Neumann <aneumann@mesosphere.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

ANeumann82 added 4 commits December 16, 2019 14:47

Added check that cert-manager is installed if webhooks are enabled

395fb58

Fixed initStep order

4395614

Use new strategy for AsYamlManifests as well

80a93d1

Only run webhook prechecks if webhooks are actually enabled

5ed29fd

ANeumann82 added 2 commits December 16, 2019 15:24

Move pre install checks into correct funcs, now that they exist

27695ae

Changed PreInstallCheck to use Result with Errors and Warnings

450b4c2

kensipe requested changes Dec 21, 2019

View reviewed changes

ANeumann82 added 2 commits January 6, 2020 11:20

Merge branch 'master' into an/verify-cert-manager-installation

15933e1

Added unit tests for prerequisite validation

b30d3ef

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

gerred added the needs review label Jan 6, 2020

gerred added this to the 0.10.0 milestone Jan 6, 2020

ANeumann82 added 2 commits January 7, 2020 11:08

Added unit tests for prerequisite validation

5a5815d

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Added check for running cert-manager deployment with warning

df88a05

Fixed CRD checks for cert-manager Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 marked this pull request as ready for review January 7, 2020 12:03

ANeumann82 requested review from alenkacz, gerred, nfnt and zen-dog as code owners January 7, 2020 12:03

ANeumann82 requested a review from kensipe January 8, 2020 14:40

ANeumann82 removed this from the 0.10.0 milestone Jan 15, 2020

ANeumann82 added this to Ready For Review in KUDO Global Jan 20, 2020

gerred requested changes Jan 21, 2020

View reviewed changes

pkg/kudoctl/kudoinit/types.go Outdated Show resolved Hide resolved

alenkacz reviewed Jan 22, 2020

View reviewed changes

pkg/kudoctl/kudoinit/prereq/webhook.go Outdated Show resolved Hide resolved

kensipe requested changes Jan 22, 2020

View reviewed changes

kensipe reviewed Jan 22, 2020

View reviewed changes

pkg/kudoctl/kudoinit/result.go Outdated Show resolved Hide resolved

kensipe reviewed Jan 22, 2020

View reviewed changes

pkg/kudoctl/kudoinit/types.go Outdated Show resolved Hide resolved

Merge branch 'master' into an/verify-cert-manager-installation

da88109

kensipe self-requested a review January 28, 2020 21:25

ANeumann82 added 4 commits January 29, 2020 11:08

Merge branch 'master' into an/verify-cert-manager-installation

82e2ef9

Renamed and organized interface

c6e0146

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Unified Result into pkg/kudoctl/verify

34199ed

Refactored Step interface Renamed Step interface methods Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Cleanup, removed unused code

c1ef733

More renaming Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested a review from gerred January 30, 2020 13:26

ANeumann82 requested a review from alenkacz January 30, 2020 13:32

gerred approved these changes Feb 11, 2020

View reviewed changes

ANeumann82 added 2 commits February 13, 2020 12:53

Merge branch 'master' into an/verify-cert-manager-installation

679c362

Adjusted use Result by new Verifiers

0d39b75

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 force-pushed the an/verify-cert-manager-installation branch from 0d39b75 to 2a79237 Compare February 13, 2020 12:44

ANeumann82 requested a review from jbarrick-mesosphere as a code owner February 13, 2020 12:44

ANeumann82 force-pushed the an/verify-cert-manager-installation branch from 2a79237 to 0d39b75 Compare February 13, 2020 13:06

ANeumann82 added 2 commits February 13, 2020 16:10

Extracted some constants

e4111e8

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

kensipe force-pushed the an/verify-cert-manager-installation branch from e4111e8 to cca66ea Compare February 27, 2020 18:57

kensipe requested changes Feb 27, 2020

View reviewed changes

Merge branch 'an/verify-cert-manager-installation' of mesosphere.com:…

ac5de55

…kudobuilder/kudo into an/verify-cert-manager-installation

Moved packages.verifier to packages to remove stuttering

8d495cc

removed error interface from Result Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

kensipe approved these changes Feb 27, 2020

View reviewed changes

Fixed integration tests

704f97d

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 merged commit 0a0c883 into master Feb 28, 2020

KUDO Global automation moved this from Ready For Review to Done Feb 28, 2020

ANeumann82 deleted the an/verify-cert-manager-installation branch February 28, 2020 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify that cert manager is installed #1186

Verify that cert manager is installed #1186

ANeumann82 commented Dec 16, 2019

ANeumann82 commented Dec 16, 2019

kensipe left a comment

ANeumann82 commented Jan 21, 2020

gerred left a comment

kensipe left a comment

kensipe Jan 22, 2020

kensipe commented Jan 22, 2020

ANeumann82 commented Jan 22, 2020

ANeumann82 commented Jan 30, 2020

gerred left a comment

ANeumann82 commented Feb 13, 2020

kensipe commented Feb 27, 2020

kensipe left a comment

ANeumann82 commented Feb 27, 2020

kensipe left a comment

Verify that cert manager is installed #1186

Verify that cert manager is installed #1186

Conversation

ANeumann82 commented Dec 16, 2019

ANeumann82 commented Dec 16, 2019

kensipe left a comment

Choose a reason for hiding this comment

ANeumann82 commented Jan 21, 2020

gerred left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Jan 22, 2020

Choose a reason for hiding this comment

kensipe commented Jan 22, 2020

ANeumann82 commented Jan 22, 2020

ANeumann82 commented Jan 30, 2020

gerred left a comment

Choose a reason for hiding this comment

ANeumann82 commented Feb 13, 2020

kensipe commented Feb 27, 2020

kensipe left a comment

Choose a reason for hiding this comment

ANeumann82 commented Feb 27, 2020

kensipe left a comment

Choose a reason for hiding this comment