Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Fix the render command to not wipe ca.pem #1014

Merged
merged 1 commit into from
Nov 22, 2017

Conversation

mumoshu
Copy link
Contributor

@mumoshu mumoshu commented Nov 20, 2017

Fixes #1003

@c-knowles Would this work for you?

@mumoshu mumoshu added this to the v0.9.9-rc.3 milestone Nov 20, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 20, 2017
@mumoshu
Copy link
Contributor Author

mumoshu commented Nov 20, 2017

Squashed a fix for an easy mistake. Please reload to see the latest commit id if anyone has already seen it.

@mumoshu mumoshu force-pushed the do-not-wipe-ca-pem branch 2 times, most recently from f54e61a to b2ad675 Compare November 20, 2017 13:36
Copy link
Contributor

@cknowles cknowles left a comment

Choose a reason for hiding this comment

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

I think this probably works. I'm not so clear on what we're generating now as it's significantly changed but we don't have much documentation on it, could we add some?

name string
data []byte
overwrite bool
ifEmptySymlinkTo string
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean if populated, create a symlink?

Copy link
Contributor Author

@mumoshu mumoshu Nov 20, 2017

Choose a reason for hiding this comment

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

Yes - If we had nothing to write, default to create a symlink to denoted by the ifEmptySymlinkTo.
Basically, we have nothing to write for worker-ca.pem as long as the user explicitly overwrite or put the file before running kube-aws render credentials. So in that case, create a symlink to ca.pem(=the same as the previous behavior).

@mumoshu
Copy link
Contributor Author

mumoshu commented Nov 20, 2017

Squashed again. Seems to work so far.

@codecov-io
Copy link

codecov-io commented Nov 20, 2017

Codecov Report

Merging #1014 into master will decrease coverage by 0.02%.
The diff coverage is 61.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1014      +/-   ##
==========================================
- Coverage   34.88%   34.86%   -0.03%     
==========================================
  Files          59       59              
  Lines        4133     4156      +23     
==========================================
+ Hits         1442     1449       +7     
- Misses       2532     2545      +13     
- Partials      159      162       +3
Impacted Files Coverage Δ
core/controlplane/config/credential.go 51.26% <14.28%> (-2.73%) ⬇️
core/controlplane/config/encrypted_assets.go 67.22% <65.75%> (-0.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a0a4dc...76e1341. Read the comment docs.

@mumoshu
Copy link
Contributor Author

mumoshu commented Nov 21, 2017

I've manually verified this to work by running kube-aws render to regenerate credentials and then updating my cluster with kube-aws update.
The cfn stack update was successful while kube-dashboard went into CrashLookBackOff probably due to the rotation of the service account signing key.

$ k logs kubernetes-dashboard-7f46664f8f-n7gm4
2017/11/21 13:50:43 Starting overwatch
2017/11/21 13:50:43 Using in-cluster config to connect to apiserver
2017/11/21 13:50:43 Using service account token for csrf signing
2017/11/21 13:50:43 No request provided. Skipping authorization
2017/11/21 13:50:43 Error while initializing connection to Kubernetes apiserver. This most likely means that the cluster is misconfigured (e.g., it has invalid apiserver certificates or service accounts configuration) or the --apiserver-host param points to a server that does not exist. Reason: the server has asked for the client to provide credentials
Refer to the troubleshooting guide for more information: https://github.com/kubernetes/dashboard/blob/master/docs/user-guide/troubleshooting.md

However, this issue is not yet resolved in kube-aws - see #1013 for more info.

@mumoshu
Copy link
Contributor Author

mumoshu commented Nov 21, 2017

I'll merge this once someone put LGTM on it

@mumoshu
Copy link
Contributor Author

mumoshu commented Nov 22, 2017

/lgtm 😃

@mumoshu mumoshu merged commit 4ad6fa4 into kubernetes-retired:master Nov 22, 2017
@mumoshu mumoshu deleted the do-not-wipe-ca-pem branch November 22, 2017 01:40
kylehodgetts pushed a commit to HotelsDotCom/kube-aws that referenced this pull request Mar 27, 2018
…a-pem

Fix the render command to not wipe ca.pem
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants