From 3212e446e5388b17b188c16dad9e721d2e25cd42 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Thu, 16 Jan 2020 11:49:28 +0100 Subject: [PATCH 1/4] Fix issue in ComputeCertificateRequestName when 52nd is a dot This strips any symbols at the end of a shortened domain name in ComputeCertificateRequestName. It also adds tests for the specific util function Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 13 ++++- pkg/api/util/names_test.go | 113 +++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 pkg/api/util/names_test.go diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 65de19ea0..3208e32c0 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -22,6 +22,7 @@ import ( "hash/fnv" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" + "gopkg.in/src-d/enry.v1/regex" ) func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { @@ -37,7 +38,13 @@ func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { return "", err } - // shorten the cert name to 52 chars to ensure the total length of the name - // is less than or equal to 64 characters - return fmt.Sprintf("%.52s-%d", crt.Name, hashF.Sum32()), nil + if len(crt.Name) >= 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 := regex.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crt.Name), -1) + crt.Name = crt.Name[:validCharIndexes[len(validCharIndexes)-1][1]] + } + + return fmt.Sprintf("%s-%d", crt.Name, hashF.Sum32()), nil } diff --git a/pkg/api/util/names_test.go b/pkg/api/util/names_test.go new file mode 100644 index 000000000..14f3a93f5 --- /dev/null +++ b/pkg/api/util/names_test.go @@ -0,0 +1,113 @@ +/* +Copyright 2019 The Jetstack cert-manager contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +import ( + "testing" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/validation" +) + +func TestComputeCertificateRequestName(t *testing.T) { + type args struct { + crt *cmapi.Certificate + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "Name generation short domains", + args: args{ + crt: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unit.test.jetstack.io", + }, + Spec: cmapi.CertificateSpec{ + CommonName: "unit.test.jetstack.io", + }, + }, + }, + wantErr: false, + want: "unit.test.jetstack.io-1683025094", + }, + { + name: "Name generation too long domains", + args: args{ + crt: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab.jetstack.io", + }, + Spec: cmapi.CertificateSpec{ + CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab.jetstack.io", + }, + }, + }, + wantErr: false, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-108802726", + }, + { + name: "Name generation for dot as 52nd char", + args: args{ + crt: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io", + }, + Spec: cmapi.CertificateSpec{ + CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io", + }, + }, + }, + wantErr: false, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-225297437", + }, + { + name: "Name generation for dot as 54td char", + args: args{ + crt: &cmapi.Certificate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io", + }, + Spec: cmapi.CertificateSpec{ + CommonName: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.jetstack.io", + }, + }, + }, + wantErr: false, + want: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa-1448584771", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ComputeCertificateRequestName(tt.args.crt) + if (err != nil) != tt.wantErr { + t.Errorf("ComputeCertificateRequestName() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("ComputeCertificateRequestName() = %v, want %v", got, tt.want) + } + if len(validation.IsQualifiedName(got)) != 0 { + t.Errorf("ComputeCertificateRequestName() = %v is not DNS-1123 valid", got) + } + }) + } +} From dbac7cf1fb3f59ea6ca1a9261909ae73404dd77d Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Thu, 16 Jan 2020 12:03:58 +0100 Subject: [PATCH 2/4] Use correct regexp package Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 3208e32c0..5a2de69be 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -21,8 +21,9 @@ import ( "fmt" "hash/fnv" + "regexp" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" - "gopkg.in/src-d/enry.v1/regex" ) func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { @@ -42,7 +43,7 @@ func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { // 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 := regex.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crt.Name), -1) + validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crt.Name), -1) crt.Name = crt.Name[:validCharIndexes[len(validCharIndexes)-1][1]] } From 0e1f64a33746bd3f839c42ab6daff5f16a004232 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Thu, 16 Jan 2020 12:07:45 +0100 Subject: [PATCH 3/4] Do not change crt.Name Signed-off-by: Maartje Eyskens --- pkg/api/util/names.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/api/util/names.go b/pkg/api/util/names.go index 5a2de69be..2b9796459 100644 --- a/pkg/api/util/names.go +++ b/pkg/api/util/names.go @@ -27,7 +27,6 @@ import ( ) func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { - crt = crt.DeepCopy() specBytes, err := json.Marshal(crt.Spec) if err != nil { return "", err @@ -39,13 +38,14 @@ func ComputeCertificateRequestName(crt *cmapi.Certificate) (string, error) { return "", err } - if len(crt.Name) >= 52 { + 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", crt.Name), -1) - crt.Name = crt.Name[:validCharIndexes[len(validCharIndexes)-1][1]] + validCharIndexes := regexp.MustCompile(`[a-zA-Z\d]`).FindAllStringIndex(fmt.Sprintf("%.52s", crtName), -1) + crtName = crtName[:validCharIndexes[len(validCharIndexes)-1][1]] } - return fmt.Sprintf("%s-%d", crt.Name, hashF.Sum32()), nil + return fmt.Sprintf("%s-%d", crtName, hashF.Sum32()), nil } From 98ca10b5ba99e7ef6bbdc445ffa66c280e944ca8 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Thu, 16 Jan 2020 12:28:15 +0100 Subject: [PATCH 4/4] Update bazel Signed-off-by: Maartje Eyskens --- pkg/api/util/BUILD.bazel | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/pkg/api/util/BUILD.bazel b/pkg/api/util/BUILD.bazel index 7dac105fb..8dc2d671b 100644 --- a/pkg/api/util/BUILD.bazel +++ b/pkg/api/util/BUILD.bazel @@ -1,4 +1,4 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", @@ -33,3 +33,14 @@ filegroup( tags = ["automanaged"], visibility = ["//visibility:public"], ) + +go_test( + name = "go_default_test", + srcs = ["names_test.go"], + embed = [":go_default_library"], + deps = [ + "//pkg/apis/certmanager/v1alpha2:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_apimachinery//pkg/util/validation:go_default_library", + ], +)