Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extracting Let's Encrypt to a client interface #61

Merged
merged 13 commits into from Aug 15, 2019

Conversation

@c-e-brumm
Copy link
Contributor

commented Aug 5, 2019

This will help with dependency injection and creating mock clients.

@app-sre-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

Can one of the admins verify this patch?

@openshift-ci-robot openshift-ci-robot added size/XL and removed size/L labels Aug 9, 2019

@openshift-ci-robot openshift-ci-robot added size/L and removed size/XL labels Aug 12, 2019

@c-e-brumm c-e-brumm marked this pull request as ready for review Aug 12, 2019

@c-e-brumm c-e-brumm changed the title WIP: Extracting Let's Encrypt to a client interface Extracting Let's Encrypt to a client interface Aug 12, 2019

@dak1n1 dak1n1 self-assigned this Aug 12, 2019

return nil
}

func (c *ACMEClient) GetAccount(kubeClient client.Client, staging bool, namespace string) (err error) {

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

If the staging setting is set at the client level, why does it need to be specifically passed into this function? Can't it be consumed from the client? Perhaps add a new setting to the ACMEClient type?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 12, 2019

Author Contributor

This is legacy logic that I wasn't trying to remove in this PR. But you're right, there is absolutely no need for staging vs production endpoints in the operator logic. It needs to just be config data supplied to the operator (endpoint, account name, keys, etc). That will be fixed in a future PR.

This comment has been minimized.

Copy link
@dofinn

dofinn Aug 13, 2019

Are tasks like this getting managed via Jira?

This comment has been minimized.

Copy link
@dofinn

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 13, 2019

Author Contributor

All certman-operator stories, https://jira.coreos.com/browse/SDE-109
This item specifically, https://jira.coreos.com/browse/SREP-1802

}

func (c *ACMEClient) GetAccount(kubeClient client.Client, staging bool, namespace string) (err error) {
accountURL, err := getLetsEncryptAccountURL(kubeClient, true)

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Staging is hard coded to true?

return err
}

privateKey, err := getLetsEncryptAccountPrivateKey(kubeClient, true)

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Staging is hard coded to true?


package leclient

const (

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

These are all exported. Do they need to be?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 12, 2019

Author Contributor

No, this is actually lifted straight from the package certificaterequest constants, where it originated, to get the pieces I needed. I forgot to curate these, good catch.


func getLetsEncryptAccountPrivateKey(kubeClient client.Client, staging bool) (privateKey crypto.Signer, err error) {

secretName := LetsEncryptProductionAccountSecretName

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Question: Do we expect an install of this operator to have both staging and prod? Is there a reason to have two different secrets in the same NS?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 12, 2019

Author Contributor

No, it will never do both, which is why it needs to be streamlined out of the code. That's coming in another story/PR.

challenge, ok := authorization.ChallengeMap[acme.ChallengeTypeDNS01]
if !ok {
domain := leClient.GetAuthorizationIndentifier()
leClient.SetChallengeType()

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Uncaught error handling

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 12, 2019

Author Contributor

Neither SetChallengeType or GetAuthorizationIndentifier should be able to fail but I've added error outputs and stanzas just in case in the future they can. Or in case future implementations can fail.

return fmt.Errorf("cloud not find DNS challenge authorization")
}

encodeDNS01KeyAuthorization := acme.EncodeDNS01KeyAuthorization(challenge.KeyAuthorization)
DNS01KeyAuthorization := leClient.GetDNS01KeyAuthorization()

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Is it possible for this response to be nil?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 13, 2019

Author Contributor

No, but check added in case.


challenge, ok := authorization.ChallengeMap[acme.ChallengeTypeDNS01]
if !ok {
domain := leClient.GetAuthorizationIndentifier()

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Is it possible for this response to be nil?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 13, 2019

Author Contributor

Do not believe so. But I'll handling just in case.

if err != nil {
return err
}
URL, _ := leClient.GetOrderURL()

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

Why are we discarding the err here?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 13, 2019

Author Contributor

The left side is unchanged from before. I've fixed this and added handling.

@@ -42,8 +42,7 @@ func (r *ReconcileCertificateRequest) updateStatus(reqLogger logr.Logger, cr *ce
cr.Status.IssuerName != certificate.Issuer.CommonName ||
cr.Status.NotBefore != certificate.NotBefore.String() ||
cr.Status.NotAfter != certificate.NotAfter.String() ||
cr.Status.SerialNumber != certificate.SerialNumber.String() ||
cr.Status.Status != "Success" {

This comment has been minimized.

Copy link
@cblecker

cblecker Aug 12, 2019

Member

This seems unrelated to the rest of the PR. Was this intended?

This comment has been minimized.

Copy link
@c-e-brumm

c-e-brumm Aug 12, 2019

Author Contributor

Not related but I accidentally committed bug fix to my feature branch. This removes always catching a non "Status: Success" and setting it to "Success".

@jharrington22
Copy link
Collaborator

left a comment

Agree with Christoph's comments but otherwise lgtm.

@openshift-ci-robot openshift-ci-robot added size/XL and removed size/L labels Aug 13, 2019

@cblecker

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

[test]

@cblecker

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

/lgtm
/approve
/hold

Please feel free to remove hold (/hold cancel) when you are ready to merge.

@openshift-ci-robot

This comment has been minimized.

Copy link

commented Aug 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: c-e-brumm, cblecker, jharrington22

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:

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

@c-e-brumm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 4eb5963 into openshift:master Aug 15, 2019

3 checks passed

ci.ext.devshift.net PR build
Details
ci/prow/images Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.