Skip to content

Introduce upgrade --from-manifests flag#2697

Merged
siggy merged 3 commits intomasterfrom
siggy/upgrade-manifest
Apr 17, 2019
Merged

Introduce upgrade --from-manifests flag#2697
siggy merged 3 commits intomasterfrom
siggy/upgrade-manifest

Conversation

@siggy
Copy link
Member

@siggy siggy commented Apr 15, 2019

The linkerd upgrade command read the control-plane's config from
Kubernetes, which required the environment to be configured to connect
to the appropriate k8s cluster.

Introduce a linkerd upgrade --from-manifests flag, allowing the user
to feed the output of linkerd install into the upgrade command.

Fixes #2629

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

Relates to #2637.

Notes

Note that the first commit, 5fb1c0c, contains all the code changes. The second commit simply renames test_helper.go to fake.go.

Examples

Install flow

linkerd install > install_manifests.yaml
kubectl apply -f install_manifests.yaml
linkerd upgrade --from-manifests install_manifests.yaml

Validate linkerd upgrade command works the same with and without --from-manifests

linkerd install | kubectl apply -f -

diff <(
  bin/linkerd upgrade
) <(
  kubectl -n linkerd get cm/linkerd-config secrets/linkerd-identity-issuer -oyaml |
    bin/linkerd upgrade --from-manifests -
)

siggy added 2 commits April 14, 2019 21:39
@siggy
Introduce upgrade --from-manifests flag
5fb1c0c 
The `linkerd upgrade` command read the control-plane's config from
Kubernetes, which required the environment to be configured to connect
to the appropriate k8s cluster.

Intrdouce a `linkerd upgrade --from-manifests` flag, allowing the user
to feed the output of `linkerd install` into the upgrade command.

Fixes #2629

Signed-off-by: Andrew Seigner <[email protected]>
@siggy
rename test_helper.go to fake.go
2dc3488 
Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy added the area/cli label Apr 15, 2019
@siggy siggy self-assigned this Apr 15, 2019
@siggy
Copy link
Member Author

siggy commented Apr 15, 2019

@jon-walton if you have a moment, please take this out for a spin, thanks!

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 15, 2019

Integration test results for 2dc3488: success 🎉
Log output: https://gist.github.com/2668e8b197c6a5e1a952b1face3d17f6

@jon-walton
Copy link
Contributor

jon-walton commented Apr 15, 2019

awesome, that works 👍 thanks! 🎉

@olix0r olix0r self-requested a review April 15, 2019 15:24
olix0r
olix0r approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clean change!

One question for @grampelberg: if we're supporting a file-based workflow, we probably need to update documentation to address the storage of the issuer credentials.

cli/cmd/upgrade.go Outdated
func newUpgradeOptionsWithDefaults() *upgradeOptions {
return &upgradeOptions{newInstallOptionsWithDefaults()}
return &upgradeOptions{
"",
Copy link
Member

@olix0r olix0r Apr 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tioli: named params might be easier to read here

@grampelberg
Copy link
Contributor

grampelberg commented Apr 15, 2019

@olix0r because they show up in the manifest?

alpeb
alpeb approved these changes Apr 15, 2019
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very elegant approach, looks great to me! 👍

@ihcsim
Copy link
Contributor

ihcsim commented Apr 15, 2019

Just wanna confirm that we are ok with writing the issuer's secret to disk.

@siggy
Copy link
Member Author

siggy commented Apr 15, 2019

@ihcsim Re: writing secrets to disk, I think that's a good question for @grampelberg, and if the answer is "yes", maybe deserves treatment in our docs.

@siggy
review feedback: named params
3ee494a 
Signed-off-by: Andrew Seigner <[email protected]>
@olix0r
Copy link
Member

olix0r commented Apr 15, 2019

@grampelberg right. Ideally, we'd encrypt that part of the manifest and have --from-manifest decrypt the secrets

@ihcsim
Copy link
Contributor

ihcsim commented Apr 15, 2019

fwiw, I think all #2629 cares about is the linkerd-config config map.

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 15, 2019

Integration test results for 3ee494a: success 🎉
Log output: https://gist.github.com/362877c549463cb48f27ac6752d32be6

@alpeb
Copy link
Member

alpeb commented Apr 15, 2019

Another possibility would be to add a note in the Upgrade CLI docs, telling people they can store all the resources in disk, but discouraging keeping the Secret one. Then we could have fetchIdentityValues() not fail when the Secret is not found when using --from-manifests. In my brief testing this doesn't break anything.
One downside I can see is when trying to do kubectl apply --prune with resource files in disk that don't match what there is in the cluster, one has to be careful not to delete the Secret in the cluster.

@jon-walton
Copy link
Contributor

jon-walton commented Apr 16, 2019

This is an interesting point.

However, when using a gitops workflow, a linkerd install --flags > install.yaml would end up in git and deployed by the deployment controller (in my case, ArgoCD). if the secret is kept out of the manifest, we'll need linkerd to create the secret and store it in a k8s secret object at runtime

@grampelberg
Copy link
Contributor

grampelberg commented Apr 16, 2019

Is there another option to writing the secret to disk? It'd need to live somewhere ...

From a docs perspective, it is pretty easy to suggest piping through gpg or the like before committing the file.

@ihcsim
Copy link
Contributor

ihcsim commented Apr 16, 2019

if the secret is kept out of the manifest, we'll need linkerd to create the secret and store it in a k8s secret object at runtime

The (identity) secret is created during installation, and remained untouched, in the cluster, during an upgrade.

@siggy
Copy link
Member Author

siggy commented Apr 17, 2019

Going to merge this as-is. I agree storing secrets in the clear on disk is not great, but also agree we can guide users via docs on best practices, such as @grampelberg's gpg approach.

@siggy siggy merged commit 8323e10 into master Apr 17, 2019
@siggy siggy deleted the siggy/upgrade-manifest branch April 17, 2019 20:32
@grampelberg
Copy link
Contributor

grampelberg commented Apr 18, 2019

Can we open an issue on the website repo to track writing up a blurb about managing the manifest?

@siggy siggy mentioned this pull request Apr 18, 2019
Closed
@siggy
Copy link
Member Author

siggy commented Apr 18, 2019

@grampelberg filed linkerd/website#275

@siggy siggy mentioned this pull request Apr 25, 2019
Merged
@ihcsim ihcsim mentioned this pull request Jun 27, 2019
Closed
@siggy siggy mentioned this pull request Oct 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@olix0r olix0r olix0r approved these changes

@alpeb alpeb alpeb approved these changes

Assignees

@siggy siggy

Labels

area/cli

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RFC: load upgrade configuration from kubernetes manifest

7 participants

@siggy @l5d-bot @jon-walton @grampelberg @ihcsim @olix0r @alpeb