From b6bcc3506c0d84baa0c020f6b776a181b931f57a Mon Sep 17 00:00:00 2001 From: "Jose R. Gonzalez" Date: Mon, 18 Dec 2023 10:05:41 -0600 Subject: [PATCH] Ensure all disconnected annotations are accounted for when reviewing guidelines Signed-off-by: Jose R. Gonzalez --- internal/csv/csv.go | 23 +++++++++++++-- internal/csv/csv_test.go | 29 ++++++++++++++++++- .../operator/restricted_network_aware.go | 14 +++++---- 3 files changed, 57 insertions(+), 9 deletions(-) diff --git a/internal/csv/csv.go b/internal/csv/csv.go index f0d5e836..fbc27b7f 100644 --- a/internal/csv/csv.go +++ b/internal/csv/csv.go @@ -24,7 +24,7 @@ const ( CSIAnnotation = "features.operators.openshift.io/csi" ) -// SupportsDisconnected accepts a stringified list of supported features +// SupportsDisconnectedViaInfrastructureFeatures accepts a stringified list of supported features // and returns true if "disconnected" is listed as a supported feature. // // E.g. '["disconnected"]'. @@ -32,7 +32,9 @@ const ( // This is case insensitive, as each infrastructure // is normalized before checking. A failure to unmarshal this structure // returns false. -func SupportsDisconnected(infrastructureFeatures string) bool { +// +// This is a legacy annotation and will be superceded by the DisconnectedAnnotation in the future. +func SupportsDisconnectedViaInfrastructureFeatures(infrastructureFeatures string) bool { var features []string err := json.Unmarshal([]byte(infrastructureFeatures), &features) @@ -49,13 +51,28 @@ func SupportsDisconnected(infrastructureFeatures string) bool { return false } +// SupportsDisconnected accepts the value of the DisconnectedAnnotation and returns whether it is +// set to the exact value of "true", which is the expected value indicating support. For this annotation, +// an explicit value of "false" is treated the same as any other non-true value. +func SupportsDisconnected(annotationValue string) bool { + return annotationValue == "true" +} + // HasInfrastructureFeaturesAnnotation returns true if the infrastructure-features annotation -// exists in the .metadata.annotations block of csv. +// exists in the .metadata.annotations block of csv. This is the legacy annotation +// superceded by the DisconnectedAnnotation func HasInfrastructureFeaturesAnnotation(csv *operatorsv1alpha1.ClusterServiceVersion) bool { _, ok := csv.GetAnnotations()[InfrastructureFeaturesAnnotation] return ok } +// HasDisconnectedAnnotation returns true if the disconnected annotation exists in the +// .metadata.annotations block of csv. +func HasDisconnectedAnnotation(csv *operatorsv1alpha1.ClusterServiceVersion) bool { + _, ok := csv.GetAnnotations()[DisconnectedAnnotation] + return ok +} + // HasRelatedImages returns true if the length of the .spec.relatedImages section of // csv is greater than 0. func HasRelatedImages(csv *operatorsv1alpha1.ClusterServiceVersion) bool { diff --git a/internal/csv/csv_test.go b/internal/csv/csv_test.go index d63b2212..f172720c 100644 --- a/internal/csv/csv_test.go +++ b/internal/csv/csv_test.go @@ -12,10 +12,37 @@ import ( ) var _ = Describe("CSV Functions", func() { + DescribeTable( + "Validating the independent disconnected annotation values", + func(value string, expected bool) { + actual := SupportsDisconnected(value) + Expect(actual).To(Equal(expected)) + }, + Entry("set to \"true\"", "true", true), + Entry("set to \"false\"", "false", false), + Entry("set to \"TRUE\"", "TRUE", false), + Entry("set to a random string", "abc", false), + ) + + When("Checking CSVs for the Disconnected Annotation", func() { + var csv operatorsv1alpha1.ClusterServiceVersion + It("Should return false if the annotation is missing", func() { + annotations := map[string]string{} + csv.Annotations = annotations + Expect(HasDisconnectedAnnotation(&csv)).To(BeFalse()) + }) + + It("Should return true if the annotation is present, regardless of its value", func() { + annotations := map[string]string{DisconnectedAnnotation: "foo"} + csv.Annotations = annotations + Expect(HasDisconnectedAnnotation(&csv)).To(BeTrue()) + }) + }) + DescribeTable( "Validating infrastructure features list disconnected", func(featurelist string, expected bool) { - actual := SupportsDisconnected(featurelist) + actual := SupportsDisconnectedViaInfrastructureFeatures(featurelist) Expect(actual).To(Equal(expected)) }, Entry("disconnected in all caps", `["DISCONNECTED"]`, true), diff --git a/internal/policy/operator/restricted_network_aware.go b/internal/policy/operator/restricted_network_aware.go index a6d96f84..e8a3db4c 100644 --- a/internal/policy/operator/restricted_network_aware.go +++ b/internal/policy/operator/restricted_network_aware.go @@ -37,14 +37,18 @@ func (p FollowsRestrictedNetworkEnablementGuidelines) validate(ctx context.Conte return false, err } + restrictedNetworkSupport := false // If the CSV does not claim to support disconnected environments, there's no reason to check that it followed guidelines. - if !libcsv.HasInfrastructureFeaturesAnnotation(csv) { - logger.Info("this operator does not have the infrastructure-features annotation. You can safely ignore this if you your operator is not intended to be restricted-network aware.") - return false, nil + if libcsv.HasDisconnectedAnnotation(csv) && libcsv.SupportsDisconnected(csv.Annotations[libcsv.DisconnectedAnnotation]) { + restrictedNetworkSupport = true + } + + if libcsv.HasInfrastructureFeaturesAnnotation(csv) && libcsv.SupportsDisconnectedViaInfrastructureFeatures(csv.Annotations[libcsv.InfrastructureFeaturesAnnotation]) { + restrictedNetworkSupport = true } - if !libcsv.SupportsDisconnected(csv.Annotations[libcsv.InfrastructureFeaturesAnnotation]) { - logger.Info("the infrastructure-features enabled for this operator did not include the \"disconnected\" identifier. You can safely ignore this if you your operator is not intended to be restricted-network aware.") + if !restrictedNetworkSupport { + logger.Info("this operator does not indicate it supports installation into restricted networks. This is safe to ignore if you are not intending to deploy in these environments.") return false, nil }