diff --git a/Gopkg.lock b/Gopkg.lock index c69791f6d..08d9a102e 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -872,6 +872,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "0950e8204b2b6dcc7641243817171611f11d58ef6d3f59b42160fa10389249d9" + inputs-digest = "66c54bf5feaa6d84fc8b6f1cc8052c55cf1f3a7dbfdacbaca277b3b08b1fb865" solver-name = "gps-cdcl" solver-version = 1 diff --git a/pkg/apis/certmanager/validation/certificate.go b/pkg/apis/certmanager/validation/certificate.go new file mode 100644 index 000000000..5c2512549 --- /dev/null +++ b/pkg/apis/certmanager/validation/certificate.go @@ -0,0 +1,123 @@ +package validation + +import ( + "fmt" + + "github.com/golang/glog" + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" +) + +// Validation functions for cert-manager v1alpha1 Certificate types + +func ValidateCertificate(crt *v1alpha1.Certificate) field.ErrorList { + allErrs := ValidateCertificateSpec(&crt.Spec, field.NewPath("spec")) + return allErrs +} + +func ValidateCertificateSpec(crt *v1alpha1.CertificateSpec, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if crt.SecretName == "" { + el = append(el, field.Required(fldPath.Child("secretName"), "must be specified")) + } + issuerRefPath := fldPath.Child("issuerRef") + if crt.IssuerRef.Name == "" { + el = append(el, field.Required(issuerRefPath.Child("name"), "must be specified")) + } + switch crt.IssuerRef.Kind { + case "": + // For now we disable this check in order to support older versions where + // defaulting doesn't occur + glog.Infof("Certificate does not set issuerRef.kind - " + + "in future versions of cert-manager, this will be a hard failure.") + // el = append(el, field.Required(issuerRefPath.Child("kind"), "must be specified")) + case "Issuer", "ClusterIssuer": + default: + el = append(el, field.Invalid(issuerRefPath.Child("kind"), crt.IssuerRef.Kind, "must be one of Issuer or ClusterIssuer")) + } + if len(crt.CommonName) == 0 && len(crt.DNSNames) == 0 { + el = append(el, field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set")) + } + if crt.ACME != nil { + el = append(el, validateACMEConfigForAllDNSNames(crt, fldPath)...) + el = append(el, ValidateACMECertificateConfig(crt.ACME, fldPath.Child("acme"))...) + } + return el +} + +// validateACMEConfigForAllDNSNames will ensure that if the provided Certificate +// specifies any ACME configuration, all domains listed on the Certificate have +// a configuration entry. +func validateACMEConfigForAllDNSNames(a *v1alpha1.CertificateSpec, fldPath *field.Path) field.ErrorList { + if a.ACME == nil { + return nil + } + el := field.ErrorList{} + acmeFldPath := fldPath.Child("acme") + errFn := func(s string) string { + return fmt.Sprintf("no ACME solver configuration specified for domain %q", s) + } + if a.CommonName != "" { + cfg := a.ACME.ConfigForDomain(a.CommonName) + if cfg == nil || len(cfg.Domains) == 0 { + el = append(el, field.Required(acmeFldPath.Child("config"), errFn(a.CommonName))) + } + } + for _, d := range a.DNSNames { + cfg := a.ACME.ConfigForDomain(d) + if cfg == nil || len(cfg.Domains) == 0 { + el = append(el, field.Required(acmeFldPath.Child("config"), errFn(d))) + } + } + return el +} + +func ValidateACMECertificateConfig(a *v1alpha1.ACMECertificateConfig, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + for i, cfg := range a.Config { + el = append(el, ValidateACMECertificateDomainConfig(&cfg, fldPath.Child("config").Index(i))...) + } + return el +} + +func ValidateACMECertificateDomainConfig(a *v1alpha1.ACMECertificateDomainConfig, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if len(a.Domains) == 0 { + el = append(el, field.Required(fldPath.Child("domains"), "at least one domain must be specified")) + } + numTypes := 0 + if a.DNS01 != nil { + numTypes++ + el = append(el, ValidateACMECertificateDNS01Config(a.DNS01, fldPath.Child("dns01"))...) + } + if a.HTTP01 != nil { + if numTypes > 0 { + el = append(el, field.Forbidden(fldPath.Child("http01"), "may not specify more than one solver type")) + } else { + numTypes++ + el = append(el, ValidateACMECertificateHTTP01Config(a.HTTP01, fldPath.Child("http01"))...) + } + } + if numTypes == 0 { + el = append(el, field.Required(fldPath, "at least one solver must be configured")) + } + return el +} + +func ValidateACMECertificateDNS01Config(a *v1alpha1.ACMECertificateDNS01Config, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if a.Provider == "" { + el = append(el, field.Required(fldPath.Child("provider"), "provider name must be set")) + } + return el +} + +func ValidateACMECertificateHTTP01Config(a *v1alpha1.ACMECertificateHTTP01Config, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if a.Ingress != "" && a.IngressClass != nil { + el = append(el, field.Forbidden(fldPath, "only one of 'ingress' and 'ingressClass' should be specified")) + } + // TODO: ensure 'ingress' is a valid resource name (i.e. DNS name) + return el +} diff --git a/pkg/apis/certmanager/validation/certificate_test.go b/pkg/apis/certmanager/validation/certificate_test.go new file mode 100644 index 000000000..a6230d939 --- /dev/null +++ b/pkg/apis/certmanager/validation/certificate_test.go @@ -0,0 +1,357 @@ +package validation + +import ( + "reflect" + "testing" + + "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +var ( + validIssuerRef = v1alpha1.ObjectReference{ + Name: "name", + Kind: "ClusterIssuer", + } +) + +func strPtr(s string) *string { + return &s +} + +func TestValidateCertificate(t *testing.T) { + fldPath := field.NewPath("spec") + scenarios := map[string]struct { + cfg *v1alpha1.Certificate + errs []*field.Error + }{ + "valid basic certificate": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + }, + "valid with blank issuerRef kind": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + SecretName: "abc", + IssuerRef: v1alpha1.ObjectReference{ + Name: "valid", + }, + }, + }, + }, + "valid with 'Issuer' issuerRef kind": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + SecretName: "abc", + IssuerRef: v1alpha1.ObjectReference{ + Name: "valid", + Kind: "Issuer", + }, + }, + }, + }, + "invalid issuerRef kind": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + SecretName: "abc", + IssuerRef: v1alpha1.ObjectReference{ + Name: "valid", + Kind: "invalid", + }, + }, + }, + errs: []*field.Error{ + field.Invalid(fldPath.Child("issuerRef", "kind"), "invalid", "must be one of Issuer or ClusterIssuer"), + }, + }, + "certificate missing secretName": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("secretName"), "must be specified"), + }, + }, + "certificate with no domains": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("dnsNames"), "at least one dnsName is required if commonName is not set"), + }, + }, + "certificate with no issuerRef": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "testcn", + SecretName: "abc", + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("issuerRef", "name"), "must be specified"), + }, + }, + "valid certificate with only dnsNames": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"validdnsname"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + }, + }, + }, + "valid acme certificate": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"validdnsname"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"validdnsname"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + }, + }, + }, + "acme certificate with missing solver configuration for dns name": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"validdnsname", "anotherdnsname"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"validdnsname"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("acme", "config"), "no ACME solver configuration specified for domain \"anotherdnsname\""), + }, + }, + "acme certificate with missing solver configuration for common name": { + cfg: &v1alpha1.Certificate{ + Spec: v1alpha1.CertificateSpec{ + CommonName: "commonname", + DNSNames: []string{"validdnsname"}, + SecretName: "abc", + IssuerRef: validIssuerRef, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"validdnsname"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("acme", "config"), "no ACME solver configuration specified for domain \"commonname\""), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateCertificate(s.cfg) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} +func TestValidateACMECertificateConfig(t *testing.T) { + fldPath := field.NewPath("") + scenarios := map[string]struct { + isExpectedFailure bool + cfg *v1alpha1.ACMECertificateConfig + errs []*field.Error + }{ + "valid acme configuration": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"abc.xyz"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + }, + "acme configuration missing for domain": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"abc.xyz"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{}, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("config").Index(0), "at least one solver must be configured"), + }, + }, + "acme dns01 configuration missing provider name": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"abc.xyz"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + DNS01: &v1alpha1.ACMECertificateDNS01Config{}, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("config").Index(0).Child("dns01", "provider"), "provider name must be set"), + }, + }, + "valid acme dns01 configuration": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"abc.xyz"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + DNS01: &v1alpha1.ACMECertificateDNS01Config{ + Provider: "abc", + }, + }, + }, + }, + }, + errs: []*field.Error{}, + }, + "no domains specified": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("config").Index(0).Child("domains"), "at least one domain must be specified"), + }, + }, + "multiple solvers configured": { + cfg: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"abc.xyz"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + DNS01: &v1alpha1.ACMECertificateDNS01Config{ + Provider: "abc", + }, + }, + }, + }, + }, + errs: []*field.Error{ + field.Forbidden(fldPath.Child("config").Index(0).Child("http01"), "may not specify more than one solver type"), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateACMECertificateConfig(s.cfg, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} + +func TestValidateACMECertificateHTTP01Config(t *testing.T) { + fldPath := field.NewPath("") + scenarios := map[string]struct { + isExpectedFailure bool + cfg *v1alpha1.ACMECertificateHTTP01Config + errs []*field.Error + }{ + "ingress field specified": { + cfg: &v1alpha1.ACMECertificateHTTP01Config{ + Ingress: "abc", + }, + }, + "ingress class field specified": { + cfg: &v1alpha1.ACMECertificateHTTP01Config{ + IngressClass: strPtr("abc"), + }, + }, + "neither field specified": { + cfg: &v1alpha1.ACMECertificateHTTP01Config{}, + errs: []*field.Error{}, + }, + "both fields specified": { + cfg: &v1alpha1.ACMECertificateHTTP01Config{ + Ingress: "abc", + IngressClass: strPtr("abc"), + }, + errs: []*field.Error{ + field.Forbidden(fldPath, "only one of 'ingress' and 'ingressClass' should be specified"), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateACMECertificateHTTP01Config(s.cfg, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} diff --git a/pkg/apis/certmanager/validation/clusterissuer.go b/pkg/apis/certmanager/validation/clusterissuer.go new file mode 100644 index 000000000..f95b181b1 --- /dev/null +++ b/pkg/apis/certmanager/validation/clusterissuer.go @@ -0,0 +1,14 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" +) + +// Validation functions for cert-manager v1alpha1 ClusterIssuer types + +func ValidateClusterIssuer(iss *v1alpha1.ClusterIssuer) field.ErrorList { + allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs +} diff --git a/pkg/apis/certmanager/validation/issuer.go b/pkg/apis/certmanager/validation/issuer.go new file mode 100644 index 000000000..043d23969 --- /dev/null +++ b/pkg/apis/certmanager/validation/issuer.go @@ -0,0 +1,198 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" +) + +// Validation functions for cert-manager v1alpha1 Issuer types + +func ValidateIssuer(iss *v1alpha1.Issuer) field.ErrorList { + allErrs := ValidateIssuerSpec(&iss.Spec, field.NewPath("spec")) + return allErrs +} + +func ValidateIssuerSpec(iss *v1alpha1.IssuerSpec, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + el = ValidateIssuerConfig(&iss.IssuerConfig, fldPath) + return el +} + +func ValidateIssuerConfig(iss *v1alpha1.IssuerConfig, fldPath *field.Path) field.ErrorList { + numConfigs := 0 + el := field.ErrorList{} + if iss.ACME != nil { + if numConfigs > 0 { + el = append(el, field.Forbidden(fldPath.Child("acme"), "may not specify more than one issuer type")) + } else { + numConfigs++ + el = append(el, ValidateACMEIssuerConfig(iss.ACME, fldPath.Child("acme"))...) + } + } + if iss.CA != nil { + if numConfigs > 0 { + el = append(el, field.Forbidden(fldPath.Child("ca"), "may not specify more than one issuer type")) + } else { + numConfigs++ + el = append(el, ValidateCAIssuerConfig(iss.CA, fldPath.Child("ca"))...) + } + } + if iss.SelfSigned != nil { + if numConfigs > 0 { + el = append(el, field.Forbidden(fldPath.Child("selfSigned"), "may not specify more than one issuer type")) + } else { + numConfigs++ + el = append(el, ValidateSelfSignedIssuerConfig(iss.SelfSigned, fldPath.Child("selfSigned"))...) + } + } + if iss.Vault != nil { + if numConfigs > 0 { + el = append(el, field.Forbidden(fldPath.Child("vault"), "may not specify more than one issuer type")) + } else { + numConfigs++ + el = append(el, ValidateVaultIssuerConfig(iss.Vault, fldPath.Child("vault"))...) + } + } + if numConfigs == 0 { + el = append(el, field.Required(fldPath, "at least one issuer must be configured")) + } + return el +} + +func ValidateACMEIssuerConfig(iss *v1alpha1.ACMEIssuer, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if len(iss.Email) == 0 { + el = append(el, field.Required(fldPath.Child("email"), "email address is a required field")) + } + if len(iss.PrivateKey.Name) == 0 { + el = append(el, field.Required(fldPath.Child("privateKey", "name"), "private key secret name is a required field")) + } + if len(iss.Server) == 0 { + el = append(el, field.Required(fldPath.Child("server"), "acme server URL is a required field")) + } + if iss.HTTP01 != nil { + el = append(el, ValidateACMEIssuerHTTP01Config(iss.HTTP01, fldPath.Child("http01"))...) + } + if iss.DNS01 != nil { + el = append(el, ValidateACMEIssuerDNS01Config(iss.DNS01, fldPath.Child("dns01"))...) + } + return el +} + +func ValidateCAIssuerConfig(iss *v1alpha1.CAIssuer, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if len(iss.SecretName) == 0 { + el = append(el, field.Required(fldPath.Child("secretName"), "")) + } + return el +} + +func ValidateSelfSignedIssuerConfig(iss *v1alpha1.SelfSignedIssuer, fldPath *field.Path) field.ErrorList { + return nil +} + +func ValidateVaultIssuerConfig(iss *v1alpha1.VaultIssuer, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if len(iss.Server) == 0 { + el = append(el, field.Required(fldPath.Child("server"), "")) + } + if len(iss.Path) == 0 { + el = append(el, field.Required(fldPath.Child("path"), "")) + } + return el + // TODO: add validation for Vault authentication types +} + +func ValidateACMEIssuerHTTP01Config(iss *v1alpha1.ACMEIssuerHTTP01Config, fldPath *field.Path) field.ErrorList { + return nil +} + +func ValidateACMEIssuerDNS01Config(iss *v1alpha1.ACMEIssuerDNS01Config, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + providersFldPath := fldPath.Child("providers") + for i, p := range iss.Providers { + fldPath := providersFldPath.Index(i) + if len(p.Name) == 0 { + el = append(el, field.Required(fldPath.Child("name"), "name must be specified")) + } + numProviders := 0 + if p.Akamai != nil { + numProviders++ + el = append(el, ValidateSecretKeySelector(&p.Akamai.AccessToken, fldPath.Child("akamai", "accessToken"))...) + el = append(el, ValidateSecretKeySelector(&p.Akamai.ClientSecret, fldPath.Child("akamai", "clientSecret"))...) + el = append(el, ValidateSecretKeySelector(&p.Akamai.ClientToken, fldPath.Child("akamai", "clientToken"))...) + if len(p.Akamai.ServiceConsumerDomain) == 0 { + el = append(el, field.Required(fldPath.Child("akamai", "serviceConsumerDomain"), "")) + } + } + if p.AzureDNS != nil { + if numProviders > 0 { + el = append(el, field.Forbidden(fldPath.Child("azuredns"), "may not specify more than one provider type")) + } else { + numProviders++ + el = append(el, ValidateSecretKeySelector(&p.AzureDNS.ClientSecret, fldPath.Child("azuredns", "clientSecretSecretRef"))...) + if len(p.AzureDNS.ClientID) == 0 { + el = append(el, field.Required(fldPath.Child("azuredns", "clientID"), "")) + } + if len(p.AzureDNS.SubscriptionID) == 0 { + el = append(el, field.Required(fldPath.Child("azuredns", "subscriptionID"), "")) + } + if len(p.AzureDNS.TenantID) == 0 { + el = append(el, field.Required(fldPath.Child("azuredns", "tenantID"), "")) + } + if len(p.AzureDNS.ResourceGroupName) == 0 { + el = append(el, field.Required(fldPath.Child("azuredns", "resourceGroupName"), "")) + } + } + } + if p.CloudDNS != nil { + if numProviders > 0 { + el = append(el, field.Forbidden(fldPath.Child("clouddns"), "may not specify more than one provider type")) + } else { + numProviders++ + el = append(el, ValidateSecretKeySelector(&p.CloudDNS.ServiceAccount, fldPath.Child("clouddns", "serviceAccountSecretRef"))...) + if len(p.CloudDNS.Project) == 0 { + el = append(el, field.Required(fldPath.Child("clouddns", "project"), "")) + } + } + } + if p.Cloudflare != nil { + if numProviders > 0 { + el = append(el, field.Forbidden(fldPath.Child("cloudflare"), "may not specify more than one provider type")) + } else { + numProviders++ + el = append(el, ValidateSecretKeySelector(&p.Cloudflare.APIKey, fldPath.Child("cloudflare", "apiKeySecretRef"))...) + if len(p.Cloudflare.Email) == 0 { + el = append(el, field.Required(fldPath.Child("cloudflare", "email"), "")) + } + } + } + if p.Route53 != nil { + if numProviders > 0 { + el = append(el, field.Forbidden(fldPath.Child("route53"), "may not specify more than one provider type")) + } else { + numProviders++ + // region is the only required field for route53 as ambient credentials can be used instead + if len(p.Route53.Region) == 0 { + el = append(el, field.Required(fldPath.Child("route53", "region"), "")) + } + } + } + if numProviders == 0 { + el = append(el, field.Required(fldPath, "at least one provider must be configured")) + } + } + return el +} + +func ValidateSecretKeySelector(sks *v1alpha1.SecretKeySelector, fldPath *field.Path) field.ErrorList { + el := field.ErrorList{} + if sks.Name == "" { + el = append(el, field.Required(fldPath.Child("name"), "secret name is required")) + } + if sks.Key == "" { + el = append(el, field.Required(fldPath.Child("key"), "secret key is required")) + } + return el +} diff --git a/pkg/apis/certmanager/validation/issuer_test.go b/pkg/apis/certmanager/validation/issuer_test.go new file mode 100644 index 000000000..143b140f7 --- /dev/null +++ b/pkg/apis/certmanager/validation/issuer_test.go @@ -0,0 +1,485 @@ +package validation + +import ( + "reflect" + "testing" + + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" +) + +var ( + validCloudDNSProvider = v1alpha1.ACMEIssuerDNS01ProviderCloudDNS{ + ServiceAccount: validSecretKeyRef, + Project: "valid", + } + validSecretKeyRef = v1alpha1.SecretKeySelector{ + LocalObjectReference: v1alpha1.LocalObjectReference{ + Name: "valid", + }, + Key: "validkey", + } + validCloudflareProvider = v1alpha1.ACMEIssuerDNS01ProviderCloudflare{ + APIKey: validSecretKeyRef, + Email: "valid", + } + validACMEIssuer = v1alpha1.ACMEIssuer{ + Email: "valid-email", + Server: "valid-server", + PrivateKey: validSecretKeyRef, + } + validVaultIssuer = v1alpha1.VaultIssuer{ + Auth: v1alpha1.VaultAuth{ + TokenSecretRef: validSecretKeyRef, + }, + Server: "something", + Path: "a/b/c", + } +) + +func TestValidateVaultIssuerConfig(t *testing.T) { + fldPath := field.NewPath("") + scenarios := map[string]struct { + spec *v1alpha1.VaultIssuer + errs []*field.Error + }{ + "valid vault issuer": { + spec: &validVaultIssuer, + }, + "vault issuer with missing fields": { + spec: &v1alpha1.VaultIssuer{}, + errs: []*field.Error{ + field.Required(fldPath.Child("server"), ""), + field.Required(fldPath.Child("path"), ""), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateVaultIssuerConfig(s.spec, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} + +func TestValidateACMEIssuerConfig(t *testing.T) { + fldPath := field.NewPath("") + scenarios := map[string]struct { + spec *v1alpha1.ACMEIssuer + errs []*field.Error + }{ + "valid acme issuer": { + spec: &validACMEIssuer, + }, + "acme issuer with missing fields": { + spec: &v1alpha1.ACMEIssuer{}, + errs: []*field.Error{ + field.Required(fldPath.Child("email"), "email address is a required field"), + field.Required(fldPath.Child("privateKey", "name"), "private key secret name is a required field"), + field.Required(fldPath.Child("server"), "acme server URL is a required field"), + }, + }, + "acme issuer with invalid dns01 config": { + spec: &v1alpha1.ACMEIssuer{ + Email: "valid-email", + Server: "valid-server", + PrivateKey: validSecretKeyRef, + DNS01: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "valid-name", + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("dns01", "providers").Index(0), "at least one provider must be configured"), + }, + }, + "acme issuer with valid dns01 config": { + spec: &v1alpha1.ACMEIssuer{ + Email: "valid-email", + Server: "valid-server", + PrivateKey: validSecretKeyRef, + DNS01: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "valid-name", + CloudDNS: &validCloudDNSProvider, + }, + }, + }, + }, + }, + "acme issuer with valid http01 config": { + spec: &v1alpha1.ACMEIssuer{ + Email: "valid-email", + Server: "valid-server", + PrivateKey: validSecretKeyRef, + HTTP01: &v1alpha1.ACMEIssuerHTTP01Config{}, + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateACMEIssuerConfig(s.spec, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} + +func TestValidateIssuerSpec(t *testing.T) { + fldPath := field.NewPath("") + scenarios := map[string]struct { + spec *v1alpha1.IssuerSpec + errs []*field.Error + }{ + "valid ca issuer": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + CA: &v1alpha1.CAIssuer{ + SecretName: "valid", + }, + }, + }, + }, + "ca issuer without secret name specified": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + CA: &v1alpha1.CAIssuer{}, + }, + }, + errs: []*field.Error{field.Required(fldPath.Child("ca", "secretName"), "")}, + }, + "valid self signed issuer": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + SelfSigned: &v1alpha1.SelfSignedIssuer{}, + }, + }, + }, + "valid acme issuer": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + ACME: &validACMEIssuer, + }, + }, + }, + "valid vault issuer": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + Vault: &validVaultIssuer, + }, + }, + }, + "missing issuer config": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{}, + }, + errs: []*field.Error{ + field.Required(fldPath, "at least one issuer must be configured"), + }, + }, + "multiple issuers configured": { + spec: &v1alpha1.IssuerSpec{ + IssuerConfig: v1alpha1.IssuerConfig{ + SelfSigned: &v1alpha1.SelfSignedIssuer{}, + CA: &v1alpha1.CAIssuer{ + SecretName: "valid", + }, + }, + }, + errs: []*field.Error{ + field.Forbidden(fldPath.Child("selfSigned"), "may not specify more than one issuer type"), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateIssuerSpec(s.spec, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} + +func TestValidateACMEIssuerDNS01Config(t *testing.T) { + fldPath := field.NewPath("") + providersPath := fldPath.Child("providers") + scenarios := map[string]struct { + cfg *v1alpha1.ACMEIssuerDNS01Config + errs []*field.Error + }{ + "missing name": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + CloudDNS: &validCloudDNSProvider, + }, + }, + }, + errs: []*field.Error{field.Required(providersPath.Index(0).Child("name"), "name must be specified")}, + }, + "missing clouddns project": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + CloudDNS: &v1alpha1.ACMEIssuerDNS01ProviderCloudDNS{ + ServiceAccount: validSecretKeyRef, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("clouddns", "project"), ""), + }, + }, + "missing clouddns service account": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + CloudDNS: &v1alpha1.ACMEIssuerDNS01ProviderCloudDNS{ + Project: "valid", + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("clouddns", "serviceAccountSecretRef", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("clouddns", "serviceAccountSecretRef", "key"), "secret key is required"), + }, + }, + "missing cloudflare token": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + Cloudflare: &v1alpha1.ACMEIssuerDNS01ProviderCloudflare{ + Email: "valid", + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("cloudflare", "apiKeySecretRef", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("cloudflare", "apiKeySecretRef", "key"), "secret key is required"), + }, + }, + "missing cloudflare email": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + Cloudflare: &v1alpha1.ACMEIssuerDNS01ProviderCloudflare{ + APIKey: validSecretKeyRef, + }, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("cloudflare", "email"), ""), + }, + }, + "missing route53 region": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + Route53: &v1alpha1.ACMEIssuerDNS01ProviderRoute53{}, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("route53", "region"), ""), + }, + }, + "missing provider config": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0), "at least one provider must be configured"), + }, + }, + "missing azuredns config": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + AzureDNS: &v1alpha1.ACMEIssuerDNS01ProviderAzureDNS{}, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("azuredns", "clientSecretSecretRef", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("azuredns", "clientSecretSecretRef", "key"), "secret key is required"), + field.Required(providersPath.Index(0).Child("azuredns", "clientID"), ""), + field.Required(providersPath.Index(0).Child("azuredns", "subscriptionID"), ""), + field.Required(providersPath.Index(0).Child("azuredns", "tenantID"), ""), + field.Required(providersPath.Index(0).Child("azuredns", "resourceGroupName"), ""), + }, + }, + "missing akamai config": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + Akamai: &v1alpha1.ACMEIssuerDNS01ProviderAkamai{}, + }, + }, + }, + errs: []*field.Error{ + field.Required(providersPath.Index(0).Child("akamai", "accessToken", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("akamai", "accessToken", "key"), "secret key is required"), + field.Required(providersPath.Index(0).Child("akamai", "clientSecret", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("akamai", "clientSecret", "key"), "secret key is required"), + field.Required(providersPath.Index(0).Child("akamai", "clientToken", "name"), "secret name is required"), + field.Required(providersPath.Index(0).Child("akamai", "clientToken", "key"), "secret key is required"), + field.Required(providersPath.Index(0).Child("akamai", "serviceConsumerDomain"), ""), + }, + }, + "valid akamai config": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + Akamai: &v1alpha1.ACMEIssuerDNS01ProviderAkamai{ + AccessToken: validSecretKeyRef, + ClientSecret: validSecretKeyRef, + ClientToken: validSecretKeyRef, + ServiceConsumerDomain: "abc", + }, + }, + }, + }, + errs: []*field.Error{}, + }, + "multiple providers configured": { + cfg: &v1alpha1.ACMEIssuerDNS01Config{ + Providers: []v1alpha1.ACMEIssuerDNS01Provider{ + { + Name: "a name", + CloudDNS: &validCloudDNSProvider, + Cloudflare: &validCloudflareProvider, + }, + }, + }, + errs: []*field.Error{ + field.Forbidden(providersPath.Index(0).Child("cloudflare"), "may not specify more than one provider type"), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateACMEIssuerDNS01Config(s.cfg, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} + +func TestValidateSecretKeySelector(t *testing.T) { + validName := v1alpha1.LocalObjectReference{"name"} + validKey := "key" + // invalidName := v1alpha1.LocalObjectReference{"-name-"} + // invalidKey := "-key-" + fldPath := field.NewPath("") + scenarios := map[string]struct { + isExpectedFailure bool + selector *v1alpha1.SecretKeySelector + errs []*field.Error + }{ + "valid selector": { + selector: &v1alpha1.SecretKeySelector{ + LocalObjectReference: validName, + Key: validKey, + }, + }, + // "invalid name": { + // isExpectedFailure: true, + // selector: &v1alpha1.SecretKeySelector{ + // LocalObjectReference: invalidName, + // Key: validKey, + // }, + // }, + // "invalid key": { + // isExpectedFailure: true, + // selector: &v1alpha1.SecretKeySelector{ + // LocalObjectReference: validName, + // Key: invalidKey, + // }, + // }, + "missing name": { + selector: &v1alpha1.SecretKeySelector{ + Key: validKey, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("name"), "secret name is required"), + }, + }, + "missing key": { + selector: &v1alpha1.SecretKeySelector{ + LocalObjectReference: validName, + }, + errs: []*field.Error{ + field.Required(fldPath.Child("key"), "secret key is required"), + }, + }, + } + for n, s := range scenarios { + t.Run(n, func(t *testing.T) { + errs := ValidateSecretKeySelector(s.selector, fldPath) + if len(errs) != len(s.errs) { + t.Errorf("Expected %v but got %v", s.errs, errs) + return + } + for i, e := range errs { + expectedErr := s.errs[i] + if !reflect.DeepEqual(e, expectedErr) { + t.Errorf("Expected %v but got %v", expectedErr, e) + } + } + }) + } +} diff --git a/pkg/controller/certificates/sync.go b/pkg/controller/certificates/sync.go index 6162ee975..7b612ac39 100644 --- a/pkg/controller/certificates/sync.go +++ b/pkg/controller/certificates/sync.go @@ -4,6 +4,7 @@ import ( "context" "crypto/x509" "fmt" + "reflect" "strings" "time" @@ -15,6 +16,7 @@ import ( "github.com/golang/glog" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" + "github.com/jetstack/cert-manager/pkg/apis/certmanager/validation" "github.com/jetstack/cert-manager/pkg/issuer" "github.com/jetstack/cert-manager/pkg/util" "github.com/jetstack/cert-manager/pkg/util/errors" @@ -29,6 +31,7 @@ const ( errorIssuerNotReady = "IssuerNotReady" errorIssuerInit = "IssuerInitError" errorSavingCertificate = "SaveCertError" + errorConfig = "ConfigError" reasonIssuingCertificate = "IssueCert" reasonRenewingCertificate = "RenewCert" @@ -46,13 +49,36 @@ const ( ) func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err error) { + crtCopy := crt.DeepCopy() + defer func() { + if _, saveErr := c.updateCertificateStatus(crt, crtCopy); saveErr != nil { + err = utilerrors.NewAggregate([]error{saveErr, err}) + } + }() + + el := validation.ValidateCertificate(crtCopy) + if len(el) > 0 { + msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate()) + crtCopy.UpdateStatusCondition(v1alpha1.CertificateConditionReady, v1alpha1.ConditionFalse, errorConfig, msg, false) + return + } else { + for i, c := range crtCopy.Status.Conditions { + if c.Type == v1alpha1.CertificateConditionReady { + if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse { + crtCopy.Status.Conditions = append(crtCopy.Status.Conditions[:i], crtCopy.Status.Conditions[i+1:]...) + break + } + } + } + } + // step zero: check if the referenced issuer exists and is ready - issuerObj, err := c.getGenericIssuer(crt) + issuerObj, err := c.getGenericIssuer(crtCopy) if err != nil { s := fmt.Sprintf("Issuer %s does not exist", err.Error()) glog.Info(s) - c.recorder.Event(crt, api.EventTypeWarning, errorIssuerNotFound, s) + c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerNotFound, s) return err } @@ -63,7 +89,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e if !issuerReady { s := fmt.Sprintf("Issuer %s not ready", issuerObj.GetObjectMeta().Name) glog.Info(s) - c.recorder.Event(crt, api.EventTypeWarning, errorIssuerNotReady, s) + c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerNotReady, s) return fmt.Errorf(s) } @@ -71,12 +97,12 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e if err != nil { s := "Error initializing issuer: " + err.Error() glog.Info(s) - c.recorder.Event(crt, api.EventTypeWarning, errorIssuerInit, s) + c.recorder.Event(crtCopy, api.EventTypeWarning, errorIssuerInit, s) return err } - expectedCN := pki.CommonNameForCertificate(crt) - expectedDNSNames := pki.DNSNamesForCertificate(crt) + expectedCN := pki.CommonNameForCertificate(crtCopy) + expectedDNSNames := pki.DNSNamesForCertificate(crtCopy) if expectedCN == "" || len(expectedDNSNames) == 0 { // TODO: Set certificate invalid condition on certificate resource // TODO: remove this check in favour of resource validation @@ -84,8 +110,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e } // grab existing certificate and validate private key - cert, err := kube.SecretTLSCert(c.secretLister, crt.Namespace, crt.Spec.SecretName) - + cert, err := kube.SecretTLSCert(c.secretLister, crtCopy.Namespace, crtCopy.Spec.SecretName) // if an error is returned, and that error is something other than // IsNotFound or invalid data, then we should return the error. if err != nil && !k8sErrors.IsNotFound(err) && !errors.IsInvalidData(err) { @@ -95,9 +120,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e // as there is an existing certificate, or we may create one below, we will // run scheduleRenewal to schedule a renewal if required at the end of // execution. - defer c.scheduleRenewal(crt) - - crtCopy := crt.DeepCopy() + defer c.scheduleRenewal(crtCopy) // if the certificate was not found, or the certificate data is invalid, we // should issue a new certificate. @@ -105,12 +128,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e // listed in the certificate spec, we should re-issue the certificate. if k8sErrors.IsNotFound(err) || errors.IsInvalidData(err) || expectedCN != cert.Subject.CommonName || !util.EqualUnsorted(cert.DNSNames, expectedDNSNames) { - err := c.issue(ctx, i, crtCopy) - updateErr := c.updateCertificateStatus(crtCopy) - if err != nil || updateErr != nil { - return utilerrors.NewAggregate([]error{err, updateErr}) - } - return nil + return c.issue(ctx, i, crtCopy) } // calculate the amount of time until expiry @@ -120,11 +138,7 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e renewIn := durationUntilExpiry - renewBefore // if we should being attempting to renew now, then trigger a renewal if renewIn <= 0 { - err := c.renew(ctx, i, crtCopy) - updateErr := c.updateCertificateStatus(crtCopy) - if err != nil || updateErr != nil { - return utilerrors.NewAggregate([]error{err, updateErr}) - } + return c.renew(ctx, i, crtCopy) } return nil @@ -306,10 +320,12 @@ func (c *Controller) renew(ctx context.Context, issuer issuer.Interface, crt *v1 return nil } -func (c *Controller) updateCertificateStatus(crt *v1alpha1.Certificate) error { +func (c *Controller) updateCertificateStatus(old, new *v1alpha1.Certificate) (*v1alpha1.Certificate, error) { + if reflect.DeepEqual(old.Status, new.Status) { + return nil, nil + } // TODO: replace Update call with UpdateStatus. This requires a custom API // server with the /status subresource enabled and/or subresource support // for CRDs (https://github.com/kubernetes/kubernetes/issues/38113) - _, err := c.cmClient.CertmanagerV1alpha1().Certificates(crt.Namespace).Update(crt) - return err + return c.cmClient.CertmanagerV1alpha1().Certificates(new.Namespace).Update(new) } diff --git a/pkg/controller/clusterissuers/sync.go b/pkg/controller/clusterissuers/sync.go index ec8acf530..b6b676749 100644 --- a/pkg/controller/clusterissuers/sync.go +++ b/pkg/controller/clusterissuers/sync.go @@ -2,6 +2,7 @@ package clusterissuers import ( "context" + "fmt" "reflect" "github.com/golang/glog" @@ -9,37 +10,46 @@ import ( "k8s.io/apimachinery/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" + "github.com/jetstack/cert-manager/pkg/apis/certmanager/validation" ) const ( errorInitIssuer = "ErrInitIssuer" + errorConfig = "ConfigError" messageErrorInitIssuer = "Error initializing issuer: " ) func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.ClusterIssuer) (err error) { issuerCopy := iss.DeepCopy() - i, err := c.issuerFactory.IssuerFor(issuerCopy) + defer func() { + if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil { + err = errors.NewAggregate([]error{saveErr, err}) + } + }() + el := validation.ValidateClusterIssuer(issuerCopy) + if len(el) > 0 { + msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate()) + issuerCopy.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorConfig, msg) + return + } else { + for i, c := range issuerCopy.Status.Conditions { + if c.Type == v1alpha1.IssuerConditionReady { + if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse { + issuerCopy.Status.Conditions = append(issuerCopy.Status.Conditions[:i], issuerCopy.Status.Conditions[i+1:]...) + break + } + } + } + } + + i, err := c.issuerFactory.IssuerFor(issuerCopy) if err != nil { return err } err = i.Setup(ctx) - defer func() { - // TODO: replace this with more efficient comparison? - if reflect.DeepEqual(issuerCopy.Status, iss.Status) { - return - } - if saveErr := c.updateIssuerStatus(issuerCopy); saveErr != nil { - errs := []error{saveErr} - if err != nil { - errs = append(errs, err) - } - err = errors.NewAggregate(errs) - } - }() - if err != nil { s := messageErrorInitIssuer + err.Error() glog.Info(s) @@ -50,10 +60,12 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.ClusterIssuer) (err return nil } -func (c *Controller) updateIssuerStatus(iss *v1alpha1.ClusterIssuer) error { +func (c *Controller) updateIssuerStatus(old, new *v1alpha1.ClusterIssuer) (*v1alpha1.ClusterIssuer, error) { + if reflect.DeepEqual(old.Status, new.Status) { + return nil, nil + } // TODO: replace Update call with UpdateStatus. This requires a custom API // server with the /status subresource enabled and/or subresource support // for CRDs (https://github.com/kubernetes/kubernetes/issues/38113) - _, err := c.cmClient.CertmanagerV1alpha1().ClusterIssuers().Update(iss) - return err + return c.cmClient.CertmanagerV1alpha1().ClusterIssuers().Update(new) } diff --git a/pkg/controller/clusterissuers/sync_test.go b/pkg/controller/clusterissuers/sync_test.go index 68a910912..6d8bedfc0 100644 --- a/pkg/controller/clusterissuers/sync_test.go +++ b/pkg/controller/clusterissuers/sync_test.go @@ -51,7 +51,7 @@ func TestUpdateIssuerStatus(t *testing.T) { issuerCopy := issuer.DeepCopy() issuerCopy.Status = newStatus - err = c.updateIssuerStatus(issuerCopy) + _, err = c.updateIssuerStatus(issuer, issuerCopy) assertErrIsNil(t, fatalf, err) actions := cmClient.Actions() diff --git a/pkg/controller/issuers/sync.go b/pkg/controller/issuers/sync.go index f2d977481..682e911b8 100644 --- a/pkg/controller/issuers/sync.go +++ b/pkg/controller/issuers/sync.go @@ -2,6 +2,7 @@ package issuers import ( "context" + "fmt" "reflect" "github.com/golang/glog" @@ -9,16 +10,40 @@ import ( "k8s.io/apimachinery/pkg/util/errors" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" + "github.com/jetstack/cert-manager/pkg/apis/certmanager/validation" ) const ( errorInitIssuer = "ErrInitIssuer" + errorConfig = "ConfigError" messageErrorInitIssuer = "Error initializing issuer: " ) func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error) { issuerCopy := iss.DeepCopy() + defer func() { + if _, saveErr := c.updateIssuerStatus(iss, issuerCopy); saveErr != nil { + err = errors.NewAggregate([]error{saveErr, err}) + } + }() + + el := validation.ValidateIssuer(issuerCopy) + if len(el) > 0 { + msg := fmt.Sprintf("Resource validation failed: %v", el.ToAggregate()) + issuerCopy.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorConfig, msg) + return + } else { + for i, c := range issuerCopy.Status.Conditions { + if c.Type == v1alpha1.IssuerConditionReady { + if c.Reason == errorConfig && c.Status == v1alpha1.ConditionFalse { + issuerCopy.Status.Conditions = append(issuerCopy.Status.Conditions[:i], issuerCopy.Status.Conditions[i+1:]...) + break + } + } + } + } + i, err := c.issuerFactory.IssuerFor(issuerCopy) if err != nil { @@ -26,20 +51,6 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error) } err = i.Setup(ctx) - defer func() { - // TODO: replace this with more efficient comparison? - if reflect.DeepEqual(issuerCopy.Status, iss.Status) { - return - } - if saveErr := c.updateIssuerStatus(issuerCopy); saveErr != nil { - errs := []error{saveErr} - if err != nil { - errs = append(errs, err) - } - err = errors.NewAggregate(errs) - } - }() - if err != nil { s := messageErrorInitIssuer + err.Error() glog.Info(s) @@ -50,10 +61,12 @@ func (c *Controller) Sync(ctx context.Context, iss *v1alpha1.Issuer) (err error) return nil } -func (c *Controller) updateIssuerStatus(iss *v1alpha1.Issuer) error { +func (c *Controller) updateIssuerStatus(old, new *v1alpha1.Issuer) (*v1alpha1.Issuer, error) { + if reflect.DeepEqual(old.Status, new.Status) { + return nil, nil + } // TODO: replace Update call with UpdateStatus. This requires a custom API // server with the /status subresource enabled and/or subresource support // for CRDs (https://github.com/kubernetes/kubernetes/issues/38113) - _, err := c.cmClient.CertmanagerV1alpha1().Issuers(iss.Namespace).Update(iss) - return err + return c.cmClient.CertmanagerV1alpha1().Issuers(new.Namespace).Update(new) } diff --git a/pkg/controller/issuers/sync_test.go b/pkg/controller/issuers/sync_test.go index f607514f4..90156c41d 100644 --- a/pkg/controller/issuers/sync_test.go +++ b/pkg/controller/issuers/sync_test.go @@ -51,7 +51,7 @@ func TestUpdateIssuerStatus(t *testing.T) { issuerCopy := issuer.DeepCopy() issuerCopy.Status = newStatus - err = c.updateIssuerStatus(issuerCopy) + _, err = c.updateIssuerStatus(issuer, issuerCopy) assertErrIsNil(t, fatalf, err) actions := cmClient.Actions()