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.

Sign up for GitHub

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

Jump to bottom

Add cluster up configuration for certificate signing duration. #51844

Merged
merged 1 commit into from
Sep 8, 2017
Merged

Add cluster up configuration for certificate signing duration. #51844

merged 1 commit into from
Sep 8, 2017
+16 −1

Conversation

jcbsmpsn
Copy link
Contributor

@jcbsmpsn jcbsmpsn commented Sep 2, 2017 •
edited
Loading 

Add CLUSTER_SIGNING_DURATION environment variable to cluster configuration scripts
to allow configuration of signing duration of certificates issued via the Certificate
Signing Request API.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 2, 2017
@k8s-github-robot k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 2, 2017
@jcbsmpsn jcbsmpsn force-pushed the add-signing-duration-configuration-flag branch 2 times, most recently from d1c1fa7 to a3e7b2b Compare September 2, 2017 01:00
@mikedanese
Copy link
Member

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2017
@abgworrall abgworrall modified the milestone: v1.8 Sep 2, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2017
@jcbsmpsn jcbsmpsn force-pushed the add-signing-duration-configuration-flag branch from a3e7b2b to 1a09aa6 Compare September 5, 2017 18:10
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 5, 2017
@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@jcbsmpsn jcbsmpsn force-pushed the add-signing-duration-configuration-flag branch from 1a09aa6 to bd0c16c Compare September 5, 2017 22:56
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2017
@gmarek
Copy link
Contributor

gmarek commented Sep 6, 2017

/unassign

@ericchiang
Copy link
Contributor

/sig auth
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2017
@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Sep 8, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, jcbsmpsn, mikedanese

Associated issue requirement bypassed by: mikedanese

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ed15498 into kubernetes:master Sep 8, 2017
This was referenced Sep 11, 2017
Closed
Closed
yujuhong
yujuhong reviewed Sep 11, 2017
View reviewed changes
cluster/gce/config-test.sh
@@ -367,6 +367,9 @@ ENABLE_PROMETHEUS_TO_SD="${ENABLE_PROMETHEUS_TO_SD:-true}"
# Optional: Run kube-proxy as a DaemonSet if set to true, run as static pods otherwise.
KUBE_PROXY_DAEMONSET="${KUBE_PROXY_DAEMONSET:-false}" # true, false

# Optional: duration of cluster signed certificates.
CLUSTER_SIGNING_DURATION="${CLUSTER_SIGNING_DURATION:-30m}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be enabled for all test clusters by default.

Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC (which may not be the case), kubelet needs to rotate client certificate and it's not enabled by default.

Copy link
Member

Choose a reason for hiding this comment

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

So why do we then revert vs actually enabling kubelet cert rotation on kubelets?
cc @ericchiang
(Not opposed, just asking)

Copy link
Contributor

Choose a reason for hiding this comment

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

@luxas I really just want to get the tests back to green as soon as possible. Once that's done happy to see other PRs modifying the test setup.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM
@jcbsmpsn probably can create an issue to follow this up later

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, reverting is the standard practice in cases like this.

Also, I am unsure whether this should be tested in a separate job, or enabled by default for all test clusters.

This was referenced Sep 11, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yujuhong yujuhong yujuhong left review comments

@luxas luxas luxas left review comments

@ericchiang ericchiang ericchiang left review comments

Assignees

@mikedanese mikedanese

@ericchiang ericchiang

@eparis eparis

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.