From 4d5386911d5f0ff163fce69027da583cf57d4134 Mon Sep 17 00:00:00 2001 From: Martin Weindel Date: Thu, 19 Sep 2019 15:22:38 +0200 Subject: [PATCH] move ACME user registration from issuer secret to issuer status; only delete certificate secrets if new flag --cascade-delete is set; delay certificate reconciliation if issuer is not ready, but != error ```improvement operator move ACME user registration from issuer secret to issuer status ``` ```improvement operator only delete certificate secrets if new flag --cascade-delete is set ``` ```improvement operator delay certificate reconciliation if issuer is not ready, but != error ``` --- README.md | 2 + pkg/apis/cert/v1alpha1/issuer.go | 10 +++-- .../cert/v1alpha1/zz_generated.deepcopy.go | 5 +++ pkg/cert/legobridge/certificate.go | 2 +- pkg/cert/legobridge/reguser.go | 24 +++++------- pkg/controller/issuer/acme/handler.go | 14 ++++--- .../issuer/certificate/reconciler.go | 38 ++++++++++++++++--- pkg/controller/issuer/controller.go | 1 + pkg/controller/issuer/core/const.go | 1 + pkg/controller/issuer/core/support.go | 24 ++++++++---- 10 files changed, 81 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 3e659125..e0434b8e 100644 --- a/README.md +++ b/README.md @@ -329,6 +329,7 @@ Usage: cert-controller-manager [flags] Flags: + --cascade-delete default for all controller "cascade-delete" options --cert-class string default for all controller "cert-class" options --cert-target-class string default for all controller "cert-target-class" options -c, --controllers string comma separated list of controllers to start (,source,target,all) (default "all") @@ -351,6 +352,7 @@ Flags: --ingress-cert.target-namespace string target namespace for cross cluster generation --ingress-cert.targets.pool.size int Worker pool size for pool targets of controller ingress-cert (default: 2) --issuer-namespace string default for all controller "issuer-namespace" options + --issuer.cascade-delete If true, certificate secrets are deleted if dependent resources (certificate, ingress) are deleted --issuer.cert-class string Identifier used to differentiate responsible controllers for entries --issuer.default-issuer string name of default issuer (from default cluster) --issuer.default-issuer-domain-ranges string domain range restrictions when using default issuer separated by comma diff --git a/pkg/apis/cert/v1alpha1/issuer.go b/pkg/apis/cert/v1alpha1/issuer.go index 89cd2ed7..f2439c1c 100644 --- a/pkg/apis/cert/v1alpha1/issuer.go +++ b/pkg/apis/cert/v1alpha1/issuer.go @@ -19,6 +19,7 @@ package v1alpha1 import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" ) // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -58,8 +59,9 @@ type ACMESpec struct { } type IssuerStatus struct { - ObservedGeneration int64 `json:"observedGeneration,omitempty"` - State string `json:"state"` - Message *string `json:"message,omitempty"` - Type *string `json:"type"` + ObservedGeneration int64 `json:"observedGeneration,omitempty"` + State string `json:"state"` + Message *string `json:"message,omitempty"` + Type *string `json:"type"` + ACME *runtime.RawExtension `json:"acme,omitempty"` } diff --git a/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go index f51853ea..e9f40b21 100644 --- a/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/cert/v1alpha1/zz_generated.deepcopy.go @@ -309,6 +309,11 @@ func (in *IssuerStatus) DeepCopyInto(out *IssuerStatus) { *out = new(string) **out = **in } + if in.ACME != nil { + in, out := &in.ACME, &out.ACME + *out = new(runtime.RawExtension) + (*in).DeepCopyInto(*out) + } return } diff --git a/pkg/cert/legobridge/certificate.go b/pkg/cert/legobridge/certificate.go index 816fe625..9e97d80b 100644 --- a/pkg/cert/legobridge/certificate.go +++ b/pkg/cert/legobridge/certificate.go @@ -105,7 +105,7 @@ func Obtain(input ObtainInput) error { if err != nil { return err } - nameservers := []string{"8.8.8.8", "8.8.4.4", "1.1.1.1"} + nameservers := []string{"8.8.8.8", "8.8.4.4"} err = client.Challenge.SetDNS01Provider(provider, dns01.AddRecursiveNameservers(dns01.ParseNameservers(nameservers))) if err != nil { return err diff --git a/pkg/cert/legobridge/reguser.go b/pkg/cert/legobridge/reguser.go index 3e7caf76..56b89e82 100644 --- a/pkg/cert/legobridge/reguser.go +++ b/pkg/cert/legobridge/reguser.go @@ -32,9 +32,8 @@ import ( ) const ( - KEY_EMAIL = "email" - KEY_REGISTRATION = "registration" - KEY_PRIVATE_KEY = "privateKey" + KEY_EMAIL = "email" + KEY_PRIVATE_KEY = "privateKey" ) type RegistrationUser struct { @@ -143,19 +142,18 @@ func (u *RegistrationUser) ToSecretData() (map[string][]byte, error) { if err != nil { return nil, fmt.Errorf("encoding private key failed: %s", err.Error()) } + return map[string][]byte{KEY_PRIVATE_KEY: privkey, KEY_EMAIL: []byte(u.Email)}, nil +} + +func (u *RegistrationUser) RawRegistration() ([]byte, error) { reg, err := json.Marshal(u.Registration) if err != nil { return nil, fmt.Errorf("encoding registration failed: %s", err.Error()) } - return map[string][]byte{KEY_PRIVATE_KEY: privkey, KEY_REGISTRATION: reg, KEY_EMAIL: []byte(u.Email)}, nil -} - -func SecretDataHasRegistration(data map[string][]byte) bool { - _, ok := data[KEY_REGISTRATION] - return ok + return reg, nil } -func RegistrationUserFromSecretData(data map[string][]byte) (*RegistrationUser, error) { +func RegistrationUserFromSecretData(registrationRaw []byte, data map[string][]byte) (*RegistrationUser, error) { privkeyBytes, ok := data[KEY_PRIVATE_KEY] if !ok { return nil, fmt.Errorf("`%s` data not found in secret", KEY_PRIVATE_KEY) @@ -165,12 +163,8 @@ func RegistrationUserFromSecretData(data map[string][]byte) (*RegistrationUser, return nil, err } - regBytes, ok := data[KEY_REGISTRATION] - if !ok { - return nil, fmt.Errorf("`%s` data not found in secret", KEY_REGISTRATION) - } reg := ®istration.Resource{} - err = json.Unmarshal(regBytes, reg) + err = json.Unmarshal(registrationRaw, reg) if err != nil { return nil, fmt.Errorf("unmarshalling registration json failed with %s", err.Error()) } diff --git a/pkg/controller/issuer/acme/handler.go b/pkg/controller/issuer/acme/handler.go index 0a5f8b9f..cfc29f0b 100644 --- a/pkg/controller/issuer/acme/handler.go +++ b/pkg/controller/issuer/acme/handler.go @@ -83,15 +83,15 @@ func (r *acmeIssuerHandler) Reconcile(logger logger.LogContext, obj resources.Ob hash := r.support.CalcSecretHash(secret) r.support.RememberIssuerSecret(obj.ObjectName(), issuer.Spec.ACME.PrivateKeySecretRef, hash) } - if secret != nil && legobridge.SecretDataHasRegistration(secret.Data) { - user, err := legobridge.RegistrationUserFromSecretData(secret.Data) + if secret != nil && issuer.Status.ACME != nil && issuer.Status.ACME.Raw != nil { + user, err := legobridge.RegistrationUserFromSecretData(issuer.Status.ACME.Raw, secret.Data) if err != nil { return r.failedAcme(logger, obj, api.STATE_ERROR, fmt.Errorf("extracting registration user from secret failed with %s", err.Error())) } if user.Email != acme.Email { return r.failedAcme(logger, obj, api.STATE_ERROR, fmt.Errorf("email of registration user from secret does not match %s != %s", user.Email, acme.Email)) } - return r.support.SucceededAndTriggerCertificates(logger, obj, &acmeType) + return r.support.SucceededAndTriggerCertificates(logger, obj, &acmeType, issuer.Status.ACME.Raw) } else if secret != nil || acme.AutoRegistration { var secretData map[string][]byte if secret != nil { @@ -117,14 +117,16 @@ func (r *acmeIssuerHandler) Reconcile(logger logger.LogContext, obj resources.Ob r.support.RememberIssuerSecret(obj.ObjectName(), issuer.Spec.ACME.PrivateKeySecretRef, hash) } - issuer.Status.State = api.STATE_READY - issuer.Status.Message = nil + regRaw, err := user.RawRegistration() + if err != nil { + return r.failedAcme(logger, obj, api.STATE_ERROR, fmt.Errorf("registration marshalling failed with %s", err.Error())) + } newObj, err := r.support.GetIssuerResources().Update(issuer) if err != nil { return r.failedAcme(logger, obj, api.STATE_ERROR, fmt.Errorf("updating resource failed with %s", err.Error())) } - return r.support.SucceededAndTriggerCertificates(logger, newObj, &acmeType) + return r.support.SucceededAndTriggerCertificates(logger, newObj, &acmeType, regRaw) } else { return r.failedAcme(logger, obj, api.STATE_ERROR, fmt.Errorf("neither `SecretRef` or `AutoRegistration: true` provided")) } diff --git a/pkg/controller/issuer/certificate/reconciler.go b/pkg/controller/issuer/certificate/reconciler.go index 298d642a..685848aa 100644 --- a/pkg/controller/issuer/certificate/reconciler.go +++ b/pkg/controller/issuer/certificate/reconciler.go @@ -86,6 +86,7 @@ func CertReconciler(c controller.Interface, support *core.Support) (reconcile.In if dnsOwnerId != "" { reconciler.dnsOwnerId = &dnsOwnerId } + reconciler.cascadeDelete, _ = c.GetBoolOption(core.OptCascadeDelete) renewalWindow, err := c.GetDurationOption(core.OptRenewalWindow) if err != nil { @@ -96,6 +97,19 @@ func CertReconciler(c controller.Interface, support *core.Support) (reconcile.In return reconciler, nil } +type recoverableError struct { + Msg string +} + +func (err *recoverableError) Error() string { + return err.Msg +} + +func isRecoverableError(err error) bool { + _, ok := err.(*recoverableError) + return ok +} + type certReconciler struct { reconcile.DefaultReconciler support *core.Support @@ -111,6 +125,7 @@ type certReconciler struct { renewalWindow time.Duration renewalCheckPeriod time.Duration classes *controller.Classes + cascadeDelete bool } func (r *certReconciler) Start() { @@ -286,8 +301,14 @@ func (r *certReconciler) restoreRegUser(crt *api.Certificate) (*legobridge.Regis return nil, "", fmt.Errorf("missing secret ref in issuer") } if issuer.Status.State != api.STATE_READY { + if issuer.Status.State != api.STATE_ERROR { + return nil, "", &recoverableError{fmt.Sprintf("referenced issuer not ready: state=%s", issuer.Status.State)} + } return nil, "", fmt.Errorf("referenced issuer not ready: state=%s", issuer.Status.State) } + if issuer.Status.ACME == nil || issuer.Status.ACME.Raw == nil { + return nil, "", fmt.Errorf("ACME registration missing in status") + } issuerSecretObjectName := resources.NewObjectName(secretRef.Namespace, secretRef.Name) issuerSecret := &corev1.Secret{} _, err = r.support.GetIssuerSecretResources().GetInto(issuerSecretObjectName, issuerSecret) @@ -295,7 +316,7 @@ func (r *certReconciler) restoreRegUser(crt *api.Certificate) (*legobridge.Regis return nil, "", fmt.Errorf("fetching issuer secret failed with %s", err.Error()) } - reguser, err := legobridge.RegistrationUserFromSecretData(issuerSecret.Data) + reguser, err := legobridge.RegistrationUserFromSecretData(issuer.Status.ACME.Raw, issuerSecret.Data) if err != nil { return nil, "", fmt.Errorf("restoring registration issuer from issuer secret failed with %s", err.Error()) } @@ -489,13 +510,15 @@ func (r *certReconciler) writeCertificateSecret(objectMeta metav1.ObjectMeta, ce } secret.Labels = map[string]string{LabelCertificateHashKey: specHash, LabelCertificateKey: "true"} secret.Data = legobridge.CertificatesToSecretData(certificates) - ownerReferences := []metav1.OwnerReference{{APIVersion: api.Version, Kind: api.CertificateKind, Name: objectMeta.GetName(), UID: objectMeta.GetUID()}} - if objectMeta.GetAnnotations() != nil && objectMeta.GetAnnotations()[source.ANNOT_FORWARD_OWNER_REFS] == "true" { - if objectMeta.OwnerReferences != nil { - ownerReferences = append(ownerReferences, objectMeta.OwnerReferences...) + if r.cascadeDelete { + ownerReferences := []metav1.OwnerReference{{APIVersion: api.Version, Kind: api.CertificateKind, Name: objectMeta.GetName(), UID: objectMeta.GetUID()}} + if objectMeta.GetAnnotations() != nil && objectMeta.GetAnnotations()[source.ANNOT_FORWARD_OWNER_REFS] == "true" { + if objectMeta.OwnerReferences != nil { + ownerReferences = append(ownerReferences, objectMeta.OwnerReferences...) + } } + secret.SetOwnerReferences(ownerReferences) } - secret.SetOwnerReferences(ownerReferences) obj, err := r.targetCluster.Resources().CreateOrUpdateObject(secret) if err != nil { @@ -585,6 +608,9 @@ func (r *certReconciler) failed(logger logger.LogContext, obj resources.Object, mod, _ := r.prepareUpdateStatus(obj, state, &msg) r.updateStatus(mod) + if isRecoverableError(err) { + return reconcile.Delay(logger, err) + } return reconcile.Failed(logger, err) } diff --git a/pkg/controller/issuer/controller.go b/pkg/controller/issuer/controller.go index afad79d5..23cfa10c 100644 --- a/pkg/controller/issuer/controller.go +++ b/pkg/controller/issuer/controller.go @@ -38,6 +38,7 @@ func init() { StringOption(core.OptDefaultIssuerDomainRanges, "domain range restrictions when using default issuer separated by comma"). StringOption(core.OptDNSNamespace, "namespace for creating challenge DNSEntries (in DNS cluster)"). StringOption(core.OptDNSOwnerId, "ownerId for creating challenge DNSEntries"). + BoolOption(core.OptCascadeDelete, "If true, certificate secrets are deleted if dependent resources (certificate, ingress) are deleted"). StringOption(source.OPT_CLASS, "Identifier used to differentiate responsible controllers for entries"). DefaultedDurationOption(core.OptRenewalWindow, 30*24*time.Hour, "certificate is renewed if its validity period is shorter"). FinalizerDomain(cert.GroupName). diff --git a/pkg/controller/issuer/core/const.go b/pkg/controller/issuer/core/const.go index a5025e2a..e4307fc4 100644 --- a/pkg/controller/issuer/core/const.go +++ b/pkg/controller/issuer/core/const.go @@ -23,4 +23,5 @@ const ( OptDNSOwnerId = "dns-owner-id" OptDefaultIssuerDomainRanges = "default-issuer-domain-ranges" OptRenewalWindow = "renewal-window" + OptCascadeDelete = "cascade-delete" ) diff --git a/pkg/controller/issuer/core/support.go b/pkg/controller/issuer/core/support.go index a9619f7e..7786a555 100644 --- a/pkg/controller/issuer/core/support.go +++ b/pkg/controller/issuer/core/support.go @@ -17,13 +17,14 @@ package core import ( + "bytes" "crypto/sha256" "fmt" - "sort" - "strings" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sort" + "strings" api "github.com/gardener/cert-management/pkg/apis/cert/v1alpha1" "github.com/gardener/cert-management/pkg/cert/legobridge" @@ -274,13 +275,20 @@ func (s *Support) Failed(logger logger.LogContext, obj resources.Object, state s return reconcile.Failed(logger, err) } -func (s *Support) SucceededAndTriggerCertificates(logger logger.LogContext, obj resources.Object, itype *string) reconcile.Status { +func (s *Support) SucceededAndTriggerCertificates(logger logger.LogContext, obj resources.Object, itype *string, regRaw []byte) reconcile.Status { s.triggerCertificates(logger, obj.ObjectName()) - return s.Succeeded(logger, obj, itype, nil) -} -func (s *Support) Succeeded(logger logger.LogContext, obj resources.Object, itype *string, msg *string) reconcile.Status { - mod, _ := s.prepareUpdateStatus(obj, api.STATE_READY, itype, msg) + mod, status := s.prepareUpdateStatus(obj, api.STATE_READY, itype, nil) + changedRegistration := false + if status.ACME == nil || status.ACME.Raw == nil { + changedRegistration = regRaw != nil + } else { + changedRegistration = !bytes.Equal(status.ACME.Raw, regRaw) + } + if changedRegistration { + status.ACME = &runtime.RawExtension{Raw: regRaw} + mod.Modify(true) + } s.updateStatus(mod) return reconcile.Succeeded(logger)