From 3913fa981fcf9dbb48488c6edd8e0d30736457fd Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 12:02:26 +0100 Subject: [PATCH 1/6] Add output about Order of Certificate Signed-off-by: Haoxiang Zhou --- cmd/ctl/pkg/status/certificate/BUILD.bazel | 1 + cmd/ctl/pkg/status/certificate/certificate.go | 46 +++++++++++- cmd/ctl/pkg/status/certificate/types.go | 73 ++++++++++++++++++- 3 files changed, 114 insertions(+), 6 deletions(-) diff --git a/cmd/ctl/pkg/status/certificate/BUILD.bazel b/cmd/ctl/pkg/status/certificate/BUILD.bazel index e1acd28cd..feda42069 100644 --- a/cmd/ctl/pkg/status/certificate/BUILD.bazel +++ b/cmd/ctl/pkg/status/certificate/BUILD.bazel @@ -10,6 +10,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//cmd/ctl/pkg/status/util:go_default_library", + "//pkg/apis/acme/v1beta1:go_default_library", "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/client/clientset/versioned:go_default_library", "//pkg/ctl:go_default_library", diff --git a/cmd/ctl/pkg/status/certificate/certificate.go b/cmd/ctl/pkg/status/certificate/certificate.go index d033f8431..3a353df9a 100644 --- a/cmd/ctl/pkg/status/certificate/certificate.go +++ b/cmd/ctl/pkg/status/certificate/certificate.go @@ -33,6 +33,7 @@ import ( "k8s.io/kubectl/pkg/util/i18n" "k8s.io/kubectl/pkg/util/templates" + cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1beta1" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" cmclient "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" "github.com/jetstack/cert-manager/pkg/ctl" @@ -149,8 +150,7 @@ func (o *Options) Run(args []string) error { req, reqErr := findMatchingCR(o.CMClient, ctx, crt) if reqErr != nil { reqErr = fmt.Errorf("error when finding CertificateRequest: %w\n", reqErr) - } - if req == nil { + } else if req == nil { reqErr = errors.New("No CertificateRequest found for this Certificate\n") } @@ -195,6 +195,22 @@ func (o *Options) Run(args []string) error { status = status.withClusterIssuer(clusterIssuer, issuerErr) } + // Nothing to output about Order and Challenge if no CR, stop early + if req == nil { + fmt.Fprintf(o.Out, status.String()) + return nil + } + + // Get Order + order, orderErr := findMatchingOrder(o.CMClient, ctx, req) + if orderErr != nil { + orderErr = fmt.Errorf("error when finding Order: %w\n", orderErr) + } else if order == nil { + orderErr = errors.New("No Order found for this Certificate\n") + } + + status.withOrder(order, orderErr) + fmt.Fprintf(o.Out, status.String()) return nil @@ -253,3 +269,29 @@ func findMatchingCR(cmClient cmclient.Interface, ctx context.Context, crt *cmapi return nil, errors.New("found multiple certificate requests with expected revision and owner") } } + +// findMatchingOrder tries to find an Order that is owned by req. +// If none found returns nil +// If one found returns the Order +// If multiple found or error occurs when listing Orders, returns error +func findMatchingOrder(cmClient cmclient.Interface, ctx context.Context, req *cmapi.CertificateRequest) (*cmacme.Order, error) { + orders, err := cmClient.AcmeV1beta1().Orders(req.Namespace).List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, err + } + + possibleMatches := []*cmacme.Order{} + for _, order := range orders.Items { + if predicate.ResourceOwnedBy(req)(&order) { + possibleMatches = append(possibleMatches, order.DeepCopy()) + } + } + + if len(possibleMatches) < 1 { + return nil, nil + } else if len(possibleMatches) == 1 { + return possibleMatches[0], nil + } else { + return nil, fmt.Errorf("found multiple orders owned by CertificateRequest %s", req.Name) + } +} diff --git a/cmd/ctl/pkg/status/certificate/types.go b/cmd/ctl/pkg/status/certificate/types.go index 2c1c164f5..904ec3805 100644 --- a/cmd/ctl/pkg/status/certificate/types.go +++ b/cmd/ctl/pkg/status/certificate/types.go @@ -21,6 +21,7 @@ import ( "crypto/x509" "encoding/hex" "fmt" + cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1beta1" "math/big" "strings" @@ -58,6 +59,8 @@ type CertificateStatus struct { SecretStatus *SecretStatus CRStatus *CRStatus + + OrderStatus *OrderStatus } type IssuerStatus struct { @@ -114,6 +117,22 @@ type CRStatus struct { Events *v1.EventList } +type OrderStatus struct { + // If Error is not nil, there was a problem getting the status of the Order resource, + // so the rest of the fields is unusable + Error error + // Name of the Order resource + Name string + // State of Order resource + State cmacme.State + // Reason why the Order resource is in its State + Reason string + // What authorizations must be completed to validate the DNS names specified on the Order + Authorizations []cmacme.ACMEAuthorization + // Time the Order failed + FailureTime *metav1.Time +} + func newCertificateStatusFromCert(crt *cmapiv1alpha2.Certificate) *CertificateStatus { if crt == nil { return nil @@ -192,8 +211,22 @@ func (status *CertificateStatus) withCR(req *cmapiv1alpha2.CertificateRequest, e if req == nil { return status } - status.Events = events - status.CRStatus = &CRStatus{Name: req.Name, Namespace: req.Namespace, Conditions: req.Status.Conditions} + status.CRStatus = &CRStatus{Name: req.Name, Namespace: req.Namespace, Conditions: req.Status.Conditions, Events: events} + return status +} + +func (status *CertificateStatus) withOrder(order *cmacme.Order, err error) *CertificateStatus { + if err != nil { + status.OrderStatus = &OrderStatus{Error: err} + return status + } + if order == nil { + return status + } + + status.OrderStatus = &OrderStatus{Name: order.Name, State: order.Status.State, + Reason: order.Status.Reason, Authorizations: order.Status.Authorizations, + FailureTime: order.Status.FailureTime} return status } @@ -224,8 +257,6 @@ func (status *CertificateStatus) String() string { output += buf.String() buf.Reset() - if status.IssuerStatus == nil { - } output += status.IssuerStatus.String() output += status.SecretStatus.String() @@ -235,6 +266,10 @@ func (status *CertificateStatus) String() string { output += status.CRStatus.String() + if status.OrderStatus != nil { + output += status.OrderStatus.String() + } + return output } @@ -372,3 +407,33 @@ func (crStatus *CRStatus) String() string { buf.Reset() return infos } + +// String returns the information about the status of a CR as a string to be printed as output +func (orderStatus *OrderStatus) String() string { + if orderStatus.Error != nil { + return orderStatus.Error.Error() + } + + output := "Order:\n" + output += fmt.Sprintf(" Name: %s\n", orderStatus.Name) + output += fmt.Sprintf(" State: %s, Reason: %s\n", orderStatus.State, orderStatus.Reason) + authString := "" + for _, auth := range orderStatus.Authorizations { + wildcardString := "nil (bool pointer not set)" + if auth.Wildcard != nil { + wildcardString = fmt.Sprintf("%t", *auth.Wildcard) + } + authString += fmt.Sprintf(" URL: %s, Identifier: %s, Initial State: %s, Wildcard: %s\n", auth.URL, auth.Identifier, auth.InitialState, wildcardString) + } + if authString == "" { + output += " No Authorizations for this Order\n" + } else { + output += " Authorizations:\n" + output += authString + } + if orderStatus.FailureTime != nil { + output += fmt.Sprintf(" FailureTime: %s\n", formatTimeString(orderStatus.FailureTime)) + } + + return output +} From fc8b123593117df48984aabb758b542e1483f60f Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 12:03:36 +0100 Subject: [PATCH 2/6] Update tests and compare strings directly as well if regex matches Signed-off-by: Haoxiang Zhou --- test/integration/ctl/BUILD.bazel | 1 + .../ctl/ctl_status_certificate_test.go | 77 ++++++++++++++++--- test/unit/gen/order.go | 6 ++ 3 files changed, 75 insertions(+), 9 deletions(-) diff --git a/test/integration/ctl/BUILD.bazel b/test/integration/ctl/BUILD.bazel index 3067d52cf..879a1ee87 100644 --- a/test/integration/ctl/BUILD.bazel +++ b/test/integration/ctl/BUILD.bazel @@ -15,6 +15,7 @@ go_test( "//cmd/ctl/pkg/renew:go_default_library", "//cmd/ctl/pkg/status/certificate:go_default_library", "//pkg/api/util:go_default_library", + "//pkg/apis/acme/v1alpha2:go_default_library", "//pkg/apis/certmanager/v1alpha2:go_default_library", "//pkg/apis/meta/v1:go_default_library", "//pkg/client/clientset/versioned:go_default_library", diff --git a/test/integration/ctl/ctl_status_certificate_test.go b/test/integration/ctl/ctl_status_certificate_test.go index 1782e34c9..14ece7ef2 100644 --- a/test/integration/ctl/ctl_status_certificate_test.go +++ b/test/integration/ctl/ctl_status_certificate_test.go @@ -31,6 +31,7 @@ import ( statuscertcmd "github.com/jetstack/cert-manager/cmd/ctl/pkg/status/certificate" apiutil "github.com/jetstack/cert-manager/pkg/api/util" + cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" @@ -101,6 +102,7 @@ MA6koCR/K23HZfML8vT6lcHvQJp9XXaHRIe9NX/M/2f6VpfO7JjKWLou5k5a issuer *cmapi.Issuer clusterIssuer *cmapi.ClusterIssuer secret *v1.Secret + order *cmacme.Order expErr bool expOutput string @@ -156,6 +158,10 @@ No CertificateRequest found for this Certificate`, secret: gen.Secret("existing-tls-secret", gen.SetSecretNamespace(ns1), gen.SetSecretData(map[string][]byte{"tls.crt": tlsCrt})), + order: gen.Order("example-order", + gen.SetOrderNamespace(ns1), + gen.SetOrderCsr([]byte("dummyCSR")), + gen.SetOrderDNSNames("www.example.com")), expErr: false, expOutput: `Name: testcrt-2 Namespace: testns-1 @@ -191,7 +197,11 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: `, + Events: +Order: + Name: example-order + State: , Reason: + No Authorizations for this Order`, }, "certificate issued and renewal in progress without Issuer": { certificate: gen.Certificate(crt3Name, @@ -229,7 +239,8 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: `, + Events: +No Order found for this Certificate`, }, "certificate issued and renewal in progress without ClusterIssuer": { certificate: gen.Certificate(crt4Name, @@ -267,7 +278,8 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: `, + Events: +No Order found for this Certificate`, }, } @@ -283,6 +295,7 @@ CertificateRequest: t.Fatal(err) } + // Set up related resources if test.req != nil { err = createCROwnedByCrt(t, cmCl, ctx, crt, test.req, test.reqStatus) if err != nil { @@ -310,6 +323,17 @@ CertificateRequest: } } + if test.order != nil { + createdReq, err := cmCl.CertmanagerV1alpha2().CertificateRequests(test.req.Namespace).Get(ctx, test.req.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + err = createOrderOwnedByCR(t, cmCl, ctx, createdReq, test.order) + if err != nil { + t.Fatal(err) + } + } + // Options to run status command streams, _, outBuf, _ := genericclioptions.NewTestIOStreams() opts := &statuscertcmd.Options{ @@ -337,10 +361,27 @@ CertificateRequest: if err != nil { t.Error(err) } + dmp := diffmatchpatch.New() if !match { - dmp := diffmatchpatch.New() diffs := dmp.DiffMain(strings.TrimSpace(test.expOutput), strings.TrimSpace(outBuf.String()), false) - t.Errorf("got unexpected ouput, diff (ignoring the regex for creation time):\n%s\n\n expected: %s\n\n got: %s", dmp.DiffPrettyText(diffs), test.expOutput, outBuf.String()) + t.Errorf("got unexpected output, diff (ignoring the regex for creation time):\n%s\n\n expected: \n%s\n\n got: \n%s", dmp.DiffPrettyText(diffs), test.expOutput, outBuf.String()) + } else { + // Regex matches, so the Creation time is set, but double check string + // as any extraneous output before and after the expected output as defined get ignored + + // Replace the regex for time with actual Creation time output and compare strings + output := outBuf.String() + s := strings.Index(output, "Created at: ") + s += len("Created at: ") + e := strings.Index(output[s:], "\n") + s + createTime := output[s:e] + timeRegex := "([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\\.[0-9]+)?(([Zz])|([\\+|\\-]([01][0-9]|2[0-3]):[0-5][0-9]))" + newExpOutput := strings.Replace(test.expOutput, timeRegex, createTime, 1) + + if strings.TrimSpace(newExpOutput) != strings.TrimSpace(outBuf.String()) { + diffs := dmp.DiffMain(strings.TrimSpace(newExpOutput), strings.TrimSpace(outBuf.String()), false) + t.Errorf("the regex matching passed, but after replacing time regex with the actual creation time, got unexpected output, diff:\n%s\n\n expected: \n%s\n\n got: \n%s", dmp.DiffPrettyText(diffs), newExpOutput, outBuf.String()) + } } }) } @@ -360,6 +401,7 @@ func setCertificateStatus(cmCl versioned.Interface, crt *cmapi.Certificate, func createCROwnedByCrt(t *testing.T, cmCl versioned.Interface, ctx context.Context, crt *cmapi.Certificate, req *cmapi.CertificateRequest, reqStatus *cmapi.CertificateRequestStatus) error { + req, err := cmCl.CertmanagerV1alpha2().CertificateRequests(crt.Namespace).Create(ctx, req, metav1.CreateOptions{}) if err != nil { return err @@ -373,10 +415,27 @@ func createCROwnedByCrt(t *testing.T, cmCl versioned.Interface, ctx context.Cont if reqStatus != nil { req.Status.Conditions = reqStatus.Conditions - } - req, err = cmCl.CertmanagerV1alpha2().CertificateRequests(crt.Namespace).UpdateStatus(ctx, req, metav1.UpdateOptions{}) - if err != nil { - t.Errorf("Update Err: %v", err) + req, err = cmCl.CertmanagerV1alpha2().CertificateRequests(crt.Namespace).UpdateStatus(ctx, req, metav1.UpdateOptions{}) + if err != nil { + t.Errorf("Update Err: %v", err) + } } return nil } + +func createOrderOwnedByCR(t *testing.T, cmCl versioned.Interface, ctx context.Context, + req *cmapi.CertificateRequest, order *cmacme.Order) error { + + order, err := cmCl.AcmeV1alpha2().Orders(req.Namespace).Create(ctx, order, metav1.CreateOptions{}) + if err != nil { + return err + } + + order.OwnerReferences = append(order.OwnerReferences, *metav1.NewControllerRef(req, cmapi.SchemeGroupVersion.WithKind("CertificateRequest"))) + order, err = cmCl.AcmeV1alpha2().Orders(req.Namespace).Update(ctx, order, metav1.UpdateOptions{}) + if err != nil { + t.Errorf("Update Err: %v", err) + } + + return nil +} diff --git a/test/unit/gen/order.go b/test/unit/gen/order.go index 5121cdfa4..c918e72bc 100644 --- a/test/unit/gen/order.go +++ b/test/unit/gen/order.go @@ -95,3 +95,9 @@ func SetOrderNamespace(namespace string) OrderModifier { order.ObjectMeta.Namespace = namespace } } + +func SetOrderCsr(csr []byte) OrderModifier { + return func(order *cmacme.Order) { + order.Spec.CSR = csr + } +} From 196959a05f78db9ad6f3f23c2e5a9e44ac5343d2 Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 14:00:02 +0100 Subject: [PATCH 3/6] No output about Orders if non-ACME Issuer Signed-off-by: Haoxiang Zhou --- cmd/ctl/pkg/status/certificate/types.go | 14 ++++++++++---- .../integration/ctl/ctl_status_certificate_test.go | 9 ++++----- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/cmd/ctl/pkg/status/certificate/types.go b/cmd/ctl/pkg/status/certificate/types.go index 904ec3805..5962273d1 100644 --- a/cmd/ctl/pkg/status/certificate/types.go +++ b/cmd/ctl/pkg/status/certificate/types.go @@ -21,7 +21,6 @@ import ( "crypto/x509" "encoding/hex" "fmt" - cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1beta1" "math/big" "strings" @@ -30,6 +29,7 @@ import ( "k8s.io/kubectl/pkg/describe" "github.com/jetstack/cert-manager/cmd/ctl/pkg/status/util" + cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1beta1" cmapiv1alpha2 "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2" "github.com/jetstack/cert-manager/pkg/util/pki" ) @@ -73,6 +73,9 @@ type IssuerStatus struct { Kind string // Conditions of Issuer/ClusterIssuer resource Conditions []cmapiv1alpha2.IssuerCondition + // boolean indicating if Issuer/ClusterIssuer is a ACME Issuer/ClusterIssuer + // Defaults to false even Error is not nil + IsACME bool } type SecretStatus struct { @@ -156,7 +159,8 @@ func (status *CertificateStatus) withIssuer(issuer *cmapiv1alpha2.Issuer, err er if issuer == nil { return status } - status.IssuerStatus = &IssuerStatus{Name: issuer.Name, Kind: "Issuer", Conditions: issuer.Status.Conditions} + status.IssuerStatus = &IssuerStatus{Name: issuer.Name, Kind: "Issuer", + Conditions: issuer.Status.Conditions, IsACME: issuer.Spec.ACME != nil} return status } @@ -168,7 +172,8 @@ func (status *CertificateStatus) withClusterIssuer(clusterIssuer *cmapiv1alpha2. if clusterIssuer == nil { return status } - status.IssuerStatus = &IssuerStatus{Name: clusterIssuer.Name, Kind: "ClusterIssuer", Conditions: clusterIssuer.Status.Conditions} + status.IssuerStatus = &IssuerStatus{Name: clusterIssuer.Name, Kind: "ClusterIssuer", + Conditions: clusterIssuer.Status.Conditions, IsACME: clusterIssuer.Spec.ACME != nil} return status } @@ -266,7 +271,8 @@ func (status *CertificateStatus) String() string { output += status.CRStatus.String() - if status.OrderStatus != nil { + // Do not print anything about Order if not ACME Issuer to avoid confusion + if status.OrderStatus != nil && status.IssuerStatus.IsACME { output += status.OrderStatus.String() } diff --git a/test/integration/ctl/ctl_status_certificate_test.go b/test/integration/ctl/ctl_status_certificate_test.go index 14ece7ef2..08553d823 100644 --- a/test/integration/ctl/ctl_status_certificate_test.go +++ b/test/integration/ctl/ctl_status_certificate_test.go @@ -154,7 +154,8 @@ No CertificateRequest found for this Certificate`, gen.SetCertificateRequestCSR([]byte("dummyCSR"))), reqStatus: &cmapi.CertificateRequestStatus{Conditions: []cmapi.CertificateRequestCondition{reqNotReadyCond}}, issuer: gen.Issuer("letsencrypt-prod", - gen.SetIssuerNamespace(ns1)), + gen.SetIssuerNamespace(ns1), + gen.SetIssuerACME(cmacme.ACMEIssuer{})), secret: gen.Secret("existing-tls-secret", gen.SetSecretNamespace(ns1), gen.SetSecretData(map[string][]byte{"tls.crt": tlsCrt})), @@ -239,8 +240,7 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: -No Order found for this Certificate`, + Events: `, }, "certificate issued and renewal in progress without ClusterIssuer": { certificate: gen.Certificate(crt4Name, @@ -278,8 +278,7 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: -No Order found for this Certificate`, + Events: `, }, } From 8c9eb43102d4a47fcf6f28ece98d7cc7ec645f03 Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 14:48:05 +0100 Subject: [PATCH 4/6] Clean up Signed-off-by: Haoxiang Zhou --- cmd/ctl/pkg/status/certificate/certificate.go | 25 ++++++++----------- cmd/ctl/pkg/status/certificate/types.go | 1 - 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/cmd/ctl/pkg/status/certificate/certificate.go b/cmd/ctl/pkg/status/certificate/certificate.go index 3a353df9a..7f4f3e58a 100644 --- a/cmd/ctl/pkg/status/certificate/certificate.go +++ b/cmd/ctl/pkg/status/certificate/certificate.go @@ -42,7 +42,7 @@ import ( var ( long = templates.LongDesc(i18n.T(` -Get details about the current status of a cert-manager Certificate resource, including information on related resources like CertificateRequest.`)) +Get details about the current status of a cert-manager Certificate resource, including information on related resources like CertificateRequest or Order.`)) example = templates.Examples(i18n.T(` # Query status of Certificate with name 'my-crt' in namespace 'my-namespace' @@ -195,22 +195,19 @@ func (o *Options) Run(args []string) error { status = status.withClusterIssuer(clusterIssuer, issuerErr) } - // Nothing to output about Order and Challenge if no CR, stop early - if req == nil { - fmt.Fprintf(o.Out, status.String()) - return nil - } + // Nothing to output about Order and Challenge if no CR + if req != nil { + // Get Order + order, orderErr := findMatchingOrder(o.CMClient, ctx, req) + if orderErr != nil { + orderErr = fmt.Errorf("error when finding Order: %w\n", orderErr) + } else if order == nil { + orderErr = errors.New("No Order found for this Certificate\n") + } - // Get Order - order, orderErr := findMatchingOrder(o.CMClient, ctx, req) - if orderErr != nil { - orderErr = fmt.Errorf("error when finding Order: %w\n", orderErr) - } else if order == nil { - orderErr = errors.New("No Order found for this Certificate\n") + status.withOrder(order, orderErr) } - status.withOrder(order, orderErr) - fmt.Fprintf(o.Out, status.String()) return nil diff --git a/cmd/ctl/pkg/status/certificate/types.go b/cmd/ctl/pkg/status/certificate/types.go index 5962273d1..d5f0fae45 100644 --- a/cmd/ctl/pkg/status/certificate/types.go +++ b/cmd/ctl/pkg/status/certificate/types.go @@ -408,7 +408,6 @@ func (crStatus *CRStatus) String() string { prefixWriter := describe.NewPrefixWriter(tabWriter) util.DescribeEvents(crStatus.Events, prefixWriter, 1) tabWriter.Flush() - fmt.Println(buf.Bytes()) infos += buf.String() buf.Reset() return infos From 43a83e71d774c9760c1dabb990fd4f1c8fcd22cc Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 16:06:01 +0100 Subject: [PATCH 5/6] Use line anchors for exact regex matching instead of string comparison Signed-off-by: Haoxiang Zhou --- .../ctl/ctl_status_certificate_test.go | 35 +++++-------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/test/integration/ctl/ctl_status_certificate_test.go b/test/integration/ctl/ctl_status_certificate_test.go index 08553d823..3d3c53b0b 100644 --- a/test/integration/ctl/ctl_status_certificate_test.go +++ b/test/integration/ctl/ctl_status_certificate_test.go @@ -119,7 +119,7 @@ MA6koCR/K23HZfML8vT6lcHvQJp9XXaHRIe9NX/M/2f6VpfO7JjKWLou5k5a inputNamespace: ns1, clusterIssuer: gen.ClusterIssuer("letsencrypt-prod"), expErr: false, - expOutput: `Name: testcrt-1 + expOutput: `^Name: testcrt-1 Namespace: testns-1 Created at: ([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9])) Conditions: @@ -136,7 +136,7 @@ error when finding Secret "example-tls": secrets "example-tls" not found Not Before: Not After: 2020-09-16T09:26:18Z Renewal Time: -No CertificateRequest found for this Certificate`, +No CertificateRequest found for this Certificate$`, }, "certificate issued and renewal in progress with Issuer": { certificate: gen.Certificate(crt2Name, @@ -164,7 +164,7 @@ No CertificateRequest found for this Certificate`, gen.SetOrderCsr([]byte("dummyCSR")), gen.SetOrderDNSNames("www.example.com")), expErr: false, - expOutput: `Name: testcrt-2 + expOutput: `^Name: testcrt-2 Namespace: testns-1 Created at: ([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9])) Conditions: @@ -202,7 +202,7 @@ CertificateRequest: Order: Name: example-order State: , Reason: - No Authorizations for this Order`, + No Authorizations for this Order$`, }, "certificate issued and renewal in progress without Issuer": { certificate: gen.Certificate(crt3Name, @@ -221,7 +221,7 @@ Order: reqStatus: &cmapi.CertificateRequestStatus{Conditions: []cmapi.CertificateRequestCondition{reqNotReadyCond}}, issuer: nil, expErr: false, - expOutput: `Name: testcrt-3 + expOutput: `^Name: testcrt-3 Namespace: testns-1 Created at: ([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9])) Conditions: @@ -240,7 +240,7 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: `, + Events: $`, }, "certificate issued and renewal in progress without ClusterIssuer": { certificate: gen.Certificate(crt4Name, @@ -259,7 +259,7 @@ CertificateRequest: reqStatus: &cmapi.CertificateRequestStatus{Conditions: []cmapi.CertificateRequestCondition{reqNotReadyCond}}, issuer: nil, expErr: false, - expOutput: `Name: testcrt-4 + expOutput: `^Name: testcrt-4 Namespace: testns-1 Created at: ([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\.[0-9]+)?(([Zz])|([\+|\-]([01][0-9]|2[0-3]):[0-5][0-9])) Conditions: @@ -278,7 +278,7 @@ CertificateRequest: Namespace: testns-1 Conditions: Ready: False, Reason: Pending, Message: Waiting on certificate issuance from order default/example-order: "pending" - Events: `, + Events: $`, }, } @@ -363,24 +363,7 @@ CertificateRequest: dmp := diffmatchpatch.New() if !match { diffs := dmp.DiffMain(strings.TrimSpace(test.expOutput), strings.TrimSpace(outBuf.String()), false) - t.Errorf("got unexpected output, diff (ignoring the regex for creation time):\n%s\n\n expected: \n%s\n\n got: \n%s", dmp.DiffPrettyText(diffs), test.expOutput, outBuf.String()) - } else { - // Regex matches, so the Creation time is set, but double check string - // as any extraneous output before and after the expected output as defined get ignored - - // Replace the regex for time with actual Creation time output and compare strings - output := outBuf.String() - s := strings.Index(output, "Created at: ") - s += len("Created at: ") - e := strings.Index(output[s:], "\n") + s - createTime := output[s:e] - timeRegex := "([0-9]+)-(0[1-9]|1[012])-(0[1-9]|[12][0-9]|3[01])[Tt]([01][0-9]|2[0-3]):([0-5][0-9]):([0-5][0-9]|60)(\\.[0-9]+)?(([Zz])|([\\+|\\-]([01][0-9]|2[0-3]):[0-5][0-9]))" - newExpOutput := strings.Replace(test.expOutput, timeRegex, createTime, 1) - - if strings.TrimSpace(newExpOutput) != strings.TrimSpace(outBuf.String()) { - diffs := dmp.DiffMain(strings.TrimSpace(newExpOutput), strings.TrimSpace(outBuf.String()), false) - t.Errorf("the regex matching passed, but after replacing time regex with the actual creation time, got unexpected output, diff:\n%s\n\n expected: \n%s\n\n got: \n%s", dmp.DiffPrettyText(diffs), newExpOutput, outBuf.String()) - } + t.Errorf("got unexpected output, diff (ignoring line anchors ^ and $ and regex for creation time):\n%s\n\n expected: \n%s\n\n got: \n%s", dmp.DiffPrettyText(diffs), test.expOutput, outBuf.String()) } }) } From a1f7a1fcc3b2d1c0cec9c19669c2e396c0f47574 Mon Sep 17 00:00:00 2001 From: Haoxiang Zhou Date: Thu, 6 Aug 2020 18:21:48 +0100 Subject: [PATCH 6/6] Review implementation Signed-off-by: Haoxiang Zhou --- cmd/ctl/pkg/status/certificate/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/ctl/pkg/status/certificate/types.go b/cmd/ctl/pkg/status/certificate/types.go index d5f0fae45..64bc7153d 100644 --- a/cmd/ctl/pkg/status/certificate/types.go +++ b/cmd/ctl/pkg/status/certificate/types.go @@ -74,7 +74,7 @@ type IssuerStatus struct { // Conditions of Issuer/ClusterIssuer resource Conditions []cmapiv1alpha2.IssuerCondition // boolean indicating if Issuer/ClusterIssuer is a ACME Issuer/ClusterIssuer - // Defaults to false even Error is not nil + // Defaults to false even when Error is not nil IsACME bool }