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

Fix so setup-files don't recreate/invalidate certificates that already exist #23550

Merged
merged 1 commit into from
Apr 1, 2016

Conversation

luxas
Copy link
Member

@luxas luxas commented Mar 28, 2016

Fixes: #23197 and a lot of other DNS and dashboard issues

This is quite critical for docker-based users and should be considered as a cherrypick-candidate as it makes a lot of people wonder why Dashboard and/or DNS doesn't work. Example: kubernetes/dashboard#374

Earlier when you shut your docker.md cluster down and started it again, all ServiceAccounts became invalidated by setup-files that happily ran once again and replaced all files. That made apiserver and controller-manager pick up the new certs (or there was a race condition, they could have picked up the old certs too, but that's unlikely) and the old certs were put into /var/run/secrets because the ServiceAccount's Secrets were stored in etcd, which setup-files didn't touch.

@fgrzadkowski @huggsboson @thockin @mikedanese @vishh @pwittrock @eparis @bgrant0607

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 28, 2016
# Create known tokens for service accounts
echo "$(create_token),admin,admin" >> /data/known_tokens.csv
echo "$(create_token),kubelet,kubelet" >> /data/known_tokens.csv
echo "$(create_token),kube_proxy,kube_proxy" >> /data/known_tokens.csv7
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a 7 got added here.

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, fixed

@huggsboson
Copy link
Contributor

other than the 7 looks good to me. My other comment is more speculative and a nice to have.

@luxas
Copy link
Member Author

luxas commented Mar 28, 2016

Yep, fixed the typo

@luxas
Copy link
Member Author

luxas commented Mar 28, 2016

@vishh Does this look good to you?

@k8s-bot
Copy link

k8s-bot commented Mar 28, 2016

GCE e2e build/test passed for commit 84fbbea48d6ee6ecf9b4e38e529d13932f4c312a.

@k8s-bot
Copy link

k8s-bot commented Mar 28, 2016

GCE e2e build/test passed for commit dcbc95ea953bc2f3d92407d1f42a7d9e7941fad2.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@cheld
Copy link
Contributor

cheld commented Mar 29, 2016

CC @zreigz, @batikanu

@vishh
Copy link
Contributor

vishh commented Mar 29, 2016

LGTM

@luxas
Copy link
Member Author

luxas commented Mar 29, 2016

How do one retest travis?

@luxas
Copy link
Member Author

luxas commented Mar 31, 2016

@fgrzadkowski Could we please just merge it? 😄
This patch makes it a lot better, and I don't think more complex if clauses will make it much better.
bash isn't race-safe after all, so if we want a better solution, we should probably swap bash for a go solution for example. I think that is overkill though. But it might be an option if we're going for a
somewhat prod-ready docker setup. Also, init containers are coming for v1.3 as I understand it, and then we'll switch to that solution instead.

But this will make it a much better for v1.2, and I think it might be a cherrypick-candidate.
WDYT?

@huggsboson
Copy link
Contributor

+1 for merging.

Side note: I do think I've seen the script die while generating the tokens, but not since the alphas, but I can't remember clearly, so it's probably not worth pursuing. Plus I can add a comment to trouble shooting section of the docs so people can blow away any kube state.

@luxas
Copy link
Member Author

luxas commented Apr 1, 2016

@fgrzadkowski Thought about it a bit more and updated to separate checks.
PTAL

@fgrzadkowski fgrzadkowski added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2016
@fgrzadkowski
Copy link
Contributor

LGTM. Thanks for the change!

@fgrzadkowski fgrzadkowski added this to the v1.2 milestone Apr 1, 2016
@luxas
Copy link
Member Author

luxas commented Apr 1, 2016

Then I think the release-note-label-needed label should be swapped for the release-note label

@fgrzadkowski
Copy link
Contributor

AFAIU once we set release-note label it will just use PR title for release notes.

@david-mcmahon Is my understand correct?

@fgrzadkowski fgrzadkowski added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Apr 1, 2016
@luxas
Copy link
Member Author

luxas commented Apr 1, 2016

Yes, I think so.
Is the title ok as-is?

@eparis
Copy link
Contributor

eparis commented Apr 1, 2016

That is correct right now. We are considering future alternatives/changes.

@david-mcmahon
Copy link
Contributor

Yes, apologies. We've been debating in an updated version of a PR updating the release-note guidance. I hope to get this in soon.
@eparis, when the bot adds release-note-label-needed, can we have it refer to https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit 858b953.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 1, 2016

GCE e2e build/test passed for commit 858b953.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@zmerlynn
Copy link
Member

zmerlynn commented Apr 4, 2016

I assume we still need the cherrypick-approved process? @eparis, @bgrant0607?

@luxas
Copy link
Member Author

luxas commented Apr 4, 2016

@zmerlynn Yes, please add the cherrypick-approved and LGTM the cherrypick PR.
Ref: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/cherry-picks.md#propose-a-cherry-pick

@eparis
Copy link
Contributor

eparis commented Apr 4, 2016

@zmerlynn you do need that label. I'm sure @bgrant0607 will come through soonish. appears there are 4 PRs like this that need review.

@bgrant0607 bgrant0607 added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 6, 2016
zmerlynn added a commit that referenced this pull request Apr 6, 2016
…upstream-release-1.2

Automated cherry pick of #23550 upstream release 1.2
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.