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

Bug 1710766: Add kubeconfigs to masters #858

Merged
merged 3 commits into from Sep 14, 2020
Merged

Bug 1710766: Add kubeconfigs to masters #858

merged 3 commits into from Sep 14, 2020

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented May 14, 2020 •
edited

When we ssh to a master there is currently no kubeconfig to use to talk to the apiserver. When we are debugging clusters and have only ssh access, building the kubeconfig manually and creating client certs is extremely painful and time consuming. Also when admin has an accident and looses system:admin kubeconfig, this can be used as temporary replacement. (He should still own the user client CA to sign new certs.) The kubeconfigs have intentionally inlined certificates for easy transfer.

The kubeconfigs being placed into /etc/kubernetes/static-pod-resources/kube-apiserver-certs/secrets/node-kubeconfigs are

  • localhost.kubeconfig
  • lb-ext.kubeconfig
  • lb-int.kubeconfig
  • localhost-recovery.kubeconfig

Currently none of them is linked as default to oc. That would probably need MCO to copy it over and somehow handle updates otherwise the oc writing (e.g. current context) would fight with cert-syncer.

Requires:

/cc @deads2k @sttts
@openshift/sig-master fyi, you'll need it at some point if you haven't already

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 14, 2020
@openshift-ci-robot
Copy link

@tnozicka: This pull request references Bugzilla bug 1710766, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP] Bug 1710766: Add kubeconfigs to masters

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 14, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2020
@tnozicka
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2020
@tnozicka tnozicka mentioned this pull request Jul 21, 2020
Merged
deads2k
deads2k reviewed Jul 21, 2020
View reviewed changes
pkg/operator/certrotationcontroller/certrotationcontroller.go Outdated
Namespace: operatorclient.OperatorNamespace,
Name: "machine-system-admin-client",
// This needs to be long lived so users can download it as backup kubeconfig if the initial one would be lost
Validity: 10 * 365 * defaultRotationDay,
Copy link
Contributor

Choose a reason for hiding this comment

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

30 days and 15 days is reasonable I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reasons for the client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it 120 and 30 so it always lives at least 30 days longer then kube-control-plane-signer. Does that work for you?

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2020
@openshift-ci-robot
Copy link

@tnozicka: This pull request references Bugzilla bug 1710766, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP] Bug 1710766: Add kubeconfigs to masters

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2020
@tnozicka tnozicka changed the title [WIP] Bug 1710766: Add kubeconfigs to masters Bug 1710766: Add kubeconfigs to masters Aug 25, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 25, 2020
@openshift-ci-robot
Copy link

@tnozicka: This pull request references Bugzilla bug 1710766, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1710766: Add kubeconfigs to masters

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tnozicka
Copy link
Contributor Author

tnozicka commented Sep 9, 2020

/retest

3 similar comments
@tnozicka
Copy link
Contributor Author

tnozicka commented Sep 9, 2020

/retest

@tnozicka
Copy link
Contributor Author

tnozicka commented Sep 9, 2020

/retest

@tnozicka
Copy link
Contributor Author

/retest

deads2k
deads2k reviewed Sep 10, 2020
View reviewed changes
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go Outdated
kubeInformersForNamespaces.InformersFor(operatorclient.OperatorNamespace).Core().V1().Secrets().Informer(),
kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Informer(),
infrastuctureInformer.Informer(),
).WithSync(c.sync).ResyncEvery(time.Second).ToController("NodeKubeconfigController", eventRecorder.WithComponentSuffix("node-kubeconfig-controller"))
Copy link
Contributor

Choose a reason for hiding this comment

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

resync is too tight. Every few minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering why I'd not write any number there, as I always prefer explicit 1 * time.Second and it is the same in targetconfigcontroller so it comes from the copy paste. We should likely fix it there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref: https://github.com/openshift/cluster-kube-apiserver-operator/blob/24ac500/pkg/operator/targetconfigcontroller/targetconfigcontroller.go#L75

deads2k
deads2k reviewed Sep 10, 2020
View reviewed changes
pkg/operator/nodekubeconfigcontroller/nodekubeconfigcontroller.go Outdated
kubeInformersForNamespaces.InformersFor(operatorclient.OperatorNamespace).Core().V1().Secrets().Informer(),
kubeInformersForNamespaces.InformersFor(operatorclient.TargetNamespace).Core().V1().Secrets().Informer(),
infrastuctureInformer.Informer(),
).WithSync(c.sync).ResyncEvery(time.Second).ToController("NodeKubeconfigController", eventRecorder.WithComponentSuffix("node-kubeconfig-controller"))
Copy link
Contributor

Choose a reason for hiding this comment

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

use the withdegraded option so you can just return an error and get degraded automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@deads2k
Copy link
Contributor

deads2k commented Sep 10, 2020

/approve

the time span is long but acceptable to start. If we need a way to mint a new admin.kubeconfig this should not be it and this is short enough to prevent that.

@tnozicka
Copy link
Contributor Author

comments addressed, @marun ptal

@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

/refresh

@tnozicka
Copy link
Contributor Author

/retest

3 similar comments
@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

/retest

@marun
Copy link
Contributor

marun commented Sep 11, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, marun, tnozicka

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tnozicka
Copy link
Contributor Author

/hold cancel
(e2e's are ready per #858 (comment))

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2020
@tnozicka
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 325b3bf into openshift:master Sep 14, 2020
@openshift-ci-robot
Copy link

@tnozicka: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state.

Bugzilla bug 1710766 has not been moved to the MODIFIED state.

In response to this:

Bug 1710766: Add kubeconfigs to masters

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tnozicka tnozicka deleted the local-kubeconfig branch September 14, 2020 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marun marun marun left review comments

@deads2k deads2k deads2k left review comments

@sttts sttts Awaiting requested review from sttts

Assignees

@marun marun

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tnozicka @openshift-ci-robot @deads2k @marun @openshift-bot @openshift-merge-robot