Add kudo init `--unsafe-self-signed-webhook-ca` option #1459

zen-dog · 2020-04-09T15:08:32Z

Summary:
added a new kudo init ... --unsafe-self-signed-webhook-ca option which can be used when installing KUDO with enabled instance admission webhook (kudo init --webhook InstanceValidation) to avoid the cert-manager dependency. When using this option a certificate signed by a self-signed CA is used by the webhook server. This option is meant to be used for local development, testing, and demos and is not meant to be used in production.

Signed-off-by: Aleksey Dukhovniy alex.dukhovniy@googlemail.com
Co-authored-by: alenkacz avarkockova@mesosphere.com

Summary: added a new `kudo init ... --self-signed-webhook-ca-for-testing-only` option which can be used when installing KUDO with enabled instance admission webhook (`kudo init --webhook InstanceValidation`) to avoid the cert-manager dependency. When using this option a certificate signed by a self-singed CA is used by the webhook server. This option is meant to be used for local development, testing and demos and is **not meant to be used in production.** Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Lets see how that goes Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Not today ¯\_(ツ)_/¯ Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

kensipe · 2020-04-10T00:53:08Z

late in the night for me and there is a lot here... I will review first thing in the morning... initial thoughts is looks nice! I'm questioning the public exposure to a flag that is for dev only. I think I would rather have an ENV var that is picked up for dev and not expose non-production flags and features to production. thoughts @zen-dog ?

ANeumann82 · 2020-04-14T10:00:04Z

I like it, but same thoughts as ken: Maybe we can make this a (rather hidden) configuration in an env var, or maybe in a .kudo/config?

zen-dog · 2020-04-14T12:17:18Z

I don't believe that making it an env var hides it enough for people not to use it in production. The safety aspect is twofold: a) psychological as the parameter name clearly indicates it is not for productions and b) technical as the generated cert is only valid for a week. IMHO an env var does not offer more "safety" than that 🤷

I removed this option from the kudo init help. So now we don't advertise it.

…ion in `init` docs Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82

I'm ok with this as is. I don't think using an ENV-var would be better, although I do think that having this as an entry in a kudo-config-file would be better.

I was reasonably sure we already have a ~/.kudo/config file where some things are stored, but it seems I have imagined that. I'd love to have that, especially for this flag, as well as some other things that might be really nice to have for operator-developers but shouldn't be exposed/active for all KUDO users.

pkg/kudoctl/kudoinit/options.go

ANeumann82 · 2020-04-15T14:12:31Z

pkg/kudoctl/kudoinit/options.go

+	// ServiceName is the controllers service name. Currently hard-coded to "kudo-controller-manager-service"
+	// as we don't support setting it in the CLI
+	ServiceName string
+	// SecretName is used by the manager when the webhooks are activated. Contains certificate and the key files for
+	// the webhook server. Currently hard-coded to "kudo-webhook-server-secret" as we don't support setting it in the CLI
+	SecretName string


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

kensipe

I like it! really good stuff.

Can we please change the flag name?

It would also be great to use thekudoinit.DefaultName in many places... but it's a blocker.

nice job!

kensipe · 2020-04-15T21:47:50Z

pkg/kudoctl/cmd/init.go

@@ -105,6 +108,7 @@ func newInitCmd(fs afero.Fs, out io.Writer) *cobra.Command {
 	f.Int64Var(&i.timeout, "wait-timeout", 300, "Wait timeout to be used")
 	f.StringVar(&i.webhooks, "webhook", "", "List of webhooks to install separated by commas (One of: InstanceValidation)")
 	f.StringVarP(&i.serviceAccount, "service-account", "", "", "Override for the default serviceAccount kudo-manager")
+	f.BoolVar(&i.selfSignedWebhookCA, "self-signed-webhook-ca-for-testing-only", false, "Use self-signed CA bundle (for testing only) for the webhooks")


I'm not a fan of the "testing-only" suffix... do we need to add "-ga" or "-prod" to everything else? I would much rather have the shorter flag name. IF you want to double check, lets add a confirmation requiring a Y/n if flag detected. I would rather just document it, it's purpose and consequences. It is up to users as to when they will use it or not.

I don't mind making it shorter at all, however, there were a number of concerns that users will end up using it in production. Hence the long and descriptive name making it clear that it shouldn't be used in prod.

if you don't mind.. who does? and what are the "number" of concerns... there seems to be only one...

kensipe · 2020-04-15T21:49:16Z

pkg/kudoctl/cmd/init.go

@@ -128,13 +132,16 @@ func (initCmd *initCmd) validate(flags *flag.FlagSet) error {
 	if initCmd.webhooks != "" && initCmd.webhooks != "InstanceValidation" {
 		return errors.New("webhooks can be only empty or contain a single string 'InstanceValidation'. No other webhooks supported")
 	}
+	if initCmd.webhooks == "" && initCmd.selfSignedWebhookCA {
+		return errors.New("self-signed CA bundle can only be used with webhooks option")


this is more of a warning.. right? there is no value of a CA bundle without webhooks... but there is no reason we can't run that way. I'm ok with it... just seems unnecessary

The webhook is populated with the CA bundle so it does need it. Also, most CLIs that I use fail in the presence of an invalid option. I think we should do the same here. It might save the user some surprises.

pkg/kudoctl/kudoinit/manager/manager.go

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

kensipe · 2020-04-16T13:04:27Z

@zen-dog regarding your ENV response

. IMHO an env var does not offer more "safety" than that 🤷
I don't believe there was a suggestion to make a "safer" approach... there isn't any... the feature is either used or not used and when used it is either used in the way we prefer or it is not. The suggestion was to not pollute the CLI UX space. Adding prescriptive words to flags seems like a smell to me... the flag should indicate what it does... not prescribe how or when it is to be used. The use of ENV was suggested because devs are used to using ENV for debug purposes... this is common with go flags... or GLOG_V2 flags to provide verbosity levels even on a production app.

I also like and prefer @ANeumann82 suggestion of having a kudo file... however the only config file we have that I'm aware of is for the client... not for the manager

kensipe · 2020-04-16T13:07:36Z

another review pass... looks like the configuration is at the client... the kudo config sounds like a good idea

zen-dog · 2020-04-16T13:29:42Z

the kudo config sounds like a good idea

I'm not fond of it. One immediate use case that we have is for e2e tests which generate all KUDO manifests by running kudo init. I'd like to keep it a flag (or at the very least an env var) to keep the testing workflow simple.

kensipe

I'm good with team consensus... I have a preference for removing "testing-only" from the flag or moving it out to an ENV or config... but this feature is useful and valuable... good work!

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

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners April 9, 2020 15:08

zen-dog requested a review from ANeumann82 April 9, 2020 15:08

zen-dog added 2 commits April 9, 2020 17:22

Activating IAC with self-signed CA for e2e tests

ca06b0f

Lets see how that goes Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Revert "Activating IAC with self-signed CA for e2e tests"

7ffb894

Not today ¯\_(ツ)_/¯ Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

*Not* advertising the --self-signed-webhook-ca-for-testing-only opt…

ba56380

…ion in `init` docs Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82 approved these changes Apr 15, 2020

View reviewed changes

Minor variable renaming

c15d719

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

kensipe requested changes Apr 15, 2020

View reviewed changes

zen-dog added 2 commits April 16, 2020 14:27

Removed hard-coded values from Options

61b69e6

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

Removed "kudo-controller-manager" magic string from the tests

c739c9f

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

kensipe approved these changes Apr 16, 2020

View reviewed changes

Renamed the option to unsafe-self-signed-webhook-ca

74e08f9

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

zen-dog changed the title ~~Add kudo init --self-signed-webhook-ca-for-testing-only option~~ Add kudo init --unsafe-self-signed-webhook-ca option Apr 16, 2020

zen-dog merged commit e6bae64 into master Apr 16, 2020

zen-dog deleted the ad/self-signed-webhook-ca branch April 16, 2020 16:13

zen-dog mentioned this pull request Apr 17, 2020

Could we get rid of the cert-manager dependency? #1132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kudo init `--unsafe-self-signed-webhook-ca` option #1459

Add kudo init `--unsafe-self-signed-webhook-ca` option #1459

zen-dog commented Apr 9, 2020 •

edited

Loading

kensipe commented Apr 10, 2020

ANeumann82 commented Apr 14, 2020

zen-dog commented Apr 14, 2020 •

edited

Loading

ANeumann82 left a comment

ANeumann82 Apr 15, 2020

kensipe left a comment

kensipe Apr 15, 2020

zen-dog Apr 16, 2020

kensipe Apr 16, 2020

kensipe Apr 15, 2020

zen-dog Apr 16, 2020

kensipe commented Apr 16, 2020

kensipe commented Apr 16, 2020

zen-dog commented Apr 16, 2020

kensipe left a comment

Add kudo init --unsafe-self-signed-webhook-ca option #1459

Add kudo init --unsafe-self-signed-webhook-ca option #1459

Conversation

zen-dog commented Apr 9, 2020 • edited Loading

kensipe commented Apr 10, 2020

ANeumann82 commented Apr 14, 2020

zen-dog commented Apr 14, 2020 • edited Loading

ANeumann82 left a comment

Choose a reason for hiding this comment

ANeumann82 Apr 15, 2020

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

kensipe Apr 15, 2020

Choose a reason for hiding this comment

zen-dog Apr 16, 2020

Choose a reason for hiding this comment

kensipe Apr 16, 2020

Choose a reason for hiding this comment

kensipe Apr 15, 2020

Choose a reason for hiding this comment

zen-dog Apr 16, 2020

Choose a reason for hiding this comment

kensipe commented Apr 16, 2020

kensipe commented Apr 16, 2020

zen-dog commented Apr 16, 2020

kensipe left a comment

Choose a reason for hiding this comment

Add kudo init `--unsafe-self-signed-webhook-ca` option #1459

Add kudo init `--unsafe-self-signed-webhook-ca` option #1459

zen-dog commented Apr 9, 2020 •

edited

Loading

zen-dog commented Apr 14, 2020 •

edited

Loading