Pr implement reconcile certs #394

sbueringer · 2019-07-11T17:57:19Z

As the one before. This PR integrates code from CAPA. This PR will generate & store certificates
in the Cluster CRD. With this we're now able to set certifcates on every control plane node (multi-node support will be added in a later PR). We're also able to generate a kubeconfig whenever we won't without ssh'ing on a control plane node

What this PR does / why we need it:
Another step towards multi-node control plane support. Without this we would have to copy certificates between control plane nodes

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Implements part of #382

k8s-ci-robot · 2019-07-11T17:57:32Z

Hi @sbueringer. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

sbueringer · 2019-07-11T17:58:57Z

pkg/cloud/openstack/services/certificates/certificates.go

+
+// ReconcileCertificates generate certificates if none exists.
+func (s *Service) ReconcileCertificates(clusterName string, clusterProviderSpec *v1alpha1.OpenstackClusterProviderSpec) error {
+	if !clusterProviderSpec.CAKeyPair.HasCertAndKey() {


@jichenjc Your comment from the PR on the other repository:

from code, looks like you are trying to generate those keys? if it's generated, then how do you store them and reuse? looks to me the Spec are the desired state but if you didn't give key at beginning, it's not desired state?? just curious ..

@jichenjc The logic is the following:

If no certificates are set in the Cluster CRD, ReconcileCertificates will generate them and add them to the Cluster CRD. But it's also possible (if wanted) to set the certificates when creating the Cluster CRD.

The main reasoning behind this is that we need a place to store certificates so we can give the same certificates to multiple control plane nodes, without copying them from one host to another. Also it's now possible to generate a kubeconfig based on the Cluster CRD alone.

I guess the Cluster CRD might not be the best place for the certs going forward but it's straight forward. But for now it's the same as in CAPA and we always can move them in the future.

I understand the logic here :), the question I have originally is I didn't see some code like 'save' ' store' the Cert and key ...so I assume kubeconfig will be the place to hold those stuff..

Got it :). That's not really intuitive but the whole spec & status are saved here:

cluster-api-provider-openstack/pkg/cloud/openstack/cluster/actuator.go

Line 107 in 51c65e2

defer func() {

But I see the defer statement is way to late. I moved it up in another PR. But I'll do it now in this PR.

It should only be a matter of cluster actuator reconciling the certs. As soon as that's done the machine actuator can use them. As long as the certs are not in the Cluster CRD yet the machine actuator has to wait (if not it will render empty certs in the userData)

k8s-ci-robot · 2019-07-11T18:05:27Z

@sbueringer: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

sbueringer · 2019-07-11T18:07:42Z

/assign @chrigl
/assign @jichenjc

jichenjc · 2019-07-12T00:08:59Z

/ok-to-test

jichenjc · 2019-07-12T00:09:34Z

I'll try this on my env and let you know :)

sbueringer · 2019-07-12T05:25:42Z

I'll try this on my env and let you know :)

Thx :). I could only verify it with CoreOS in my environment

sbueringer · 2019-07-13T08:21:35Z

I'll rebase on top of master and also test it with clusterctl

This is heavily inspired bei CAPA. We now reconcile and store certificates in the Cluster CRD. Thus it's possible to distribute the same CAs over all control plane nodes (as soon as multi-node control plane supported is implemented). We also don't have to ssh on the/a control plane node to get a valid kubeconfig. We now can just generate one from the CA.

… controllerClient This enables us to run the controller outside the Workload Cluster, e.g. in a Management Cluster.

Now it's possible to use a local userdata folder. This is mostly useful for development to avoid updating the user data Secrets all the time.

jichenjc · 2019-07-15T01:59:25Z

hi @sbueringer I tried this PR my understanding (from above comments) is this can be auto generated
but I got this error ,I guess I need more time to know how to add CA stuffs...

E0715 01:57:11.448579 1 actuator.go:323] Machine error openstack-master-8w7cr: error creating Openstack instance: CA cert material in the ClusterProviderSpec is missing cert/key
W0715 01:57:11.448610 1 machine_controller.go:229] Failed to create machine "openstack-master-8w7cr": error creating Openstack instance: CA cert material in the ClusterProviderSpec is missing cert/key

sbueringer · 2019-07-15T04:19:43Z

hi @sbueringer I tried this PR my understanding (from above comments) is this can be auto generated
but I got this error ,I guess I need more time to know how to add CA stuffs...

E0715 01:57:11.448579 1 actuator.go:323] Machine error openstack-master-8w7cr: error creating Openstack instance: CA cert material in the ClusterProviderSpec is missing cert/key
W0715 01:57:11.448610 1 machine_controller.go:229] Failed to create machine "openstack-master-8w7cr": error creating Openstack instance: CA cert material in the ClusterProviderSpec is missing cert/key

Hi, That error is kind of expected. But only a few times. So the machine actuator should wait until the cluster actuator reconciled the certificates. After that is done the machine should be created. I tried the whole clusterctl flow with CoreOS and it worked.

jichenjc · 2019-07-15T04:23:25Z

perfect, I will wait for longer time , I might be too rushy here.. thanks for the detail info~

sbueringer · 2019-07-15T04:26:11Z

Yup. But it shouldn't take to long. Maybe the problem is that something fails in the Cluster actuator and that's why the defer is never triggered

sbueringer · 2019-07-15T05:05:18Z

@jichenjc Commit regarding defer storeCluster is pushed

jichenjc · 2019-07-16T00:46:00Z

cmd/clusterctl/examples/openstack/machine-deployment.yaml.template

          kind: "OpenstackProviderSpec"
          flavor: m1.medium
          image: <Image Name>
-          sshUserName: <SSH Username>


I would keep those for debug purpose.... or at least show a way to user how to debug issues

and sshUsername and keyname below should be a group... remove or keep them both

I'm not sure.

keyname is used to inject the given keypair. So that's absolutely necessary to jump on the host later, without it, it won't work. sshUserName would be only to document to the user inside the Machine resource what the name of his os user is. I'm not sure if he already knows that and if it's strictly necessary to put this inside the Machine resource

But I also don't have very strong opinions on keeping or deleting them both. I think deleting both is not really an option because without keyname we don't inject the key which makes it possible in the first place to ssh onto the host.

I guess I would go with documenting how to ssh onto the host, because leaving the property inside the machine resource implies to me that it's required by the Cluster API controllers in some way, which it isn't.

jichenjc · 2019-07-16T01:09:06Z

I confirmed ubuntu test can pass test (though some other minor changes but not related) :)

so lgtm if above comments addressed... thanks~

jichenjc · 2019-07-16T04:31:08Z

ok, I think a follow up document update will be needed.
I can do that update accordingly

jichenjc · 2019-07-16T04:31:14Z

/lgtm

jichenjc · 2019-07-16T04:31:41Z

give other folks a chance to take a look

sbueringer · 2019-07-16T05:37:51Z

ok, I think a follow up document update will be needed.
I can do that update accordingly

That would be nice. Thx!

give other folks a chance to take a look

Yup of course. I pinged @chrigl already. I cannot approve it anyway :)

jichenjc · 2019-07-17T01:32:31Z

/approve

we do have chance to fix minor issues later on, for now, since both of us tested, I think we can go with it

k8s-ci-robot · 2019-07-17T01:32:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, sbueringer

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~~ [jichenjc]

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

jichenjc · 2019-07-17T09:27:47Z

pkg/deployer/deployer.go

-	parts := strings.Split(result, "STARTFILE")
-	if len(parts) != 2 {
-		return "", nil
+	server := fmt.Sprintf("https://%s:6443", ip)


this might be the root cause for #399

* Reconcile certificates and store them in the cluster crd This is heavily inspired bei CAPA. We now reconcile and store certificates in the Cluster CRD. Thus it's possible to distribute the same CAs over all control plane nodes (as soon as multi-node control plane supported is implemented). We also don't have to ssh on the/a control plane node to get a valid kubeconfig. We now can just generate one from the CA. * Generate a kubeconfig for the Workload cluster instead of reusing the controllerClient This enables us to run the controller outside the Workload Cluster, e.g. in a Management Cluster. * Add options for local user data Now it's possible to use a local userdata folder. This is mostly useful for development to avoid updating the user data Secrets all the time. * added vaildate certificates * Removed sshUserName from templates, moved defer storeCluster up

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 11, 2019

k8s-ci-robot requested review from flaper87 and krousey July 11, 2019 17:57

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 11, 2019

sbueringer mentioned this pull request Jul 11, 2019

Pr implement reconcile certs sbueringer/cluster-api-provider-openstack#1

Closed

sbueringer commented Jul 11, 2019

View reviewed changes

sbueringer force-pushed the pr-implement-reconcile-certs branch from d0bb419 to 4e393d7 Compare July 11, 2019 18:04

k8s-ci-robot assigned chrigl and jichenjc Jul 11, 2019

sbueringer mentioned this pull request Jul 11, 2019

Support for multi-node control plane #382

Closed

4 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 12, 2019

sbueringer added 3 commits July 13, 2019 10:30

Generate a kubeconfig for the Workload cluster instead of reusing the…

9b4e1f7

… controllerClient This enables us to run the controller outside the Workload Cluster, e.g. in a Management Cluster.

Add options for local user data

ee1e643

Now it's possible to use a local userdata folder. This is mostly useful for development to avoid updating the user data Secrets all the time.

sbueringer force-pushed the pr-implement-reconcile-certs branch from b875d2e to ee1e643 Compare July 13, 2019 08:31

added vaildate certificates

51c65e2

Removed sshUserName from templates, moved defer storeCluster up

7213750

jichenjc reviewed Jul 16, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2019

k8s-ci-robot merged commit 5524321 into kubernetes-sigs:master Jul 17, 2019

sbueringer mentioned this pull request Jul 17, 2019

avoid ssh to master node to get kubeconfig if we can #387

Closed

jichenjc reviewed Jul 17, 2019

View reviewed changes

jichenjc mentioned this pull request Jul 17, 2019

port number 443 in master script is incorrect #399

Closed

sbueringer deleted the pr-implement-reconcile-certs branch July 27, 2019 12:48

EmilienM mentioned this pull request Jan 31, 2023

Manual rebase on upstream openshift/cluster-api-provider-openstack#252

Closed

Pr implement reconcile certs #394

Pr implement reconcile certs #394

Uh oh!

Conversation

sbueringer commented Jul 11, 2019

Uh oh!

k8s-ci-robot commented Jul 11, 2019

Uh oh!

sbueringer Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jichenjc Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 11, 2019

Uh oh!

sbueringer commented Jul 11, 2019

Uh oh!

jichenjc commented Jul 12, 2019

Uh oh!

jichenjc commented Jul 12, 2019

Uh oh!

sbueringer commented Jul 12, 2019

Uh oh!

sbueringer commented Jul 13, 2019

Uh oh!

jichenjc commented Jul 15, 2019

Uh oh!

sbueringer commented Jul 15, 2019

Uh oh!

jichenjc commented Jul 15, 2019

Uh oh!

sbueringer commented Jul 15, 2019

Uh oh!

sbueringer commented Jul 15, 2019

Uh oh!

jichenjc Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jichenjc commented Jul 16, 2019

Uh oh!

jichenjc commented Jul 16, 2019

Uh oh!

jichenjc commented Jul 16, 2019

Uh oh!

jichenjc commented Jul 16, 2019

Uh oh!

sbueringer commented Jul 16, 2019

Uh oh!

jichenjc commented Jul 17, 2019

Uh oh!

k8s-ci-robot commented Jul 17, 2019

Uh oh!

jichenjc Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sbueringer Jul 11, 2019 •

edited

Loading

sbueringer Jul 16, 2019 •

edited

Loading