Introduce install stages by siggy · Pull Request #2719 · linkerd/linkerd2

siggy · 2019-04-18T18:24:06Z

This change introduces two named parameters for linkerd install, split
by privilege:

linkerd install config
- Namespace
- ClusterRoles
- ClusterRoleBindings
- CustomResourceDefinition
- ServiceAccounts
linkerd install control-plane
- ConfigMaps
- Secrets
- Deployments
- Services

Comprehensive linkerd install is still supported.

TODO:

linkerd check support
linkerd upgrade support
integration tests

Part of #2337

Signed-off-by: Andrew Seigner [email protected]

l5d-bot · 2019-04-18T18:42:42Z

Integration test results for 507f9f3: success 🎉
Log output: https://gist.github.com/049a9eee8989349fca192f3c19b2ec2b

siggy · 2019-04-18T20:37:31Z

Per #2337 (comment) I'm considering scoping stage 1 strictly to cluster-level resources:

linkerd install config
- Namespace
- ClusterRoles
- ClusterRoleBindings
- CustomResourceDefinition
linkerd install control-plane
- ConfigMaps
- ServiceAccounts
- Secrets
- Deployments
- Services

alpeb

Looks like a good start to me 👍

Per your last comment, if you limit the first stage to just cluster-level resources, you'd increase the chance of getting warning events during the second stage.

Also users might start entering config flags in the first stage, so we'll have to persist them in the main ConfigMap in that stage. This also means we'll have to update that config with flags passed during the second stage. We can't fail in the second stage if there's already a ConfigMap, so most likely we'll have to introduce a new field to distinguish between finished installs and configed-only ones.

I added a couple of comments regarding ordering/dependencies in #2337 (comment).

alpeb · 2019-04-19T14:44:25Z

cli/cmd/install.go

+  linkerd install | kubectl apply -f -
+
+  # Installation may also be broken up into two stages by user privilege.
+  linkerd install config | kubectl apply -f -


How about adding a note that this stage requires cluster-admin privileges?

alpeb · 2019-04-19T14:55:27Z

cli/cmd/install.go

-	proxyInjectorConfigTemplateName = "templates/proxy_injector-config.yaml"
-	proxyInjectorTemplateName       = "templates/proxy_injector.yaml"
-	spValidatorConfigTemplateName   = "templates/sp_validator-config.yaml"
-	spValidatorTemplateName         = "templates/sp_validator.yaml"


Out of curiosity, why did these get the axe?

Since these strings only occur in one place, I prefer to just hard-code them. This way I can look at one place in the file rather than two to get my head around what's happening.

ihcsim

LGTM. I assume the TODOs will happen in other PRs?

As for scoping stage 1 strictly to cluster-level resources, I think it depends if an operator/admin who runs linkerd install config cares about things like secrets and global configs or not, in particular, I am thinking of the --identity-* flags.

olix0r · 2019-04-19T16:55:57Z

My expectation is the linkerd control plane admin will have to care about the issuer credentials; not the cluster admin.

siggy · 2019-04-19T18:06:39Z

@alpeb

Per your last comment, if you limit the first stage to just cluster-level resources, you'd increase the chance of getting warning events during the second stage.

I think this is ok for now (at least not worse than current state). Per #2337 (comment), Kubernetes is designed to handle not everything being available during startup, and I'm increasingly feeling that we should not try to control/solve deployment ordering.

Also users might start entering config flags in the first stage, so we'll have to persist them in the main ConfigMap in that stage. This also means we'll have to update that config with flags passed during the second stage. We can't fail in the second stage if there's already a ConfigMap, so most likely we'll have to introduce a new field to distinguish between finished installs and configed-only ones.

I've rebased the whole PR to make the first stage cluster-level resources only, and also added some code to disable most flags for the first stage.

l5d-bot · 2019-04-19T18:33:04Z

Integration test results for bb9fc00: success 🎉
Log output: https://gist.github.com/3d4dc5bd53b3144a62d9b62b551983d4

l5d-bot · 2019-04-19T18:53:52Z

Integration test results for 7e4ad1f: success 🎉
Log output: https://gist.github.com/66659ed36f7f713c0db9eee5c38efeeb

l5d-bot · 2019-04-19T19:55:08Z

Integration test results for a1d4f9b: success 🎉
Log output: https://gist.github.com/4d5138f9bd7c913e039375eba393d9f5

alpeb · 2019-04-19T20:42:31Z

cli/cmd/install.go

+			if f.Changed {
+				switch f.Name {
+				// TODO: remove "proxy-auto-inject" when it becomes default
+				case "linkerd-namespace", "proxy-auto-inject":


I think we should add to this list ignore-cluster and all the global config flags.

I meant all the "global flags" (--api-addr, etc)

The flags var passed into this function comes from installOptions.recordableFlagSet(), which does not include the "global flags".

$ bin/go-run cli install config --api-addr foo > /dev/null && echo $? 0 $ bin/go-run cli install config --admin-port 123 > /dev/null && echo $? Error: flag not available for config stage: --admin-port Usage:

... that said, i'm realizing that there is also installOnlyFlagSet, none of which should be set during linkerd install config, i have pushed a new commit to reflect this. i don't think linkerd install config --ignore-cluster should be supported, as it has no effect on the output.

linkerd install config --ignore-cluster should be supported, as it has no effect on the output.

linkerd install and linkerd install config both check if linkerd already exists in the cluster, so I was thinking on allowing that flag in both cases for consistency.

oops sorry i mean linkerd install config --ignore-cluster should not be supported (i think you read it the way i intended anyway ;) ) ...

nice catch! i failed to realize that linkerd install config would error out if a cluster already existed, and not providing an --ignore-cluster flag leaves the user completely unable to render that output. i've pushed a new commit to always allow linkerd install config to succeed, regardless of cluster state.

alpeb · 2019-04-19T20:56:09Z

If not through some sort of state field in the global ConfigMap, what's the plan for ensuring linkerd install control-plane gets run after linkerd instal config? I assume listing ClusterRoles and ClusterRoleBindings is not available under simple namespace-level privileges, so we could only maybe check the existence of the linkerd namespace, which would be a very limited check...

siggy · 2019-04-19T21:11:43Z

If not through some sort of state field in the global ConfigMap, what's the plan for ensuring linkerd install control-plane gets run after linkerd instal config? I assume listing ClusterRoles and ClusterRoleBindings is not available under simple namespace-level privileges, so we could only maybe check the existence of the linkerd namespace, which wouldn't be a very limited check...

@alpeb I think we can manage a lot of this via docs. I'd like to ensure users can run these commands in any order, just to see the output. While the behavior of linkerd install and linkerd upgrade are now reliant on k8s state, I'd like to minimize that surface area.

l5d-bot · 2019-04-19T22:15:44Z

Integration test results for 8a8be09: success 🎉
Log output: https://gist.github.com/3964792ad98f219c3a87cfa810c98e47

dadjeibaah · 2019-04-20T00:07:16Z

I kinda think we may need to evaluate the user experience more when a user tries to install the control plane without running linkerd install config. When I run the command, I get a ton of

Error from server (NotFound): error when creating "STDIN": namespaces "linkerd" not found

Which may be a little shocking when running install control-plane. WDYT?

siggy · 2019-04-20T00:36:29Z

Which may be a little shocking when running install control-plane. WDYT?

@dadjeibaah Your comments and @alpeb's are making me think we need to guide the user a bit more via the cli. Maybe we can have linkerd install control-plane return an error if the namespace (and maybe RBAC?) does not exist, and provide a flag to just get the YAML if they really want it:

$ linkerd install control-plane
Error: 'linkerd' namespace does not exist, run 'linkerd install config' first, or 'linkerd install control-plane ignore-cluster' to get the output anyway.

$ linkerd install control-plane --ignore-cluster
---
kind: Service
...

@grampelberg thoughts?

alpeb · 2019-04-22T19:51:30Z

It's not clear which flags are available to each stage. You think it would be too much to have these be totally different commands, like install-config and install-control-plane? Keeping of course the full install command, still with a note explaining you can separate the install using those other two commands.

siggy · 2019-04-22T19:57:20Z

@alpeb good points re: which flags work for which commands. Creating different commands would make that clearer. I think another approach could be to set these up as proper Cobra subcommands of install.

Either way, I'd like to get this baseline functionality merged as-is, to minimize merge conflicts with ongoing install/inject work. I've filed #2729 to track this.

siggy · 2019-04-22T20:21:42Z

@alpeb @dadjeibaah @grampelberg latest commit causes linkerd install control-plane to fail is namespace does not exist:

$ bin/go-run cli install control-plane
Failed to find the Linkerd control-plane namespace. If this expected, use the --ignore-cluster flag.
Error: The "linkerd" namespace does not exist

$ bin/go-run cli install control-plane --ignore-cluster
---
kind: ConfigMap
...

l5d-bot · 2019-04-22T20:42:15Z

Integration test results for 6a9be59: success 🎉
Log output: https://gist.github.com/9a7805c9ae13571dd4c007ec3268c20b

alpeb · 2019-04-22T22:46:29Z

cli/cmd/install.go

+	hc := newHealthChecker(
+		[]healthcheck.CategoryID{healthcheck.KubernetesAPIChecks},
+		time.Time{},
+	)


TIOLI: we might gain some consistency by reusing this same minimalistic healthchecker in exitIfClusterExists. That might need some new hc.CheckConfigMap method to replace the direct call we're making in existIfClusterExists.

Agree, I looked at refactoring this a bit further, then backed it out in the interest of getting this PR through. I captured your comment, and related requirements for healthcheck and client creation via #2735.

grampelberg

I'd like to expand on #2729 a little bit with different help text as well as flags.
@adleong had a good suggestion for a flag that was different than --ignore-cluster, it seemed like a good idea at the time.
Should we move all service accounts into stage1? There's no RBAC but as a cluster admin I'd restrict creation of service accounts on a per-namespace basis.

Mostly just some questions/comments, this feels pretty good as it is. Definitely a step in the right direction!

adleong · 2019-04-23T17:15:29Z

The check that the Linkerd namespace exists before running linkerd install control-plane feels similar to other pre-command checks (e.g. checking that the control plane is reachable before running stat) so I'd call the flag something like --skip-checks. If I understand correctly, --ignore-cluster is specifically for overriding the linkerd config when running install over an existing install.

adleong · 2019-04-23T17:23:53Z

cli/cmd/install.go

+
+	err := hc.CheckNamespace(context.Background(), controlPlaneNamespace, true)
+	if err != nil {
+		fmt.Fprintln(os.Stderr, "Failed to find the Linkerd control-plane namespace. If this expected, use the --ignore-cluster flag.")


Probably our first suggestion should be to run linkerd install config. Maybe something like:

"Failed to find required control-plane namespace: %s. Run linkerd install config -l %s | kubectl apply -f - to create it (this requires cluster administration permissions.) See {website docs link} for more information. Or use {flag} to proceed anyway."

This change introduces two named parameters for `linkerd install`, split by privilege: - `linkerd install config` - Namespace - ClusterRoles - ClusterRoleBindings - CustomResourceDefinition - `linkerd install control-plane` - ConfigMaps - ServiceAccounts - Secrets - Deployments - Services Comprehensive `linkerd install` is still supported. TODO: - `linkerd check` support - `linkerd upgrade` support - integration tests Part of #2337 Signed-off-by: Andrew Seigner <[email protected]>

Signed-off-by: Andrew Seigner <[email protected]>

adleong · 2019-04-23T17:57:14Z

cli/cmd/install.go

+
+	success := hc.RunChecks(exitOnError)
+	if !success {
+		fmt.Fprintln(os.Stderr, "Failed to connect to Kubernetes. If this expected, use the --ignore-cluster flag.")


I think this message needs to be updated as well

Signed-off-by: Andrew Seigner <[email protected]>

l5d-bot · 2019-04-23T18:20:14Z

Integration test results for b2c04f4: success 🎉
Log output: https://gist.github.com/321cf0ef9f631daf15d9a5d39e4ad6b2

adleong

⭐ 🐶 LGTM!

adleong · 2019-04-23T18:31:12Z

cli/cmd/public_api.go

 package cmd

+// TODO: this file should probably move out of `./cli/cmd` into something like
+// `./cli/publicapi` or `./cli/pkg`


maybe create an issue and link to it in this TODO?

filed #2735 yesterday to track, have updated the comment for reference.

Signed-off-by: Andrew Seigner <[email protected]>

alpeb

Looks good to me 👍

l5d-bot · 2019-04-23T21:53:28Z

Integration test results for 2f85d3d: fail 😕
Log output: https://gist.github.com/8a91af95f70fa2b0fcbc5807b0689352

siggy added area/cli area/install labels Apr 18, 2019

siggy self-assigned this Apr 18, 2019

siggy requested review from alpeb and ihcsim April 18, 2019 18:24

alpeb reviewed Apr 19, 2019

View reviewed changes

ihcsim approved these changes Apr 19, 2019

View reviewed changes

siggy force-pushed the siggy/install-stages branch from 507f9f3 to bb9fc00 Compare April 19, 2019 17:56

siggy force-pushed the siggy/install-stages branch from bb9fc00 to 7e4ad1f Compare April 19, 2019 18:38

siggy force-pushed the siggy/install-stages branch from 7e4ad1f to a1d4f9b Compare April 19, 2019 19:40

siggy mentioned this pull request Apr 19, 2019

Introduce multi-stage upgrade #2723

Merged

alpeb reviewed Apr 19, 2019

View reviewed changes

siggy mentioned this pull request Apr 22, 2019

explicit flag support for multi-stage install #2729

Closed

alpeb reviewed Apr 22, 2019

View reviewed changes

siggy mentioned this pull request Apr 22, 2019

reevaluate APIs in cli for healthcheck / k8s client / public api client #2735

Closed

1 task

grampelberg reviewed Apr 23, 2019

View reviewed changes

adleong reviewed Apr 23, 2019

View reviewed changes

siggy added 8 commits April 23, 2019 10:42

review feedback: check all install flags

3eae66b

Signed-off-by: Andrew Seigner <[email protected]>

remove "global flag" linkerd-namespace

515fb19

Signed-off-by: Andrew Seigner <[email protected]>

review feedback: output config when cluster exists

e34249f

Signed-off-by: Andrew Seigner <[email protected]>

fail install control-plane without ns

95906b8

Signed-off-by: Andrew Seigner <[email protected]>

review feedback: --skip-checks, better error msg

95bace9

Signed-off-by: Andrew Seigner <[email protected]>

move service accounts into stage 1

75e4cbe

Signed-off-by: Andrew Seigner <[email protected]>

fixup following rebase

3e7cf8d

Signed-off-by: Andrew Seigner <[email protected]>

siggy force-pushed the siggy/install-stages branch from e2fc912 to 3e7cf8d Compare April 23, 2019 17:49

adleong reviewed Apr 23, 2019

View reviewed changes

fix skip-checks error message

b2c04f4

Signed-off-by: Andrew Seigner <[email protected]>

adleong approved these changes Apr 23, 2019

View reviewed changes

reference github issue in TODO

2f85d3d

Signed-off-by: Andrew Seigner <[email protected]>

alpeb approved these changes Apr 23, 2019

View reviewed changes

siggy merged commit b2b4780 into master Apr 23, 2019

siggy deleted the siggy/install-stages branch April 23, 2019 22:39

grampelberg mentioned this pull request Apr 24, 2019

Focus install by role and stage #2337

Closed

11 tasks

This was referenced Apr 24, 2019

RFC: Break up linkerd install into stages #2164

Closed

Define multi-stage commands as subcommands #2772

Merged

Conversation

siggy commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l5d-bot commented Apr 18, 2019

Uh oh!

siggy commented Apr 18, 2019

Uh oh!

alpeb left a comment • edited by siggy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ihcsim left a comment

Choose a reason for hiding this comment

Uh oh!

olix0r commented Apr 19, 2019

Uh oh!

siggy commented Apr 19, 2019

Uh oh!

l5d-bot commented Apr 19, 2019

Uh oh!

l5d-bot commented Apr 19, 2019

Uh oh!

l5d-bot commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siggy Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpeb commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siggy commented Apr 19, 2019

Uh oh!

l5d-bot commented Apr 19, 2019

Uh oh!

dadjeibaah commented Apr 20, 2019

Uh oh!

siggy commented Apr 20, 2019

Uh oh!

alpeb commented Apr 22, 2019

Uh oh!

siggy commented Apr 22, 2019

Uh oh!

siggy commented Apr 22, 2019

Uh oh!

l5d-bot commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grampelberg left a comment

Choose a reason for hiding this comment

Uh oh!

adleong commented Apr 23, 2019

Uh oh!

adleong Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l5d-bot commented Apr 23, 2019

Uh oh!

adleong left a comment

siggy commented Apr 18, 2019 •

edited

Loading

alpeb left a comment •

edited by siggy

Loading

siggy Apr 19, 2019 •

edited

Loading

alpeb commented Apr 19, 2019 •

edited

Loading

adleong Apr 23, 2019 •

edited

Loading