From 8a6e07f85c077f25708c6f52f101b783e30dffbe Mon Sep 17 00:00:00 2001 From: JasonPowr Date: Fri, 6 Sep 2024 12:32:31 +0100 Subject: [PATCH] Fix deletion after upgrade --- api/v1alpha1/securesign_types.go | 6 +++--- api/v1alpha1/timestampauthority_types.go | 1 + api/v1alpha1/timestampauthority_types_test.go | 17 +++++++++++++++++ api/v1alpha1/zz_generated.deepcopy.go | 6 +++++- .../manifests/rhtas.redhat.com_securesigns.yaml | 4 ++++ .../rhtas.redhat.com_timestampauthorities.yaml | 8 ++++++++ .../crd/bases/rhtas.redhat.com_securesigns.yaml | 4 ++++ .../rhtas.redhat.com_timestampauthorities.yaml | 8 ++++++++ .../controller/securesign/actions/ensure_tsa.go | 2 +- test/e2e/byodb_test.go | 2 +- test/e2e/common_install_test.go | 2 +- test/e2e/key_autodiscovery_test.go | 2 +- test/e2e/provided_certs_test.go | 2 +- test/e2e/update/suite_test.go | 2 +- test/e2e/upgrade_test.go | 9 ++++++++- 15 files changed, 64 insertions(+), 11 deletions(-) diff --git a/api/v1alpha1/securesign_types.go b/api/v1alpha1/securesign_types.go index c9e1991e4..9d1a79bfa 100644 --- a/api/v1alpha1/securesign_types.go +++ b/api/v1alpha1/securesign_types.go @@ -30,9 +30,9 @@ type SecuresignSpec struct { Fulcio FulcioSpec `json:"fulcio,omitempty"` Trillian TrillianSpec `json:"trillian,omitempty"` //+kubebuilder:default:={keys:{{name: rekor.pub},{name: ctfe.pub},{name: fulcio_v1.crt.pem},{name: tsa.certchain.pem}}} - Tuf TufSpec `json:"tuf,omitempty"` - Ctlog CTlogSpec `json:"ctlog,omitempty"` - TimestampAuthority TimestampAuthoritySpec `json:"tsa,omitempty"` + Tuf TufSpec `json:"tuf,omitempty"` + Ctlog CTlogSpec `json:"ctlog,omitempty"` + TimestampAuthority *TimestampAuthoritySpec `json:"tsa,omitempty"` } // SecuresignStatus defines the observed state of Securesign diff --git a/api/v1alpha1/timestampauthority_types.go b/api/v1alpha1/timestampauthority_types.go index 5551a2136..cdb7a8a8c 100644 --- a/api/v1alpha1/timestampauthority_types.go +++ b/api/v1alpha1/timestampauthority_types.go @@ -61,6 +61,7 @@ type TimestampAuthoritySigner struct { // +kubebuilder:validation:XValidation:rule="(!has(self.rootCA) && !has(self.leafCA)) || (has(self.rootCA.privateKeyRef) == has(self.leafCA.privateKeyRef))",message="must provide private keys for both root and leaf certificate authorities" // +kubebuilder:validation:XValidation:rule=(has(self.certificateChainRef) || self.rootCA.organizationName != ""),message=organizationName cannot be empty for root certificate authority // +kubebuilder:validation:XValidation:rule=(has(self.certificateChainRef) || self.leafCA.organizationName != ""),message=organizationName cannot be empty for leaf certificate authority +// +kubebuilder:validation:XValidation:rule=(has(self.certificateChainRef) || self.intermediateCA[0].organizationName != ""),message="organizationName cannot be empty for intermediate certificate authority, please make sure all are in place" type CertificateChain struct { //Reference to the certificate chain //+optional diff --git a/api/v1alpha1/timestampauthority_types_test.go b/api/v1alpha1/timestampauthority_types_test.go index 339748c6e..37bd7c273 100644 --- a/api/v1alpha1/timestampauthority_types_test.go +++ b/api/v1alpha1/timestampauthority_types_test.go @@ -57,6 +57,15 @@ var _ = Describe("TSA", func() { Expect(k8sClient.Get(context.Background(), getKey(created), created)).ToNot(Succeed()) }) + It("can be empty", func() { + created := Securesign{ + Spec: SecuresignSpec{ + TimestampAuthority: nil, + }, + } + Expect(apierrors.IsInvalid(k8sClient.Create(context.Background(), &created))).To(BeFalse()) + }) + When("changing external access setting", func() { It("enabled false->true", func() { created := generateTSAObject("tsa-access-1") @@ -126,6 +135,14 @@ var _ = Describe("TSA", func() { To(MatchError(ContainSubstring("organizationName cannot be empty for root certificate authority"))) }) + It("missing org name for intermediate CA", func() { + invalidObject := generateTSAObject("missing-org-name") + invalidObject.Spec.Signer.CertificateChain.IntermediateCA[0].OrganizationName = "" + Expect(apierrors.IsInvalid(k8sClient.Create(context.Background(), invalidObject))).To(BeTrue()) + Expect(k8sClient.Create(context.Background(), invalidObject)). + To(MatchError(ContainSubstring("organizationName cannot be empty for intermediate certificate authority, please make sure all are in place"))) + }) + It("missing org name for leaf CA", func() { invalidObject := generateTSAObject("missing-org-name") invalidObject.Spec.Signer.CertificateChain.LeafCA.OrganizationName = "" diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index c22c5ddad..8d1642b6f 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -947,7 +947,11 @@ func (in *SecuresignSpec) DeepCopyInto(out *SecuresignSpec) { in.Trillian.DeepCopyInto(&out.Trillian) in.Tuf.DeepCopyInto(&out.Tuf) in.Ctlog.DeepCopyInto(&out.Ctlog) - in.TimestampAuthority.DeepCopyInto(&out.TimestampAuthority) + if in.TimestampAuthority != nil { + in, out := &in.TimestampAuthority, &out.TimestampAuthority + *out = new(TimestampAuthoritySpec) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SecuresignSpec. diff --git a/bundle/manifests/rhtas.redhat.com_securesigns.yaml b/bundle/manifests/rhtas.redhat.com_securesigns.yaml index e09499ed1..1eea0a2dd 100644 --- a/bundle/manifests/rhtas.redhat.com_securesigns.yaml +++ b/bundle/manifests/rhtas.redhat.com_securesigns.yaml @@ -1161,6 +1161,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate + certificate authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: diff --git a/bundle/manifests/rhtas.redhat.com_timestampauthorities.yaml b/bundle/manifests/rhtas.redhat.com_timestampauthorities.yaml index 7a74903a4..b19d442dd 100644 --- a/bundle/manifests/rhtas.redhat.com_timestampauthorities.yaml +++ b/bundle/manifests/rhtas.redhat.com_timestampauthorities.yaml @@ -347,6 +347,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate certificate + authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: @@ -1075,6 +1079,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate certificate + authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: diff --git a/config/crd/bases/rhtas.redhat.com_securesigns.yaml b/config/crd/bases/rhtas.redhat.com_securesigns.yaml index d42cb6dcb..16193a723 100644 --- a/config/crd/bases/rhtas.redhat.com_securesigns.yaml +++ b/config/crd/bases/rhtas.redhat.com_securesigns.yaml @@ -1161,6 +1161,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate + certificate authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: diff --git a/config/crd/bases/rhtas.redhat.com_timestampauthorities.yaml b/config/crd/bases/rhtas.redhat.com_timestampauthorities.yaml index e7712506f..363236639 100644 --- a/config/crd/bases/rhtas.redhat.com_timestampauthorities.yaml +++ b/config/crd/bases/rhtas.redhat.com_timestampauthorities.yaml @@ -347,6 +347,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate certificate + authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: @@ -1075,6 +1079,10 @@ spec: authority rule: (has(self.certificateChainRef) || self.leafCA.organizationName != "") + - message: organizationName cannot be empty for intermediate certificate + authority, please make sure all are in place + rule: (has(self.certificateChainRef) || self.intermediateCA[0].organizationName + != "") file: description: Configuration for file-based signer properties: diff --git a/internal/controller/securesign/actions/ensure_tsa.go b/internal/controller/securesign/actions/ensure_tsa.go index ac5b8bf4d..940f52a1f 100644 --- a/internal/controller/securesign/actions/ensure_tsa.go +++ b/internal/controller/securesign/actions/ensure_tsa.go @@ -49,7 +49,7 @@ func (i tsaAction) Handle(ctx context.Context, instance *rhtasv1alpha1.Securesig }) return i.StatusUpdate(ctx, instance) } - tsa.Spec = instance.Spec.TimestampAuthority + tsa.Spec = *instance.Spec.TimestampAuthority if err = controllerutil.SetControllerReference(instance, tsa, i.Client.Scheme()); err != nil { return i.Failed(err) diff --git a/test/e2e/byodb_test.go b/test/e2e/byodb_test.go index 490cc2235..22bf1e58f 100644 --- a/test/e2e/byodb_test.go +++ b/test/e2e/byodb_test.go @@ -85,7 +85,7 @@ var _ = Describe("Securesign install with byodb", Ordered, func() { Name: "my-db", }, }}, - TimestampAuthority: v1alpha1.TimestampAuthoritySpec{ + TimestampAuthority: &v1alpha1.TimestampAuthoritySpec{ ExternalAccess: v1alpha1.ExternalAccess{ Enabled: true, }, diff --git a/test/e2e/common_install_test.go b/test/e2e/common_install_test.go index 886f678e5..1ca1f5398 100644 --- a/test/e2e/common_install_test.go +++ b/test/e2e/common_install_test.go @@ -93,7 +93,7 @@ var _ = Describe("Securesign install with certificate generation", Ordered, func Trillian: v1alpha1.TrillianSpec{Db: v1alpha1.TrillianDB{ Create: ptr.To(true), }}, - TimestampAuthority: v1alpha1.TimestampAuthoritySpec{ + TimestampAuthority: &v1alpha1.TimestampAuthoritySpec{ ExternalAccess: v1alpha1.ExternalAccess{ Enabled: true, }, diff --git a/test/e2e/key_autodiscovery_test.go b/test/e2e/key_autodiscovery_test.go index 5df84e2bd..8e525b801 100644 --- a/test/e2e/key_autodiscovery_test.go +++ b/test/e2e/key_autodiscovery_test.go @@ -128,7 +128,7 @@ var _ = Describe("Securesign key autodiscovery test", Ordered, func() { Retain: ptr.To(false), }, }}, - TimestampAuthority: v1alpha1.TimestampAuthoritySpec{ + TimestampAuthority: &v1alpha1.TimestampAuthoritySpec{ ExternalAccess: v1alpha1.ExternalAccess{ Enabled: true, }, diff --git a/test/e2e/provided_certs_test.go b/test/e2e/provided_certs_test.go index b5afd405b..b1593d424 100644 --- a/test/e2e/provided_certs_test.go +++ b/test/e2e/provided_certs_test.go @@ -164,7 +164,7 @@ var _ = Describe("Securesign install with provided certs", Ordered, func() { Retain: ptr.To(false), }, }}, - TimestampAuthority: v1alpha1.TimestampAuthoritySpec{ + TimestampAuthority: &v1alpha1.TimestampAuthoritySpec{ ExternalAccess: v1alpha1.ExternalAccess{ Enabled: true, }, diff --git a/test/e2e/update/suite_test.go b/test/e2e/update/suite_test.go index bed2f9134..bc645b9b0 100644 --- a/test/e2e/update/suite_test.go +++ b/test/e2e/update/suite_test.go @@ -83,7 +83,7 @@ func securesignResource(namespace *v1.Namespace) *rhtasv1alpha1.Securesign { Retain: ptr.To(false), }, }}, - TimestampAuthority: rhtasv1alpha1.TimestampAuthoritySpec{ + TimestampAuthority: &rhtasv1alpha1.TimestampAuthoritySpec{ ExternalAccess: rhtasv1alpha1.ExternalAccess{ Enabled: true, }, diff --git a/test/e2e/upgrade_test.go b/test/e2e/upgrade_test.go index a7975cf6a..283c514aa 100644 --- a/test/e2e/upgrade_test.go +++ b/test/e2e/upgrade_test.go @@ -365,7 +365,7 @@ var _ = Describe("Operator upgrade", Ordered, func() { gomega.Expect(s).ToNot(gomega.BeNil()) gomega.Expect(meta.FindStatusCondition(s.GetConditions(), actions.TSACondition).Reason).To(gomega.Equal(constants.NotDefined)) - s.Spec.TimestampAuthority = tasv1alpha.TimestampAuthoritySpec{ + s.Spec.TimestampAuthority = &tasv1alpha.TimestampAuthoritySpec{ ExternalAccess: tasv1alpha.ExternalAccess{ Enabled: true, }, @@ -555,6 +555,13 @@ var _ = Describe("Operator upgrade", Ordered, func() { newImageName, )).To(gomega.Succeed()) }) + + It("Make sure securesign can be deleted after upgrade", func() { + gomega.Eventually(func(g gomega.Gomega) { + s := securesign.Get(ctx, cli, namespace.Name, securesignDeployment.Name)() + gomega.Expect(cli.Delete(ctx, s)).Should(gomega.Succeed()) + }).Should(gomega.Succeed()) + }) }) func findClusterServiceVersion(ctx context.Context, cli runtimeCli.Client, conditions func(version v1alpha1.ClusterServiceVersion) bool, ns string) func() *v1alpha1.ClusterServiceVersion {