Creating the keyset.yaml file if it does not exist #4582

chrislovecnm · 2018-03-05T17:52:27Z

If the Keypair is not found kops creates a new keyset file. We are
setting the Format to v1, which denotes that we do not have a keyset
file.

This commit enables upgrading from kops 1.8 -> 1.9 while upgrading an
existing cluster. Clusters built with kops 1.8 do not have the keyset
file, and these code changes allow the creation of the keyset file.

Fixes: #4546

~~The only thing I do not like about this fix is that we are generating a new private key. Not a horrible thing, but not sure how stuff like etcd will behave.~~

Edit: with changes I am not generating a new private key.

chrislovecnm · 2018-03-05T22:34:00Z

/assign @blakebarnett @mikesplain

blakebarnett · 2018-03-05T22:34:59Z

Apparently this results in a new private key being generated for the CA? This would require us to rotate every cert we've created using the CA (a large number), if there's any way to make it re-use the existing private key that would be very preferable.

cc: @justinsb

chrislovecnm · 2018-03-05T22:41:28Z

@blakebarnett I am not certain that a new private cert is being generated or we are creating two private certs with a keyset.yaml.

@justinsb any ideas?

chrislovecnm · 2018-03-05T22:52:27Z

upup/pkg/fi/fitasks/keypair.go

 		} else {
 			glog.Warningf("Ignoring changes in key: %v", fi.DebugAsJsonString(changes))
 		}
 	}

+	// TODO: I am wondering if we reuse the certificate information when a.KeysetAPIVersion == "" rather than
+	// TODO: issuing the new cert


I am seeing two private pems in the keypair.yaml file. Any idea how we can re-use the pem?

I think that we are eventually calling https://github.com/kubernetes/kops/blob/master/pkg/pki/csr.go#L94

And that is creating the new cert.

kops/pkg/pki/csr.go

Line 94 in 0b0932b

certificateData, err := x509.CreateCertificate(crypto_rand.Reader, template, parent, template.PublicKey, signerPrivateKey.Key)

justinsb · 2018-03-06T01:45:13Z

upup/pkg/fi/fitasks/keypair.go

+		} else if a.KeypairType == "" {
+			// The a.KeysetAPIVersion will == "" when the keyset is a legacy keyset and does not have a keypair.yaml
+			// file store in the state store
+			createCertificate = true


Why are you setting createCertificate here?

I can test without it, but I think my testing did not create the keypair.yaml.

As we discussed, pretty sure this shouldn't drive creation of a new certificate, but should instead drive a new UpdateStoredVersion or similar call

justinsb · 2018-03-06T02:18:28Z

pkg/model/pki.go

@@ -34,6 +34,9 @@ type PKIModelBuilder struct {

 var _ fi.ModelBuilder = &PKIModelBuilder{}

+// keypairType is used to differentiate the old secretes with the newer versions


typo: secretes

we need the spelling tool

justinsb · 2018-03-06T02:20:04Z

pkg/model/pki.go

@@ -34,6 +34,9 @@ type PKIModelBuilder struct {

 var _ fi.ModelBuilder = &PKIModelBuilder{}

+// keypairType is used to differentiate the old secretes with the newer versions
+const keypairType = "Keypair"


What is the other type? Maybe we have a field "Format" or "APIVersion", and we have "legacy" & "kops/v1alpha2" as that most closely matches what we're actually doing, I think.

Although the obvious problem with this is that I don't know how easy it is to get the stored version from the apimachinery. But I'm OK with cheating at the moment (assuming v1alpha2)

This was the only thing in the API object that I could find that was actually set. I tested the heck out of this, and the only thing I could figure out how to access is the object name Keypair.

justinsb · 2018-03-06T02:23:44Z

upup/pkg/fi/fitasks/keypair.go

@@ -51,6 +51,9 @@ type Keypair struct {
 	Subject string `json:"subject"`
 	// Type the type of certificate i.e. CA, server, client etc
 	Type string `json:"type"`
+	// KeypairType stores the api version of kops.Keyset.  We are storing this info in order to determine if kops
+	// is accessing legacy secrets that do not use keyset.yaml.
+	KeypairType string `json:"keypairType"`


I would call it APIVersion or Format. And then I'd say "This lets us convert the stored keypair to our preferred version." I don't think we should say "We are storing this info" because we're not storing it - tasks are ephemeral

Sure ... renaming

justinsb · 2018-03-06T02:24:11Z

upup/pkg/fi/ca.go

@@ -44,7 +44,7 @@ type KeystoreItem struct {
 type Keystore interface {
 	// FindKeypair finds a cert & private key, returning nil where either is not found
 	// (if the certificate is found but not keypair, that is not an error: only the cert will be returned)
-	FindKeypair(name string) (*pki.Certificate, *pki.PrivateKey, error)
+	FindKeypair(name string) (*pki.Certificate, *pki.PrivateKey, string, error)


I'm a little worried that we might need a version for both cert & privatekey, but we'll see...

Well your test is not working ... see your PR for comments.

justinsb · 2018-03-06T02:25:04Z

upup/pkg/fi/fitasks/keypair.go

+		} else if a.KeypairType == "" {
+			// The a.KeysetAPIVersion will == "" when the keyset is a legacy keyset and does not have a keypair.yaml
+			// file store in the state store
+			createCertificate = true


As we discussed, pretty sure this shouldn't drive creation of a new certificate, but should instead drive a new UpdateStoredVersion or similar call

justinsb · 2018-03-06T02:26:31Z

upup/pkg/fi/k8sapi/k8s_keystore.go

 	}

-	return keypair.Certificate, keypair.PrivateKey, nil
+	// TODO: KubernetesKeystore does not seem to be using the newer version of keypairs.


Not sure what you mean. keyset.yaml doesn't make sense here - keyset.yaml makes a filesystem behave more like an API server. So we don't need keyset.yaml for an API-server-backed key store.

justinsb · 2018-03-06T02:27:12Z

upup/pkg/fi/vfs_castore.go

@@ -233,27 +233,27 @@ func (c *VFSCAStore) parseKeysetYaml(data []byte) (*kops.Keyset, error) {
 // loadCertificatesBundle loads a keyset from the path
 // Returns (nil, nil) if the file is not found
 // Bundles avoid the need for a list-files permission, which can be tricky on e.g. GCE
-func (c *VFSCAStore) loadKeysetBundle(p vfs.Path) (*keyset, error) {
+func (c *VFSCAStore) loadKeysetBundle(p vfs.Path) (*keyset, string, error) {


You could put the stored version into the (internal) keyset type - should be simpler

need more context

chrislovecnm · 2018-03-08T22:52:46Z

tests/keypair_test.go

+// memfs://keystore/private/ca/keyset.yaml
+// memfs://keystore/private/kubelet/2.key
+// memfs://keystore/private/kubelet/4.key
+// memfs://keystore/private/kubelet/keyset.yaml


Test is failing and this is what I am getting :(

robinpercy · 2018-03-09T18:25:39Z

upup/pkg/fi/fitasks/keypair.go

 		} else if changes.Type != "" {
 			createCertificate = true
+			glog.V(8).Infof("creating certificate new Type")
 		} else {
 			glog.Warningf("Ignoring changes in key: %v", fi.DebugAsJsonString(changes))


@chrislovecnm As per our out-of-band convo, can we suppress this during an upgrade?

robinpercy · 2018-03-09T18:27:50Z

I've run through a couple tests that create a cluster with kops 1.8, then use this patch to upgrade the cluster. keyset.yaml is being created correctly.

chrislovecnm · 2018-03-09T19:35:42Z

/assign @robinpercy

PTAL

chrislovecnm · 2018-03-09T19:39:37Z

/hold

Creating the keypair.yaml file if it does not exist. If the Keypair is not found kops creates a new keyset file. We are setting the Keyset Task Format to 'Keypair', which denotes that we do not have a keypair.yaml file. This commit enables upgrading from kops 1.8 -> 1.9 while upgrading an existing cluster. Clusters built with kops 1.8 do not have the keypair file, and these code changes allow the creation of that file.

chrislovecnm · 2018-03-09T20:16:12Z

I have refactored some, and added a bunch of code level documentation. Both @robinpercy and I have tested and the upgrades are working well :) When you upgrade to kops 1.9 from kops 1.8 and update and existing cluster built with kops 1.8 the new keypair.yaml files are created.

robinpercy · 2018-03-09T21:00:19Z

/lgtm

chrislovecnm · 2018-03-09T21:20:49Z

@justinsb I patched and modified your test PR here: ae759ce

Added set the Format member, and flipped the boolean value as we talked about. I will drop the commit once you let me know that you have seen the changes.

The pass should be passing now. PTAL

justinsb · 2018-03-10T20:43:40Z

/lgtm

k8s-ci-robot · 2018-03-10T20:43:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, justinsb, robinpercy

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~~ [chrislovecnm,justinsb]

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

chrislovecnm · 2018-03-10T20:55:10Z

/hold cancel

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 5, 2018

chrislovecnm force-pushed the create-keyset-yaml branch from 02f3111 to 3e03bd6 Compare March 5, 2018 18:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 5, 2018

chrislovecnm force-pushed the create-keyset-yaml branch 2 times, most recently from 3a4536f to 3809385 Compare March 5, 2018 22:28

k8s-ci-robot assigned blakebarnett and mikesplain Mar 5, 2018

chrislovecnm commented Mar 5, 2018

View reviewed changes

justinsb reviewed Mar 6, 2018

View reviewed changes

justinsb mentioned this pull request Mar 7, 2018

Test for keyset.yaml generation #4598

Closed

chrislovecnm force-pushed the create-keyset-yaml branch from 3809385 to 903b153 Compare March 8, 2018 21:16

chrislovecnm commented Mar 8, 2018

View reviewed changes

chrislovecnm force-pushed the create-keyset-yaml branch 2 times, most recently from f882a0d to 7740ec3 Compare March 9, 2018 18:17

robinpercy reviewed Mar 9, 2018

View reviewed changes

chrislovecnm force-pushed the create-keyset-yaml branch from 7740ec3 to 2b9afa4 Compare March 9, 2018 19:33

k8s-ci-robot assigned robinpercy Mar 9, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2018

chrislovecnm force-pushed the create-keyset-yaml branch 2 times, most recently from c0f1193 to 6939861 Compare March 9, 2018 19:59

chrislovecnm force-pushed the create-keyset-yaml branch from 6939861 to e719d56 Compare March 9, 2018 20:11

chrislovecnm force-pushed the create-keyset-yaml branch from e719d56 to c13b952 Compare March 9, 2018 20:14

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 9, 2018

patching with PR 4598, need to remove this commit

f785d2b

chrislovecnm force-pushed the create-keyset-yaml branch from ae759ce to f785d2b Compare March 9, 2018 21:22

k8s-ci-robot assigned justinsb Mar 10, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2018

k8s-ci-robot merged commit 6606ab4 into kubernetes:master Mar 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating the keyset.yaml file if it does not exist #4582

Creating the keyset.yaml file if it does not exist #4582

chrislovecnm commented Mar 5, 2018 •

edited

Loading

chrislovecnm commented Mar 5, 2018

blakebarnett commented Mar 5, 2018

chrislovecnm commented Mar 5, 2018

chrislovecnm Mar 5, 2018

chrislovecnm Mar 5, 2018

chrislovecnm Mar 5, 2018

justinsb Mar 6, 2018

chrislovecnm Mar 6, 2018

justinsb Mar 6, 2018

justinsb Mar 6, 2018

chrislovecnm Mar 8, 2018

justinsb Mar 6, 2018 •

edited

Loading

chrislovecnm Mar 8, 2018

justinsb Mar 6, 2018

chrislovecnm Mar 8, 2018

justinsb Mar 6, 2018

chrislovecnm Mar 8, 2018

justinsb Mar 6, 2018

justinsb Mar 6, 2018

justinsb Mar 6, 2018

chrislovecnm Mar 8, 2018

chrislovecnm Mar 8, 2018

robinpercy Mar 9, 2018

chrislovecnm Mar 9, 2018

robinpercy commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

robinpercy commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

justinsb commented Mar 10, 2018

k8s-ci-robot commented Mar 10, 2018

chrislovecnm commented Mar 10, 2018

		@@ -34,6 +34,9 @@ type PKIModelBuilder struct {

		var _ fi.ModelBuilder = &PKIModelBuilder{}

		// keypairType is used to differentiate the old secretes with the newer versions

Creating the keyset.yaml file if it does not exist #4582

Creating the keyset.yaml file if it does not exist #4582

Conversation

chrislovecnm commented Mar 5, 2018 • edited Loading

chrislovecnm commented Mar 5, 2018

blakebarnett commented Mar 5, 2018

chrislovecnm commented Mar 5, 2018

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

justinsb Mar 6, 2018 • edited Loading

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

robinpercy commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

robinpercy commented Mar 9, 2018

chrislovecnm commented Mar 9, 2018

justinsb commented Mar 10, 2018

k8s-ci-robot commented Mar 10, 2018

chrislovecnm commented Mar 10, 2018

chrislovecnm commented Mar 5, 2018 •

edited

Loading

justinsb Mar 6, 2018 •

edited

Loading