Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce upgrade --from-manifests flag #2697

Merged
merged 3 commits into from
Apr 17, 2019
Merged

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
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]>
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

awesome, that works 👍 thanks! 🎉

@olix0r olix0r self-requested a review April 15, 2019 15:24
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.


func newUpgradeOptionsWithDefaults() *upgradeOptions {
return &upgradeOptions{newInstallOptionsWithDefaults()}
return &upgradeOptions{
"",
Copy link
Member

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

@olix0r because they show up in the manifest?

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.

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

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

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

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

@siggy
Copy link
Member Author

siggy commented Apr 18, 2019

@grampelberg filed linkerd/website#275

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: load upgrade configuration from kubernetes manifest
7 participants