From bb89b50c8f7a08bc2609a81991ccd4ad79906a83 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Fri, 28 Aug 2020 09:54:18 +0200 Subject: [PATCH 1/7] Fix invalid DNS-1123 on ACME computed names Signed-off-by: Maartje Eyskens --- pkg/controller/acmeorders/util.go | 12 +++- pkg/controller/acmeorders/util_test.go | 53 +++++++++++++++ .../certificaterequests/acme/acme.go | 22 +++++-- .../certificaterequests/acme/acme_test.go | 65 +++++++++++++++++++ 4 files changed, 145 insertions(+), 7 deletions(-) diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index 8b4697ef1..2ec6443f0 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -20,8 +20,10 @@ import ( "context" "encoding/json" "fmt" - "github.com/jetstack/cert-manager/pkg/acme" "hash/fnv" + "regexp" + + "github.com/jetstack/cert-manager/pkg/acme" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -86,6 +88,14 @@ func buildChallengeName(orderName string, chSpec cmacme.ChallengeSpec) (string, return "", err } + if len(orderName) >= 52 { + // shorten the cert name to 52 chars to ensure the total length of the name + // also shorten the 52 char string to the last non-symbol character + // is less than or equal to 64 characters + validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", orderName), -1) + orderName = orderName[:validCharIndexes[len(validCharIndexes)-1][1]] + } + return fmt.Sprintf("%s-%d", orderName, hash), nil } diff --git a/pkg/controller/acmeorders/util_test.go b/pkg/controller/acmeorders/util_test.go index 5d7385e48..40e7e5f02 100644 --- a/pkg/controller/acmeorders/util_test.go +++ b/pkg/controller/acmeorders/util_test.go @@ -1274,3 +1274,56 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }) } } + +func Test_buildChallengeName(t *testing.T) { + type args struct { + orderName string + chSpec cmacme.ChallengeSpec + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "Test output of a short domain", + args: args{ + orderName: "unit.test.jetstack.io-1683025094-3418488114", + chSpec: cmacme.ChallengeSpec{}, + }, + want: "unit.test.jetstack.io-1683025094-3418488114-4226717179", + wantErr: false, + }, + { + name: "Test output of a 52 character CR name", + args: args{ + orderName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", + chSpec: cmacme.ChallengeSpec{}, + }, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", + wantErr: false, + }, + { + name: "Test output of a dot at 52nd character CR name", + args: args{ + orderName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com-tl2t89g", + chSpec: cmacme.ChallengeSpec{}, + }, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := buildChallengeName(tt.args.orderName, tt.args.chSpec) + if (err != nil) != tt.wantErr { + t.Errorf("buildChallengeName() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("buildChallengeName() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index 234df746c..67c6f0e57 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -22,6 +22,7 @@ import ( "encoding/json" "fmt" "hash/fnv" + "regexp" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -208,7 +209,7 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm CommonName: csr.Subject.CommonName, DNSNames: csr.DNSNames, } - hash, err := hashOrder(spec) + name, err := computeACMEOrderName(cr, spec) if err != nil { return nil, err } @@ -218,7 +219,7 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm // the hyphen. return &cmacme.Order{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("%.52s-%d", cr.Name, hash), + Name: name, Namespace: cr.Namespace, Labels: cr.Labels, Annotations: cr.Annotations, @@ -230,20 +231,29 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm }, nil } -func hashOrder(orderSpec cmacme.OrderSpec) (uint32, error) { +func computeACMEOrderName(cr *v1.CertificateRequest, orderSpec cmacme.OrderSpec) (string, error) { // create a shallow copy of the OrderSpec so we can overwrite the Request field orderSpec.Request = nil orderSpecBytes, err := json.Marshal(orderSpec) if err != nil { - return 0, err + return "", err } hashF := fnv.New32() _, err = hashF.Write(orderSpecBytes) if err != nil { - return 0, err + return "", err } - return hashF.Sum32(), nil + crName := cr.Name + if len(crName) >= 52 { + // shorten the cert name to 52 chars to ensure the total length of the name + // also shorten the 52 char string to the last non-symbol character + // is less than or equal to 64 characters + validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crName), -1) + crName = crName[:validCharIndexes[len(validCharIndexes)-1][1]] + } + + return fmt.Sprintf("%s-%d", crName, hashF.Sum32()), nil } diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index b83ed9d77..c503c1a38 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -420,3 +420,68 @@ func runTest(t *testing.T, test testT) { test.builder.CheckAndFinish(err) } + +func TestComputeACMEOrderName(t *testing.T) { + type args struct { + cr *cmapi.CertificateRequest + orderSpec cmacme.OrderSpec + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "Test output of a short domain", + args: args{ + cr: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unit.test.jetstack.io-1683025094", + }, + }, + orderSpec: cmacme.OrderSpec{}, + }, + want: "unit.test.jetstack.io-1683025094-3418488114", + wantErr: false, + }, + { + name: "Test output of a 52 character CR name", + args: args{ + cr: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-108802726", + }, + }, + orderSpec: cmacme.OrderSpec{}, + }, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-3418488114", + wantErr: false, + }, + { + name: "Test output of a dot at 52nd character CR name", + args: args{ + cr: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com-tl2t89g", + }, + }, + orderSpec: cmacme.OrderSpec{}, + }, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-3418488114", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := computeACMEOrderName(tt.args.cr, tt.args.orderSpec) + if (err != nil) != tt.wantErr { + t.Errorf("ComputeACMEOrderName() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("ComputeACMEOrderName() got = %v, want %v", got, tt.want) + } + }) + } +} From 69186afbdd407e017b2d55ad837d55ae822d0d00 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Fri, 28 Aug 2020 09:59:48 +0200 Subject: [PATCH 2/7] Move logic to utils Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 21 ++++++++++++------- pkg/controller/acmeorders/util.go | 14 +++---------- .../certificaterequests/acme/acme.go | 11 +--------- 3 files changed, 17 insertions(+), 29 deletions(-) diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 2f3e2a0e7..228c24c8d 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -38,14 +38,19 @@ func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { return "", err } - crtName := crt.Name - if len(crtName) >= 52 { - // shorten the cert name to 52 chars to ensure the total length of the name - // also shorten the 52 char string to the last non-symbol character - // is less than or equal to 64 characters - validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crtName), -1) - crtName = crtName[:validCharIndexes[len(validCharIndexes)-1][1]] - } + crtName := DNSSafeSchortenTo52Characters(crt.Name) return fmt.Sprintf("%s-%d", crtName, hashF.Sum32()), nil } + +func DNSSafeSchortenTo52Characters(in string) string { + if len(in) >= 52 { + // shorten the cert name to 52 chars to ensure the total length of the name + // also shorten the 52 char string to the last non-symbol character + // is less than or equal to 64 characters + validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", in), -1) + in = in[:validCharIndexes[len(validCharIndexes)-1][1]] + } + + return in +} diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index 2ec6443f0..d9c4bc3b4 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -21,13 +21,12 @@ import ( "encoding/json" "fmt" "hash/fnv" - "regexp" - - "github.com/jetstack/cert-manager/pkg/acme" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/jetstack/cert-manager/pkg/acme" acmecl "github.com/jetstack/cert-manager/pkg/acme/client" + "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" "github.com/jetstack/cert-manager/pkg/controller/acmeorders/selectors" @@ -88,14 +87,7 @@ func buildChallengeName(orderName string, chSpec cmacme.ChallengeSpec) (string, return "", err } - if len(orderName) >= 52 { - // shorten the cert name to 52 chars to ensure the total length of the name - // also shorten the 52 char string to the last non-symbol character - // is less than or equal to 64 characters - validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", orderName), -1) - orderName = orderName[:validCharIndexes[len(validCharIndexes)-1][1]] - } - + orderName = util.DNSSafeSchortenTo52Characters(orderName) return fmt.Sprintf("%s-%d", orderName, hash), nil } diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index 67c6f0e57..1ee6e6d95 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -22,7 +22,6 @@ import ( "encoding/json" "fmt" "hash/fnv" - "regexp" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -246,14 +245,6 @@ func computeACMEOrderName(cr *v1.CertificateRequest, orderSpec cmacme.OrderSpec) return "", err } - crName := cr.Name - if len(crName) >= 52 { - // shorten the cert name to 52 chars to ensure the total length of the name - // also shorten the 52 char string to the last non-symbol character - // is less than or equal to 64 characters - validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crName), -1) - crName = crName[:validCharIndexes[len(validCharIndexes)-1][1]] - } - + crName := apiutil.DNSSafeSchortenTo52Characters(cr.Name) return fmt.Sprintf("%s-%d", crName, hashF.Sum32()), nil } From f1c6c93df5fded5d3c00846a518310280b82a93c Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Fri, 28 Aug 2020 15:06:54 +0200 Subject: [PATCH 3/7] Fix CR and make a general function Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 14 ++-- pkg/api/util/names_test.go | 10 +-- pkg/controller/acmeorders/BUILD.bazel | 1 + pkg/controller/acmeorders/util.go | 15 +---- pkg/controller/acmeorders/util_test.go | 53 --------------- .../certificaterequests/acme/acme.go | 23 +------ .../certificaterequests/acme/acme_test.go | 65 ------------------- .../certificates/internal/test/test.go | 2 +- .../requestmanager_controller.go | 2 +- .../certificates/requestmanager/util_test.go | 2 +- test/e2e/framework/helper/certificates.go | 2 +- 11 files changed, 20 insertions(+), 169 deletions(-) diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 228c24c8d..43e124c1b 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -22,28 +22,26 @@ import ( "hash/fnv" "regexp" - - cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" ) -func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { - specBytes, err := json.Marshal(crt.Spec) +func ComputeName(prefix string, obj interface{}) (string, error) { + objectBytes, err := json.Marshal(obj) if err != nil { return "", err } hashF := fnv.New32() - _, err = hashF.Write(specBytes) + _, err = hashF.Write(objectBytes) if err != nil { return "", err } - crtName := DNSSafeSchortenTo52Characters(crt.Name) + prefix = DNSSafeShortenTo52Characters(prefix) - return fmt.Sprintf("%s-%d", crtName, hashF.Sum32()), nil + return fmt.Sprintf("%s-%d", prefix, hashF.Sum32()), nil } -func DNSSafeSchortenTo52Characters(in string) string { +func DNSSafeShortenTo52Characters(in string) string { if len(in) >= 52 { // shorten the cert name to 52 chars to ensure the total length of the name // also shorten the 52 char string to the last non-symbol character diff --git a/pkg/api/util/names_test.go b/pkg/api/util/names_test.go index 556698ed8..1e00d4cf6 100644 --- a/pkg/api/util/names_test.go +++ b/pkg/api/util/names_test.go @@ -24,7 +24,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation" ) -func TestComputeCertificateRequestName(t *testing.T) { +func TestComputeName(t *testing.T) { type args struct { crt *cmapi.Certificate } @@ -97,16 +97,16 @@ func TestComputeCertificateRequestName(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := ComputeCertificateRequestName(tt.args.crt) + got, err := ComputeName(tt.args.crt.Name, tt.args.crt.Spec) if (err != nil) != tt.wantErr { - t.Errorf("ComputeCertificateRequestName() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("ComputeName() error = %v, wantErr %v", err, tt.wantErr) return } if got != tt.want { - t.Errorf("ComputeCertificateRequestName() = %v, want %v", got, tt.want) + t.Errorf("ComputeName() = %v, want %v", got, tt.want) } if len(validation.IsQualifiedName(got)) != 0 { - t.Errorf("ComputeCertificateRequestName() = %v is not DNS-1123 valid", got) + t.Errorf("ComputeName() = %v is not DNS-1123 valid", got) } }) } diff --git a/pkg/controller/acmeorders/BUILD.bazel b/pkg/controller/acmeorders/BUILD.bazel index 6d345dee3..4382434f3 100644 --- a/pkg/controller/acmeorders/BUILD.bazel +++ b/pkg/controller/acmeorders/BUILD.bazel @@ -14,6 +14,7 @@ go_library( "//pkg/acme:go_default_library", "//pkg/acme/accounts:go_default_library", "//pkg/acme/client:go_default_library", + "//pkg/api/util:go_default_library", "//pkg/apis/acme/v1:go_default_library", "//pkg/apis/certmanager/v1:go_default_library", "//pkg/client/clientset/versioned:go_default_library", diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index d9c4bc3b4..8789fae9d 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -22,11 +22,12 @@ import ( "fmt" "hash/fnv" + "github.com/jetstack/cert-manager/pkg/api/util" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/acme" acmecl "github.com/jetstack/cert-manager/pkg/acme/client" - "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" "github.com/jetstack/cert-manager/pkg/controller/acmeorders/selectors" @@ -65,7 +66,7 @@ func buildChallenge(ctx context.Context, cl acmecl.Interface, issuer cmapi.Gener return nil, err } - chName, err := buildChallengeName(o.Name, *chSpec) + chName, err := util.ComputeName(o.Name, chSpec) if err != nil { return nil, err } @@ -81,16 +82,6 @@ func buildChallenge(ctx context.Context, cl acmecl.Interface, issuer cmapi.Gener }, nil } -func buildChallengeName(orderName string, chSpec cmacme.ChallengeSpec) (string, error) { - hash, err := hashChallenge(chSpec) - if err != nil { - return "", err - } - - orderName = util.DNSSafeSchortenTo52Characters(orderName) - return fmt.Sprintf("%s-%d", orderName, hash), nil -} - func hashChallenge(spec cmacme.ChallengeSpec) (uint32, error) { specBytes, err := json.Marshal(spec) if err != nil { diff --git a/pkg/controller/acmeorders/util_test.go b/pkg/controller/acmeorders/util_test.go index 40e7e5f02..5d7385e48 100644 --- a/pkg/controller/acmeorders/util_test.go +++ b/pkg/controller/acmeorders/util_test.go @@ -1274,56 +1274,3 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }) } } - -func Test_buildChallengeName(t *testing.T) { - type args struct { - orderName string - chSpec cmacme.ChallengeSpec - } - tests := []struct { - name string - args args - want string - wantErr bool - }{ - { - name: "Test output of a short domain", - args: args{ - orderName: "unit.test.jetstack.io-1683025094-3418488114", - chSpec: cmacme.ChallengeSpec{}, - }, - want: "unit.test.jetstack.io-1683025094-3418488114-4226717179", - wantErr: false, - }, - { - name: "Test output of a 52 character CR name", - args: args{ - orderName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", - chSpec: cmacme.ChallengeSpec{}, - }, - want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", - wantErr: false, - }, - { - name: "Test output of a dot at 52nd character CR name", - args: args{ - orderName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com-tl2t89g", - chSpec: cmacme.ChallengeSpec{}, - }, - want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-4226717179", - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := buildChallengeName(tt.args.orderName, tt.args.chSpec) - if (err != nil) != tt.wantErr { - t.Errorf("buildChallengeName() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("buildChallengeName() got = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index 1ee6e6d95..5e68a49b0 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -19,9 +19,7 @@ package acme import ( "context" "crypto/x509" - "encoding/json" "fmt" - "hash/fnv" k8sErrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -208,7 +206,7 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm CommonName: csr.Subject.CommonName, DNSNames: csr.DNSNames, } - name, err := computeACMEOrderName(cr, spec) + name, err := apiutil.ComputeName(cr.Name, spec) if err != nil { return nil, err } @@ -229,22 +227,3 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm Spec: spec, }, nil } - -func computeACMEOrderName(cr *v1.CertificateRequest, orderSpec cmacme.OrderSpec) (string, error) { - // create a shallow copy of the OrderSpec so we can overwrite the Request field - orderSpec.Request = nil - - orderSpecBytes, err := json.Marshal(orderSpec) - if err != nil { - return "", err - } - - hashF := fnv.New32() - _, err = hashF.Write(orderSpecBytes) - if err != nil { - return "", err - } - - crName := apiutil.DNSSafeSchortenTo52Characters(cr.Name) - return fmt.Sprintf("%s-%d", crName, hashF.Sum32()), nil -} diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go index c503c1a38..b83ed9d77 100644 --- a/pkg/controller/certificaterequests/acme/acme_test.go +++ b/pkg/controller/certificaterequests/acme/acme_test.go @@ -420,68 +420,3 @@ func runTest(t *testing.T, test testT) { test.builder.CheckAndFinish(err) } - -func TestComputeACMEOrderName(t *testing.T) { - type args struct { - cr *cmapi.CertificateRequest - orderSpec cmacme.OrderSpec - } - tests := []struct { - name string - args args - want string - wantErr bool - }{ - { - name: "Test output of a short domain", - args: args{ - cr: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit.test.jetstack.io-1683025094", - }, - }, - orderSpec: cmacme.OrderSpec{}, - }, - want: "unit.test.jetstack.io-1683025094-3418488114", - wantErr: false, - }, - { - name: "Test output of a 52 character CR name", - args: args{ - cr: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-108802726", - }, - }, - orderSpec: cmacme.OrderSpec{}, - }, - want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-3418488114", - wantErr: false, - }, - { - name: "Test output of a dot at 52nd character CR name", - args: args{ - cr: &cmapi.CertificateRequest{ - ObjectMeta: metav1.ObjectMeta{ - Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com-tl2t89g", - }, - }, - orderSpec: cmacme.OrderSpec{}, - }, - want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-3418488114", - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := computeACMEOrderName(tt.args.cr, tt.args.orderSpec) - if (err != nil) != tt.wantErr { - t.Errorf("ComputeACMEOrderName() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("ComputeACMEOrderName() got = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/pkg/controller/certificates/internal/test/test.go b/pkg/controller/certificates/internal/test/test.go index 3561ec703..65435c9f7 100644 --- a/pkg/controller/certificates/internal/test/test.go +++ b/pkg/controller/certificates/internal/test/test.go @@ -84,7 +84,7 @@ func createCryptoBundle(originalCert *cmapi.Certificate, fixedClock *fakeclock.F if crt.Spec.PrivateKey == nil { crt.Spec.PrivateKey = &cmapi.CertificatePrivateKey{} } - reqName, err := apiutil.ComputeCertificateRequestName(crt) + reqName, err := apiutil.ComputeName(crt.Name, crt.Spec) if err != nil { return nil, err } diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index d1d334526..3334d21b2 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -322,7 +322,7 @@ func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi cr := &cmapi.CertificateRequest{ ObjectMeta: metav1.ObjectMeta{ Namespace: crt.Namespace, - GenerateName: crt.Name + "-", + GenerateName: apiutil.DNSSafeShortenTo52Characters(crt.Name) + "-", Annotations: annotations, Labels: crt.Labels, OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(crt, certificateGvk)}, diff --git a/pkg/controller/certificates/requestmanager/util_test.go b/pkg/controller/certificates/requestmanager/util_test.go index 7b7e7a1ed..58310d2c9 100644 --- a/pkg/controller/certificates/requestmanager/util_test.go +++ b/pkg/controller/certificates/requestmanager/util_test.go @@ -73,7 +73,7 @@ func createCryptoBundle(originalCert *cmapi.Certificate) (*cryptoBundle, error) if crt.Spec.PrivateKey == nil { crt.Spec.PrivateKey = &cmapi.CertificatePrivateKey{} } - reqName, err := apiutil.ComputeCertificateRequestName(crt) + reqName, err := apiutil.ComputeName(crt.Name, crt.Spec) if err != nil { return nil, err } diff --git a/test/e2e/framework/helper/certificates.go b/test/e2e/framework/helper/certificates.go index f9ba53b69..549138bb7 100644 --- a/test/e2e/framework/helper/certificates.go +++ b/test/e2e/framework/helper/certificates.go @@ -364,7 +364,7 @@ func (h *Helper) describeCertificateRequestFromCertificate(ns string, certificat return } - crName, err := apiutil.ComputeCertificateRequestName(certificate) + crName, err := apiutil.ComputeName(certificate.Name, certificate.Spec) if err != nil { log.Logf("Failed to compute CertificateRequest name from certificate: %s", err) return From 6756856e828c750176560e143ef41ae77d5a764f Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Mon, 31 Aug 2020 10:08:54 +0200 Subject: [PATCH 4/7] Nil out request in spec again for order Signed-off-by: Maartje Eyskens --- pkg/controller/certificaterequests/acme/acme.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index 5e68a49b0..d61ee8f56 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -206,7 +206,11 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm CommonName: csr.Subject.CommonName, DNSNames: csr.DNSNames, } - name, err := apiutil.ComputeName(cr.Name, spec) + + computeNameSpec := spec.DeepCopy() + // create a shallow copy of the OrderSpec so we can overwrite the Request field + computeNameSpec.Request = nil + name, err := apiutil.ComputeName(cr.Name, computeNameSpec) if err != nil { return nil, err } From 302772013de60e98110d8cb0b2e5b7e334d27566 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 1 Sep 2020 15:03:25 +0200 Subject: [PATCH 5/7] Add note on why we shorten names Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 43e124c1b..e74cdacf4 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -36,6 +36,8 @@ func ComputeName(prefix string, obj interface{}) (string, error) { return "", err } + // we're shortening to stay under 64 as we use this in services + // and pods down the road for ACME resources. prefix = DNSSafeShortenTo52Characters(prefix) return fmt.Sprintf("%s-%d", prefix, hashF.Sum32()), nil From 9d0a1b136ec130f1dc9f46e732aa1835e9d22e11 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 1 Sep 2020 15:05:48 +0200 Subject: [PATCH 6/7] Update pkg/controller/certificaterequests/acme/acme.go Signed-off-by: Maartje Eyskens Co-authored-by: Richard Wall --- pkg/controller/certificaterequests/acme/acme.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/certificaterequests/acme/acme.go b/pkg/controller/certificaterequests/acme/acme.go index d61ee8f56..29bfee2a6 100644 --- a/pkg/controller/certificaterequests/acme/acme.go +++ b/pkg/controller/certificaterequests/acme/acme.go @@ -208,7 +208,7 @@ func buildOrder(cr *v1.CertificateRequest, csr *x509.CertificateRequest) (*cmacm } computeNameSpec := spec.DeepCopy() - // create a shallow copy of the OrderSpec so we can overwrite the Request field + // create a deep copy of the OrderSpec so we can overwrite the Request field computeNameSpec.Request = nil name, err := apiutil.ComputeName(cr.Name, computeNameSpec) if err != nil { From c73c121ce1fd5ec186749c48d7da6733c611f883 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 1 Sep 2020 15:08:31 +0200 Subject: [PATCH 7/7] Order imports Signed-off-by: Maartje Eyskens --- pkg/controller/acmeorders/util.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index 8789fae9d..f30e16935 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -22,12 +22,11 @@ import ( "fmt" "hash/fnv" - "github.com/jetstack/cert-manager/pkg/api/util" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/acme" acmecl "github.com/jetstack/cert-manager/pkg/acme/client" + "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" "github.com/jetstack/cert-manager/pkg/controller/acmeorders/selectors"