Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #1108 (2023-12-18 16:08:36)

Ensure all disconnected annotations are accounted for when reviewing guidelines
We recently added support for new annotations for various infrastructure features, including disconnected/restricted network support, but our informational check that reviews the CSV's implementation of the guidelines for disconnected support was not accounting for the new annotation.

As a result, we'd emit logs indicating that "this operator didn't support deployment into environments with restricted networks" even if the new annotation was present, which was misleading. 

This PR properly checks both annotations, and in the presence of either with a value indicating support for deployment into these environments, executes the logic to see how closely the maintainer adheres to the guidelines.

This also adds relevant tests, and relabels/documents the older annotation as "legacy".

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#" title="Author self-approved">komish</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [komish]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#pullrequestreview-1787289765" title="LGTM">bcrochet</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#" title="Author self-approved">komish</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [bcrochet,komish]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#pullrequestreview-1787289765" title="LGTM">bcrochet</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#" title="Author self-approved">komish</a>*, *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108#pullrequestreview-1787309535" title="Approved">skattoju</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [bcrochet,komish,skattoju]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dcibot: - SUCCESS https://www.distributed-ci.io/jobs/feb12798-16a7-4b04-a93f-be3a3be57ce3/jobStates
 from https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/1108

Patch:
diff --git a/internal/csv/csv.go b/internal/csv/csv.go
index f0d5e83..fbc27b7 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/policy/operator/restricted_network_aware.go b/internal/policy/operator/restricted_network_aware.go
index a6d96f8..e8a3db4 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
 	}
 


Test patch:
diff --git a/internal/csv/csv_test.go b/internal/csv/csv_test.go
index d63b221..f172720 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),