[WIP] - Add fix-certs cmd #444

tnozicka · 2019-04-29T13:20:43Z

Requires:

Add recovery-apiserver command #448

/cc @mfojtik @deads2k

openshift-ci-robot · 2019-04-29T13:21:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [tnozicka]

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

tnozicka · 2019-04-30T15:41:23Z

recovery-apiserver got it's own PR #448

sttts · 2019-05-06T07:45:15Z

Commit message is bad of second commit.

sttts · 2019-05-06T07:46:25Z

pkg/cmd/fixcerts/fixcerts.go

+	}
+
+	cmd := &cobra.Command{
+		Use: "fix-certs",


description is missing. What is this command doing?

sttts · 2019-05-06T07:46:33Z

pkg/cmd/fixcerts/fixcerts.go

+	KubeApiserverImage    string
+	PodManifestDir        string
+	StaticPodResourcesDir string
+	Timeout               time.Duration


Timeout of what?

sttts · 2019-05-06T07:46:45Z

pkg/cmd/fixcerts/fixcerts.go

+	cmd.Flags().StringVar(&o.KubeApiserverImage, "kube-apiserver-image", o.KubeApiserverImage, "")
+	cmd.Flags().StringVar(&o.StaticPodResourcesDir, "static-pod-resources", o.StaticPodResourcesDir, "path to store a directory containing static pod resources for recovery apiserver manifest")
+	cmd.Flags().StringVar(&o.PodManifestDir, "pod-manifest-dir", o.PodManifestDir, "directory for the static pod manifes")
+	cmd.Flags().DurationVar(&o.Timeout, "timeout", o.Timeout, "timeout, 0 means infinite")


Timeout of what?

tnozicka · 2019-05-06T07:47:03Z

yes, this one is in progress still

sttts · 2019-05-06T07:47:19Z

pkg/cmd/fixcerts/fixcerts.go

+		PodManifestDir:        o.PodManifestDir,
+		StaticPodResourcesDir: o.StaticPodResourcesDir,
+	}
+	defer apiserver.Destroy()


shouldn't this be after creation?

sttts · 2019-05-06T07:51:22Z

pkg/cmd/fixcerts/fixcerts.go

+		return fmt.Errorf("failed to create config client: %v", err)
+	}
+
+	operatorConfigInformers := operatorexternalinformers.NewSharedInformerFactory(operatorConfigClient, 10*time.Minute)


missing the start of both shared informer factories.

sttts · 2019-05-06T07:56:13Z

pkg/cmd/fixcerts/fixcerts.go

+		APIVersion: "v1",
+		Kind:       "namespace",
+		Name:       "kube-system",
+	}) // fake


what is fake?

sttts · 2019-05-06T07:57:03Z

pkg/cmd/fixcerts/fixcerts.go

+
+	certRotationWg := sync.WaitGroup{}
+	go func() {
+		certRotationWg.Add(1)


add outside of the loop. This can race.

tnozicka · 2019-05-06T07:58:24Z

@sttts this is WIP still, I'll let you know when it is ready to review, I have not even went through it myself yet :)

sttts · 2019-05-06T07:58:24Z

pkg/cmd/fixcerts/fixcerts.go

+		return fmt.Errorf("failed to find kube-apiserver certs dir: %v", err)
+	}
+
+	kubeControllerManagerManifest := path.Join(o.PodManifestDir, KubeControllerManagerStaticPodFileName)


sttts · 2019-05-06T07:59:46Z

pkg/cmd/fixcerts/fixcerts.go

+			return fmt.Errorf("unknown object type %q", def.objectType)
+		}
+
+		dir := path.Join(def.toplevelDir, def.objectType, def.name)


sttts · 2019-05-06T08:00:04Z

pkg/cmd/fixcerts/fixcerts.go

+
+		for name, bytes := range data {
+			filePath := path.Join(dir, name)
+			err := recovery.EnsureFileContent(filePath, bytes)


does this use 0600 for secrets?

keeps the perm of what was there

and fallback is 600

pkg/cmd/fixcerts/kubecontrollermanagercertrotation.go

mfojtik · 2019-05-06T12:44:40Z

Dockerfile

@@ -11,3 +11,6 @@ COPY --from=builder /go/src/github.com/openshift/cluster-kube-apiserver-operator
 COPY manifests/*.yaml /manifests
 COPY manifests/image-references /manifests
 LABEL io.openshift.release.operator true
+# FIXME: entrypoint shouldn't be bash but the binary (needs fixing the chain)


@mfojtik I'd expect a reason

mfojtik · 2019-05-06T12:44:45Z

Dockerfile.rhel7

@@ -11,3 +11,5 @@ COPY --from=builder /go/src/github.com/openshift/cluster-kube-apiserver-operator
 COPY manifests/*.yaml /manifests
 COPY manifests/image-references /manifests
 LABEL io.openshift.release.operator true
+# FIXME: entrypoint shouldn't be bash but the binary (needs fixing the chain)


mfojtik · 2019-05-06T12:47:24Z

pkg/cmd/recoveryapiserver/create.go

+func NewCreateCommand() *cobra.Command {
+	o := &CreateOptions{
+		Options:               NewDefaultOptions(),
+		KubeApiserverImage:    "", // TODO: set the public pullspec


remove, error out in Validate() ?

mfojtik · 2019-05-06T12:48:18Z

pkg/cmd/recoveryapiserver/create.go

+}
+
+func (o *CreateOptions) Run() error {
+	ctx, cancel := watch.ContextWithOptionalTimeout(context.Background(), o.Timeout)


context.TODO()

in future we will pass the context via Run() that context will be wired to TERM signal handler

mfojtik · 2019-05-06T12:49:04Z

pkg/cmd/recoveryapiserver/create.go

+func (o *CreateOptions) Run() error {
+	ctx, cancel := watch.ContextWithOptionalTimeout(context.Background(), o.Timeout)
+	defer cancel()
+	// TODO: hook up signals


termHandler := server.SetupSignalHandler() ctx, shutdown := context.WithCancel(context.TODO()) go func() { defer shutdown() <-termHandler }()

mfojtik · 2019-05-06T12:49:32Z

pkg/cmd/recoveryapiserver/create.go

+		return nil
+	}
+
+	fmt.Printf("Waiting for recovery apiserver to come up.")


use klog or at least add new line ;-)

mfojtik · 2019-05-06T12:51:36Z

pkg/recovery/apiserver.go

+		return nil, err
+	}
+
+	return &clientcmdapiv1.Config{


rest.CopyConfig

from where? you are also commenting on the wrong PR, this one is just picking up #448

mfojtik · 2019-05-06T12:51:57Z

pkg/recovery/apiserver.go

+	}, nil
+}
+
+func (s *Apiserver) GetKubeClientset() (*kubernetes.Clientset, error) {


why you need clientset?

deads2k · 2019-05-06T13:26:49Z

pkg/cmd/fixcerts/fixcerts.go

+	}
+
+	// TODO: add/prettify descriptions
+	cmd.Flags().StringVar(&o.KubeApiserverImage, "kube-apiserver-image", o.KubeApiserverImage, "")


same as other pull, I don't want to choose images or directories.

deads2k · 2019-05-06T13:27:04Z

pkg/cmd/fixcerts/fixcerts.go

+	defer cancel()
+	// TODO: hook up signals
+
+	apiserver := &recovery.Apiserver{


this looks weird.

deads2k · 2019-05-06T13:28:10Z

pkg/cmd/fixcerts/fixcerts.go

+		StaticPodResourcesDir: o.StaticPodResourcesDir,
+	}
+
+	err := apiserver.Create()


We should not do this. Fixing the certs should use the admin.kubeconfig from it's known location. It should not try to start a kube-apiserver

deads2k · 2019-05-06T13:30:08Z

pkg/cmd/fixcerts/fixcerts.go

+
+	operatorConfigInformers := operatorexternalinformers.NewSharedInformerFactory(operatorConfigClient, 10*time.Minute)
+	certRotationWg.Add(1)
+	go func() {


This is an odd structure. Start is non-blocking. I don't see a reason to wrap it in a gofunc

deads2k · 2019-05-06T13:32:36Z

pkg/cmd/fixcerts/fixcerts.go

+	certRotationWg.Add(1)
+	go func() {
+		defer certRotationWg.Done()
+		kubeApiserverInformersForNamespaces.Start(certRotationCtx.Done())


same coment about gofunc

deads2k · 2019-05-06T13:33:02Z

pkg/cmd/fixcerts/fixcerts.go

+	eventRecorder := events.NewKubeRecorder(kubeClient.CoreV1().Events(""), "fix-certs (CLI)", &corev1.ObjectReference{
+		APIVersion: "v1",
+		Kind:       "namespace",
+		Name:       "kube-system",


use the openshift-kube-apiserver-operator namespace and set the namespace value or we end up in default and unfindable.

deads2k · 2019-05-06T13:34:46Z

pkg/cmd/fixcerts/fixcerts.go

+	klog.Info("Waiting for certs to be refreshed...")
+	// FIXME: wait for valid certs
+	// time.Sleep(5*time.Minute)
+	time.Sleep(30 * time.Second)


This won't do. Add a RunOnce at each level so we can be sure we run and avoid the go func/defer, wait, hope flow.

This won't do. Add a RunOnce at each level so we can be sure we run and avoid the go func/defer, wait, hope flow.

You'll also need a WaitForPrereqs func to sync the bits at least once.

deads2k · 2019-05-06T13:37:19Z

pkg/cmd/fixcerts/fixcerts.go

+	}
+
+	timestamp := time.Now().Format(time.RFC3339)
+	kubeControllerManagerPod.Annotations["force-triggered-by-fix-certs-at"] = timestamp


I expected to see the kube-apiserver pod here too. Let's do them all, just to be sure.

openshift-ci-robot · 2019-05-06T17:23:28Z

@tnozicka: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-operator	`faea844`	link	`/test e2e-aws-operator`
ci/prow/e2e-aws	`faea844`	link	`/test e2e-aws`
ci/prow/verify	`faea844`	link	`/test verify`
ci/prow/unit	`faea844`	link	`/test unit`
ci/prow/images	`faea844`	link	`/test images`
ci/prow/e2e-aws-upgrade	`faea844`	link	`/test e2e-aws-upgrade`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

deads2k · 2019-05-06T21:00:54Z

merged in #461

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2019

openshift-ci-robot requested a review from mfojtik April 29, 2019 13:20

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 29, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2019

tnozicka changed the title ~~[WIP] - Add rotate certs cmd~~ [WIP] - Add fix certs cmd Apr 29, 2019

tnozicka changed the title ~~[WIP] - Add fix certs cmd~~ [WIP] - Add fix-certs cmd Apr 29, 2019

tnozicka force-pushed the add-rotate-certs-cmd branch from f030372 to 1063a46 Compare April 30, 2019 07:57

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2019

tnozicka force-pushed the add-rotate-certs-cmd branch 2 times, most recently from 272ef65 to 40fc9d5 Compare April 30, 2019 08:31

gbraad mentioned this pull request Apr 30, 2019

Retrigger certificate rotation/generation for internal cluster communication crc-org/crc#11

Closed

tnozicka force-pushed the add-rotate-certs-cmd branch from 40fc9d5 to a3c5f6e Compare April 30, 2019 13:12

tnozicka mentioned this pull request Apr 30, 2019

Add recovery-apiserver command #448

Closed

tnozicka force-pushed the add-rotate-certs-cmd branch from a3c5f6e to f005ff1 Compare May 3, 2019 15:17

sttts reviewed May 6, 2019

View reviewed changes

pkg/cmd/fixcerts/fixcerts.go

}

cmd := &cobra.Command{

Use: "fix-certs",

Copy link

Contributor

sttts May 6, 2019

Choose a reason for hiding this comment

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

description is missing. What is this command doing?

sttts reviewed May 6, 2019

View reviewed changes

pkg/cmd/fixcerts/fixcerts.go

APIVersion: "v1",

Kind: "namespace",

Name: "kube-system",

}) // fake

Copy link

Contributor

sttts May 6, 2019

Choose a reason for hiding this comment

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

what is fake?

sttts reviewed May 6, 2019

View reviewed changes

pkg/cmd/fixcerts/kubecontrollermanagercertrotation.go Outdated Show resolved Hide resolved

sttts reviewed May 6, 2019

View reviewed changes

pkg/cmd/fixcerts/kubecontrollermanagercertrotation.go Outdated Show resolved Hide resolved

This was referenced May 6, 2019

static-pod: fail liveness when secrets/kube-scheduler-client-cert-key changes openshift/cluster-kube-scheduler-operator#126

Closed

static-pod: fail liveness when secrets/csr-signer+serving-cert changes openshift/cluster-kube-controller-manager-operator#248

Closed

tnozicka force-pushed the add-rotate-certs-cmd branch from f005ff1 to d50803f Compare May 6, 2019 09:42

mfojtik reviewed May 6, 2019

View reviewed changes

deads2k reviewed May 6, 2019

View reviewed changes

tnozicka force-pushed the add-rotate-certs-cmd branch from d50803f to 237621a Compare May 6, 2019 13:31

deads2k reviewed May 6, 2019

View reviewed changes

tnozicka force-pushed the add-rotate-certs-cmd branch from 237621a to aa4a704 Compare May 6, 2019 17:17

tnozicka added 2 commits May 6, 2019 19:19

Add recovery-apiserver command

aa69fd2

sync

faea844

tnozicka force-pushed the add-rotate-certs-cmd branch from aa4a704 to faea844 Compare May 6, 2019 17:20

deads2k mentioned this pull request May 6, 2019

generate certificates synchronously and do not start a kube-apiserver #461

Merged

deads2k closed this May 6, 2019

[WIP] - Add fix-certs cmd #444

[WIP] - Add fix-certs cmd #444

Conversation

tnozicka commented Apr 29, 2019 • edited Loading

openshift-ci-robot commented Apr 29, 2019

tnozicka commented Apr 30, 2019

sttts commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 6, 2019

deads2k commented May 6, 2019

tnozicka commented Apr 29, 2019 •

edited

Loading