SecretStore and CAStore implementations backed by API #3409
Conversation
k8s-ci-robot
added
cncf-cla: yes
justinsb
force-pushed
the
clientset_secret_stores
branch
2 times, most recently
from
40e06fd
to
327794e
Compare
justinsb
force-pushed
the
clientset_secret_stores
branch
from
327794e
to
d99927b
Compare
upup/pkg/fi/clientset_castore.go
Outdated
| "k8s.io/kops/util/pkg/vfs" | ||
| "math/big" | ||
| "sync" | ||
| "time" |
There was a problem hiding this comment.
How does core format there imports? Should we have time and such on top?
There was a problem hiding this comment.
Applied goimports. It would change a lot of things to bulk apply it now, but you're right - we should certainly apply it where we can.
(It also isn't perfect, in that it seems to keep existing blocks - but it's better than nothing)
upup/pkg/fi/clientset_castore.go
Outdated
| "time" | ||
| ) | ||
|
|
||
| type ClientsetCAStore struct { |
| if keyset == nil { | ||
| keyset, err = c.generateCACertificate() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
We have lots of err not fmt. What should we be doing?
There was a problem hiding this comment.
If we're calling an internal function where we can't really add context to the error message, I tend to return as-is. I don't know what information I could add that generateCACertificate does not have.
| @@ -0,0 +1,630 @@ | |||
| /* | |||
There was a problem hiding this comment.
Very little if any logging and no unit tests. How do we test this, and should we add some logging to help with debugging?
There was a problem hiding this comment.
We have logging, read more ;)
| } | ||
|
|
||
| // IssueCert implements CAStore::IssueCert | ||
| func (c *ClientsetCAStore) IssueCert(name string, serial *big.Int, privateKey *pki.PrivateKey, template *x509.Certificate) (*pki.Certificate, error) { |
There was a problem hiding this comment.
So is this where we would tie in for using an external service for certs?
There was a problem hiding this comment.
Unclear. You can already the API with a "real" certificate today using ingress. If you're using using an external service for certs, you also have to tie in all the kubernetes certificate issuance into that (i.e. all the internal certs) and I am not aware of anyone that has done so.
| } | ||
|
|
||
| // Make double-sure it round-trips | ||
| keyset, err := c.loadKeyset(name) |
There was a problem hiding this comment.
Do we have duplicate code doing this twice?
There was a problem hiding this comment.
Should we wrap this in a helper method? We are call something like this twice.
There was a problem hiding this comment.
Created storeAndVerifyKeypair helper
| // NamePrefix is a prefix we use to avoid collisions with other keysets | ||
| const NamePrefix = "token-" | ||
|
|
||
| type ClientsetSecretStore struct { |
| _, err = c.createSecret(secret, name) | ||
| if err != nil { | ||
| if errors.IsAlreadyExists(err) && i == 0 { | ||
| glog.Infof("Got already-exists error when writing secret; likely due to concurrent creation. Will retry") |
There was a problem hiding this comment.
Anyway to test for this, than assume that this is the problem?
There was a problem hiding this comment.
We just read it, it didn't exist; we try to create it, it does exist. Presumably somebody created it in between the read & write. Or we're using an inconsistent datastore ;-) The only thing we can do is to retry...
| return s, nil | ||
| } | ||
|
|
||
| // createSecret writes the secret, but only if it does not exists |
There was a problem hiding this comment.
Does not exist not exits?
| return fmt.Errorf("error parsing SSH public key: %v", err) | ||
| } | ||
|
|
||
| // TODO: Reintroduce or remove |
There was a problem hiding this comment.
What are we loosing by not having this?
There was a problem hiding this comment.
Unclear at the present time... we have nowhere to store it on the SSHCredential API type (previously they were on the Keyset API type)
Not yet wired in
justinsb
force-pushed
the
clientset_secret_stores
branch
from
d99927b
to
914fe68
Compare
|
Pushed fixes |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrislovecnm 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 |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/retest |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. . |
Not yet wired in