Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

kube-client-agent: Build kubeconfig client with empty ConfigOverrides #21

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Nov 15, 2018

Fixes #20

@csrwng
Copy link
Contributor Author

csrwng commented Nov 15, 2018

@abhinavdahiya this fixes the client creation for me. Still not sure why BuildConfigFromFlags is not working in this case.

I also skip returning an error when the CSR creation fails because it already exists. Otherwise, if it times out waiting for approval, subsequent runs will never succeed.

@@ -108,7 +118,7 @@ func (c *CertAgent) RequestCertificate() error {
}

// send CSR to the signer
if _, err := c.client.Create(csr); err != nil {
if _, err := c.client.Create(csr); err != nil && !errors.IsAlreadyExists(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this will cause the client to accept the certificate from old csr request but GenerateCSRObject has replaced the private key on disk with new one...

we should't generate if csr with the expected name already exists.
@csrwng

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abhinavdahiya I see, you're right.
We need to fix the flow though. In the case that we time out waiting for the certificate to be signed, we will fail and try again, generating a new private key, but will never be able to create another CSR because one already exists. Should the CSR name be randomized? (as in using GenerateName?)

@ericavonb
Copy link
Contributor

cc @mrogers590

@csrwng
Copy link
Contributor Author

csrwng commented Dec 12, 2018

@abhinavdahiya removed the part that adds the IsAlreadyExists check, only leaves the change to how the client is created.

@abhinavdahiya abhinavdahiya merged commit 52278c2 into coreos:master Dec 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants