Skip to content

Commit

Permalink
Implement feedback from code review
Browse files Browse the repository at this point in the history 
Signed-off-by: irbekrm <irbekrm@gmail.com>
  • Loading branch information
@irbekrm
irbekrm committed May 14, 2021
1 parent e82ea35 commit 9ecf896
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 68 deletions.
14 changes: 11 additions & 3 deletions pkg/issuer/acme/setup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ const (
messageTemplateNotRSA = "ACME private key in %q is not of type RSA"
messageTemplateFailedToParseURL = "Failed to parse existing ACME server URI %q: %v"
messageTemplateFailedToParseAccountURL = "Failed to parse existing ACME account URI %q: %v"
messageTemplateFailedToGetEABKey = "failed to get External Account Binding key from secret: %v"
)

// Setup will verify an existing ACME registration, or create one if not
Expand All @@ -86,7 +87,7 @@ func (a *Acme) Setup(ctx context.Context) error {
if newURL, ok := acmev1ToV2Mappings[a.issuer.GetSpec().ACME.Server]; ok {
reason = errorInvalidConfig
msg = fmt.Sprintf(messageTemplateUpdateToV2, a.issuer.GetSpec().ACME.Server, newURL)
// return nil so that Setup only gets called again after the spec is updated
// Return nil, because we do not want to re-queue an Issuer with an invalid spec.
return nil
}

Expand Down Expand Up @@ -118,11 +119,14 @@ func (a *Acme) Setup(ctx context.Context) error {
a.issuer.GetStatus().ACMEStatus().URI = ""

case a.issuer.GetSpec().ACME.DisableAccountKeyGeneration && apierrors.IsNotFound(err):
wrapErr := fmt.Errorf("%s%s%w", messageAccountVerificationFailed,
wrapErr := fmt.Errorf("%s%s%v", messageAccountVerificationFailed,
messageNoSecretKeyGenerationDisabled,
err)
reason = errorAccountVerificationFailed
msg = wrapErr.Error()
// TODO: we should not re-queue the Issuer here as a resync will happen
// when the user adds the Secret or changes Issuer's spec. Should be
// fixed by https://github.com/jetstack/cert-manager/issues/4004
return wrapErr

case errors.IsInvalidData(err):
Expand All @@ -146,6 +150,10 @@ func (a *Acme) Setup(ctx context.Context) error {
// TODO: don't always clear the client cache.
// In future we should intelligently manage items in the account cache
// and remove them when the corresponding issuer is updated/deleted.
// TODO: if we fail earlier, the issuer is considered not ready and we
// probably don't want other controllers to use its client from the cache.
// We could therefore move the removing of the client up to the start of
// this function.
a.accountRegistry.RemoveClient(string(a.issuer.GetUID()))
httpClient := accounts.BuildHTTPClient(a.metrics, a.issuer.GetSpec().ACME.SkipTLSVerify)
cl := a.clientBuilder(httpClient, *a.issuer.GetSpec().ACME, rsaPk)
Expand Down Expand Up @@ -382,7 +390,7 @@ func (a *Acme) getEABKey(ctx context.Context, ns string) ([]byte, error) {
}

if err != nil {
return nil, fmt.Errorf("failed to get External Account Binding key from secret: %s", err)
return nil, fmt.Errorf(messageTemplateFailedToGetEABKey, err)
}

encodedKeyData, ok := sec.Data[eab.Key]
Expand Down
137 changes: 72 additions & 65 deletions pkg/issuer/acme/setup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
apiutil "github.com/jetstack/cert-manager/pkg/api/util"
cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1"
cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1"
v1 "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1"
controllertest "github.com/jetstack/cert-manager/pkg/controller/test"
"github.com/jetstack/cert-manager/pkg/util"
Expand All @@ -57,10 +56,15 @@ func TestAcme_Setup(t *testing.T) {

baseIssuer = gen.Issuer("test-issuer",
gen.SetIssuerACMEURL(acmev2Prod))
// base issuer condition
baseCondition = gen.IssuerCondition(v1.IssuerConditionReady,
// base issuer's conditions
readyFalseCondition = gen.IssuerCondition(cmapi.IssuerConditionReady,
gen.SetIssuerConditionStatus(cmmeta.ConditionFalse),
gen.SetIssuerConditionLastTransitionTime(&nowMetaTime))
readyTrueCondition = gen.IssuerCondition(cmapi.IssuerConditionReady,
gen.SetIssuerConditionStatus(cmmeta.ConditionTrue),
gen.SetIssuerConditionReason(successAccountRegistered),
gen.SetIssuerConditionMessage(messageAccountRegistered),
gen.SetIssuerConditionLastTransitionTime(&nowMetaTime))
issuerSecretKeyName = "test"

ecdsaPrivKey = mustGenerateEDCSAKey(t)
Expand Down Expand Up @@ -97,14 +101,12 @@ func TestAcme_Setup(t *testing.T) {
)

tests := map[string]struct {
issuer v1.GenericIssuer
issuer cmapi.GenericIssuer

// Private key returned by keyFromSecret stub.
kfsKey crypto.Signer
// Error returned by keyFromSecret stub.
kfsErr error
// Whether keyFromSecret should be called.
kfsShouldBeCalled bool

// Whether RemoveClient should be called.
removeClientShouldBeCalled bool
Expand All @@ -120,7 +122,10 @@ func TestAcme_Setup(t *testing.T) {
// Error returned by cl.GetReg
getRegErr error

// Error returned when creating ACME account key.
acmePrivKeySecretCreateErr error
// ACME account key created by createAccountPrivateKey.
acmePrivKey *rsa.PrivateKey

eabSecret *corev1.Secret
eabSecretGetErr error
Expand All @@ -136,7 +141,7 @@ func TestAcme_Setup(t *testing.T) {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEURL(acmev1Prod)),
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorInvalidConfig),
gen.SetIssuerConditionMessage(fmt.Sprintf(messageTemplateUpdateToV2, acmev1Prod, acmev2Prod))),
},
Expand All @@ -145,19 +150,18 @@ func TestAcme_Setup(t *testing.T) {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEURL(fmt.Sprintf("%s/", acmev1Staging))),
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorInvalidConfig),
gen.SetIssuerConditionMessage(fmt.Sprintf(messageTemplateUpdateToV2, fmt.Sprintf("%s/", acmev1Staging), acmev2Staging))),
},
},
"ACME private key secret does not exist, account key generation not disabled, key secret creation fails": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEPrivKeyRef(issuerSecretKeyName)),
kfsShouldBeCalled: true,
kfsErr: notFoundErr,
acmePrivKeySecretCreateErr: someErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+someErr.Error())),
},
Expand All @@ -167,31 +171,38 @@ func TestAcme_Setup(t *testing.T) {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEDisableAccountKeyGeneration(true),
),
kfsShouldBeCalled: true,
kfsErr: notFoundErr,
kfsErr: notFoundErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountVerificationFailed),
gen.SetIssuerConditionMessage(messageAccountVerificationFailed+messageNoSecretKeyGenerationDisabled+notFoundErr.Error())),
},
wantsErr: true,
},
"ACME private key secret does not exist, account key generation is enabled, key creation succeeds": {
issuer: gen.IssuerFrom(baseIssuer),
kfsErr: notFoundErr,
acmePrivKey: rsaPrivKey.(*rsa.PrivateKey),
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(readyTrueCondition)},
removeClientShouldBeCalled: true,
addClientShouldBeCalled: true,
expectedRegisteredAcc: &acmeapi.Account{},
},
"ACME private key secret exists, but contains invalid private key": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsErr: invalidDataErr,
issuer: gen.IssuerFrom(baseIssuer),
kfsErr: invalidDataErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountVerificationFailed),
gen.SetIssuerConditionMessage(fmt.Sprintf("%s%v", messageInvalidPrivateKey, invalidDataErr))),
},
},
"Checking ACME private key secret fails with an unknown error": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsErr: someErr,
issuer: gen.IssuerFrom(baseIssuer),
kfsErr: someErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountVerificationFailed),
gen.SetIssuerConditionMessage(messageAccountVerificationFailed+someErr.Error())),
},
Expand All @@ -200,22 +211,20 @@ func TestAcme_Setup(t *testing.T) {
"ACME account's key is not an RSA key": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEPrivKeyRef(issuerSecretKeyName)),
kfsShouldBeCalled: true,
kfsKey: ecdsaPrivKey,
kfsKey: ecdsaPrivKey,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountVerificationFailed),
gen.SetIssuerConditionMessage(fmt.Sprintf(messageTemplateNotRSA, issuerSecretKeyName))),
},
},
"ACME server URL is an invalid URL": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEURL(invalidURL)),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorInvalidURL),
gen.SetIssuerConditionMessage(invalidURLMessage)),
},
Expand All @@ -225,11 +234,10 @@ func TestAcme_Setup(t *testing.T) {
"ACME account URL is an invalid URL": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEAccountURL(invalidURL)),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorInvalidURL),
gen.SetIssuerConditionMessage(invalidAccountURLMessage)),
},
Expand All @@ -243,95 +251,101 @@ func TestAcme_Setup(t *testing.T) {
gen.SetIssuerACMEEmail(someEmail),
gen.SetIssuerACMELastRegisteredEmail(someEmail),
gen.AddIssuerCondition(
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyTrueCondition,
gen.SetIssuerConditionStatus(cmmeta.ConditionTrue)))),
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyTrueCondition,
gen.SetIssuerConditionStatus(cmmeta.ConditionTrue),
gen.SetIssuerConditionMessage(messageAccountRegistered),
gen.SetIssuerConditionReason(successAccountRegistered)),
},
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
addClientShouldBeCalled: true,
},
"EAB for issuer specified, but the corresponding secret is not found": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEEAB(someString, someString)),
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
eabSecretGetErr: notFoundErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+notFoundErr.Error())),
},
expectedEvents: []string{
fmt.Sprintf("%s %s %s", corev1.EventTypeWarning, errorAccountRegistrationFailed, messageAccountRegistrationFailed+notFoundErr.Error()),
},
},
"EAB for issuer specified, attempting to retrieve secret fails with unknown error": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEEAB(someString, someString)),
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
eabSecretGetErr: someErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+fmt.Sprintf(messageTemplateFailedToGetEABKey, someErr))),
},
wantsErr: true,
},
"Attempt to register ACME account returns unknown error": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
registerErr: someErr,
expectedRegisteredAcc: &acmeapi.Account{},
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+someErr.Error())),
},
wantsErr: true,
},
"Attempt to register ACME account returns an ACME error in range [400,500)": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
expectedRegisteredAcc: &acmeapi.Account{},
registerErr: acmeErr450,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+acmeErr450.Error())),
},
},
"Attempt to register ACME account returns an ACME error outside of range [400,500)": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
expectedRegisteredAcc: &acmeapi.Account{},
registerErr: acmeErr500,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+acmeErr500.Error())),
},
wantsErr: true,
},
"ACME account already exists, attempting to retrieve it fails with unknown error": {
issuer: gen.IssuerFrom(baseIssuer),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
expectedRegisteredAcc: &acmeapi.Account{},
registerErr: acmeapi.ErrAccountAlreadyExists,
getRegErr: someErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyFalseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+someErr.Error())),
},
wantsErr: true,
},
"EAB for issuer specified, but the corresponding secret is not found": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEEAB(someString, someString)),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
eabSecretGetErr: notFoundErr,
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
gen.SetIssuerConditionReason(errorAccountRegistrationFailed),
gen.SetIssuerConditionMessage(messageAccountRegistrationFailed+notFoundErr.Error())),
},
expectedEvents: []string{
fmt.Sprintf("%s %s %s", corev1.EventTypeWarning, errorAccountRegistrationFailed, messageAccountRegistrationFailed+notFoundErr.Error()),
},
},
"ACME account with EAB registered successfully": {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEEAB(someString, someString)),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
addClientShouldBeCalled: true,
Expand All @@ -341,7 +355,7 @@ func TestAcme_Setup(t *testing.T) {
Key: []byte(eabKey),
}},
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyTrueCondition,
gen.SetIssuerConditionStatus(cmmeta.ConditionTrue),
gen.SetIssuerConditionReason(successAccountRegistered),
gen.SetIssuerConditionMessage(messageAccountRegistered)),
Expand All @@ -351,7 +365,6 @@ func TestAcme_Setup(t *testing.T) {
issuer: gen.IssuerFrom(baseIssuer,
gen.SetIssuerACMEEmail(someEmail),
gen.SetIssuerACMEEABWithKeyAlgorithm(someString, someString, cmacme.HS256)),
kfsShouldBeCalled: true,
kfsKey: rsaPrivKey,
removeClientShouldBeCalled: true,
addClientShouldBeCalled: true,
Expand All @@ -363,7 +376,7 @@ func TestAcme_Setup(t *testing.T) {
Contact: []string{someEmailURL},
},
expectedConditions: []cmapi.IssuerCondition{
*gen.IssuerConditionFrom(baseCondition,
*gen.IssuerConditionFrom(readyTrueCondition,
gen.SetIssuerConditionStatus(cmmeta.ConditionTrue),
gen.SetIssuerConditionReason(successAccountRegistered),
gen.SetIssuerConditionMessage(messageAccountRegistered)),
Expand Down Expand Up @@ -436,12 +449,6 @@ func TestAcme_Setup(t *testing.T) {
t.Errorf("Expected error %v, got %v", test.wantsErr, gotErr)
}

if kfsWasCalled != test.kfsShouldBeCalled {
t.Errorf("Expected Acme.keyFromSecret to be called: %v, was called: %v",
test.kfsShouldBeCalled,
kfsWasCalled)
}

// Verify that a client was removed from cache if expected.
if removeClientWasCalled != test.removeClientShouldBeCalled {
t.Errorf("Expected Acme.accountsRegistry.RemoveClient to be called: %v, was called: %v",
Expand Down

0 comments on commit 9ecf896

Please sign in to comment.