review comments, unit & e2e tests for comments, and support for standard/experimental .spec.serviceaccount handling

Signed-off-by: grokspawn <jordan@nimblewidget.com>

Author: grokspawn

api/v1/clusterextension_types.go

@@ -67,13 +67,20 @@ type ClusterExtensionSpec struct {
 	// +kubebuilder:validation:Required
 	Namespace string `json:"namespace"`
 
+	// <opcon:standard:description>
+	// serviceAccount is a required field that references a ServiceAccount used to
+	// perform all interactions with the cluster that are required to manage the extension.
+	// </opcon:standard:description>
+	// <opcon:standard:validation:Required>
+	//
+	// <opcon:experimental:description>
 	// serviceAccount is an optional field that references a ServiceAccount used to
 	// perform all interactions with the cluster that are required to manage the extension.
 	// If not set, operator-controller will use its own ServiceAccount for extension management.
 	// The ServiceAccount must be configured with the necessary permissions to perform these interactions.
 	// The ServiceAccount must exist in the namespace referenced in the spec.
-	//
-	// +optional
+	// </opcon:experimental:description>
+	// <opcon:experimental:validation:Optional>
 	ServiceAccount ServiceAccountReference `json:"serviceAccount,omitzero"`
 
 	// source is a required field which selects the installation source of content
@@ -403,8 +410,8 @@ type ServiceAccountReference struct {
 	// +kubebuilder:validation:MaxLength:=253
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable"
 	// +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters"
-	// +kubebuilder:validation:Optional
-	Name string `json:"name"`
+	// +optional
+	Name string `json:"name,omitempty"`
 }
 
 // PreflightConfig holds the configuration for the preflight checks.  If used, at least one preflight check must be non-nil.

api/v1/clusterextension_types_test.go

@@ -1,6 +1,7 @@
 package v1_test
 
 import (
+	"encoding/json"
 	"fmt"
 	"go/ast"
 	"go/parser"
@@ -11,6 +12,7 @@ import (
 
 	"golang.org/x/exp/slices" // replace with "slices" in go 1.21
 
+	v1 "github.com/operator-framework/operator-controller/api/v1"
 	"github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets"
 )
 
@@ -100,3 +102,62 @@ func parseConstants(prefix string) ([]string, error) {
 	}
 	return constValues, nil
 }
+
+func TestServiceAccountMarshaling(t *testing.T) {
+	tests := []struct {
+		name          string
+		spec          v1.ClusterExtensionSpec
+		expectField   string
+		unexpectField string
+	}{
+		{
+			name: "ServiceAccount with name is marshaled",
+			spec: v1.ClusterExtensionSpec{
+				Namespace: "test-ns",
+				ServiceAccount: v1.ServiceAccountReference{
+					Name: "test-sa",
+				},
+				Source: v1.SourceConfig{
+					SourceType: "Catalog",
+					Catalog: &v1.CatalogFilter{
+						PackageName: "test-package",
+					},
+				},
+			},
+			expectField: "serviceAccount",
+		},
+		{
+			name: "ServiceAccount with empty name is omitted",
+			spec: v1.ClusterExtensionSpec{
+				Namespace:      "test-ns",
+				ServiceAccount: v1.ServiceAccountReference{},
+				Source: v1.SourceConfig{
+					SourceType: "Catalog",
+					Catalog: &v1.CatalogFilter{
+						PackageName: "test-package",
+					},
+				},
+			},
+			unexpectField: "serviceAccount",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			data, err := json.Marshal(tt.spec)
+			if err != nil {
+				t.Fatalf("failed to marshal spec: %v", err)
+			}
+
+			jsonStr := string(data)
+
+			if tt.expectField != "" && !strings.Contains(jsonStr, tt.expectField) {
+				t.Errorf("expected field %q to be present in JSON output, got: %s", tt.expectField, jsonStr)
+			}
+
+			if tt.unexpectField != "" && strings.Contains(jsonStr, tt.unexpectField) {
+				t.Errorf("expected field %q to be omitted from JSON output, got: %s", tt.unexpectField, jsonStr)
+			}
+		})
+	}
+}

docs/api-reference/olmv1-api-reference.md

@@ -340,7 +340,7 @@ _Appears in:_
 | Field | Description | Default | Validation |
 | --- | --- | --- | --- |
 | `namespace` _string_ | namespace is a reference to a Kubernetes namespace.<br />This is the namespace in which the provided ServiceAccount must exist.<br />It also designates the default namespace where namespace-scoped resources<br />for the extension are applied to the cluster.<br />Some extensions may contain namespace-scoped resources to be applied in other namespaces.<br />This namespace must exist.<br />namespace is required, immutable, and follows the DNS label standard<br />as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),<br />start and end with an alphanumeric character, and be no longer than 63 characters<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 |  | MaxLength: 63 <br />Required: \{\} <br /> |
-| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions<br />with the cluster that are required to manage the extension.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br />serviceAccount is required. |  | Required: \{\} <br /> |
+| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | <opcon:standard:description><br />serviceAccount is a required field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br /></opcon:standard:description><br /><opcon:standard:validation:Required><br /><opcon:experimental:description><br />serviceAccount is an optional field that references a ServiceAccount used to<br />perform all interactions with the cluster that are required to manage the extension.<br />If not set, operator-controller will use its own ServiceAccount for extension management.<br />The ServiceAccount must be configured with the necessary permissions to perform these interactions.<br />The ServiceAccount must exist in the namespace referenced in the spec.<br /></opcon:experimental:description><br /><opcon:experimental:validation:Optional> |  |  |
 | `source` _[SourceConfig](#sourceconfig)_ | source is a required field which selects the installation source of content<br />for this ClusterExtension. Selection is performed by setting the sourceType.<br />Catalog is currently the only implemented sourceType, and setting the<br />sourcetype to "Catalog" requires the catalog field to also be defined.<br />Below is a minimal example of a source definition (in yaml):<br />source:<br />  sourceType: Catalog<br />  catalog:<br />    packageName: example-package |  | Required: \{\} <br /> |
 | `install` _[ClusterExtensionInstallConfig](#clusterextensioninstallconfig)_ | install is an optional field used to configure the installation options<br />for the ClusterExtension such as the pre-flight check configuration. |  |  |
 | `config` _[ClusterExtensionConfig](#clusterextensionconfig)_ | config is an optional field used to specify bundle specific configuration<br />used to configure the bundle. Configuration is bundle specific and a bundle may provide<br />a configuration schema. When not specified, the default configuration of the resolved bundle will be used.<br />config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide<br />a configuration schema the final manifests will be derived on a best-effort basis. More information on how<br />to configure the bundle should be found in its end-user documentation.<br /><opcon:experimental> |  |  |
@@ -439,7 +439,7 @@ _Appears in:_
 
 
 
-ServiceAccountReference identifies the serviceAccount used fo install a ClusterExtension.
+ServiceAccountReference identifies the serviceAccount used to install a ClusterExtension.
 
 
 
@@ -448,7 +448,7 @@ _Appears in:_
 
 | Field | Description | Default | Validation |
 | --- | --- | --- | --- |
-| `name` _string_ | name is a required, immutable reference to the name of the ServiceAccount<br />to be used for installation and management of the content for the package<br />specified in the packageName field.<br />This ServiceAccount must exist in the installNamespace.<br />name follows the DNS subdomain standard as defined in [RFC 1123].<br />It must contain only lowercase alphanumeric characters,<br />hyphens (-) or periods (.), start and end with an alphanumeric character,<br />and be no longer than 253 characters.<br />Some examples of valid values are:<br />  - some-serviceaccount<br />  - 123-serviceaccount<br />  - 1-serviceaccount-2<br />  - someserviceaccount<br />  - some.serviceaccount<br />Some examples of invalid values are:<br />  - -some-serviceaccount<br />  - some-serviceaccount-<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 |  | MaxLength: 253 <br />Required: \{\} <br /> |
+| `name` _string_ | name is a required, immutable reference to the name of the ServiceAccount<br />to be used for installation and management of the content for the package<br />specified in the packageName field.<br />This ServiceAccount must exist in the installNamespace.<br />name follows the DNS subdomain standard as defined in [RFC 1123].<br />It must contain only lowercase alphanumeric characters,<br />hyphens (-) or periods (.), start and end with an alphanumeric character,<br />and be no longer than 253 characters.<br />Some examples of valid values are:<br />  - some-serviceaccount<br />  - 123-serviceaccount<br />  - 1-serviceaccount-2<br />  - someserviceaccount<br />  - some.serviceaccount<br />Some examples of invalid values are:<br />  - -some-serviceaccount<br />  - some-serviceaccount-<br />[RFC 1123]: https://tools.ietf.org/html/rfc1123 |  | MaxLength: 253 <br /> |
 
 
 #### SourceConfig

hack/tools/crd-generator/main.go

@@ -23,6 +23,7 @@ import (
 	"log"
 	"os"
 	"regexp"
+	"slices"
 	"strings"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -136,7 +137,7 @@ func runGenerator(args ...string) {
 				if channel == StandardChannel && strings.Contains(version.Name, "alpha") {
 					channelCrd.Spec.Versions[i].Served = false
 				}
-				version.Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties)
+				channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Properties, channelCrd.Spec.Versions[i].Schema.OpenAPIV3Schema.Required = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties, version.Schema.OpenAPIV3Schema.Required)
 			}
 
 			conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion)
@@ -179,25 +180,43 @@ func runGenerator(args ...string) {
 	}
 }
 
-func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps {
+func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps, existingRequired []string) (map[string]apiextensionsv1.JSONSchemaProps, []string) {
+	// Start with existing required fields (from kubebuilder markers)
+	requiredFields := slices.Clone(existingRequired)
+
 	for name := range props {
 		jsonProps := props[name]
-		p := opconTweaks(channel, name, jsonProps)
+		p, reqStatus := opconTweaks(channel, name, jsonProps)
 		if p == nil {
 			delete(props, name)
+			// Remove from required list if present
+			requiredFields = slices.DeleteFunc(requiredFields, func(s string) bool { return s == name })
 		} else {
 			props[name] = *p
+			// Update required list based on tag
+			switch reqStatus {
+			case "required":
+				if !slices.Contains(requiredFields, name) {
+					requiredFields = append(requiredFields, name)
+				}
+			case "optional":
+				requiredFields = slices.DeleteFunc(requiredFields, func(s string) bool { return s == name })
+			// "" (unspecified) means keep existing status
+			}
 		}
 	}
-	return props
+	return props, requiredFields
 }
 
 // Custom Opcon API Tweaks for tags prefixed with `<opcon:` that get past
 // the limitations of Kubebuilder annotations.
-func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSchemaProps) *apiextensionsv1.JSONSchemaProps {
+// Returns the modified schema and a string indicating required status:
+// "required", "optional", or "" (unspecified - preserve existing)
+func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSchemaProps) (*apiextensionsv1.JSONSchemaProps, string) {
+	requiredStatus := "" // "required", "optional", or "" (unspecified)
 	if channel == StandardChannel {
 		if strings.Contains(jsonProps.Description, "<opcon:experimental>") {
-			return nil
+			return nil, ""
 		}
 	}
 
@@ -237,6 +256,22 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 				Rule:    celMatch[2],
 			})
 		}
+
+		optReqRe := regexp.MustCompile(validationPrefix + "(Optional|Required)>")
+		optReqMatches := optReqRe.FindAllStringSubmatch(jsonProps.Description, 64)
+		for _, optReqMatch := range optReqMatches {
+			if len(optReqMatch) != 2 {
+				log.Fatalf("Invalid %s Optional/Required tag for %s", validationPrefix, name)
+			}
+
+			numValid++
+			switch optReqMatch[1] {
+			case "Optional":
+				requiredStatus = "optional"
+			case "Required":
+				requiredStatus = "required"
+			}
+		}
 	}
 
 	if numValid < numExpressions {
@@ -246,34 +281,42 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche
 	jsonProps.Description = formatDescription(jsonProps.Description, channel, name)
 
 	if len(jsonProps.Properties) > 0 {
-		jsonProps.Properties = opconTweaksMap(channel, jsonProps.Properties)
+		jsonProps.Properties, jsonProps.Required = opconTweaksMap(channel, jsonProps.Properties, jsonProps.Required)
 	} else if jsonProps.Items != nil && jsonProps.Items.Schema != nil {
-		jsonProps.Items.Schema = opconTweaks(channel, name, *jsonProps.Items.Schema)
+		jsonProps.Items.Schema, _ = opconTweaks(channel, name, *jsonProps.Items.Schema)
 	}
 
-	return &jsonProps
+	return &jsonProps, requiredStatus
 }
 
 func formatDescription(description string, channel string, name string) string {
-	startTag := "<opcon:experimental:description>"
-	endTag := "</opcon:experimental:description>"
-	if channel == StandardChannel && strings.Contains(description, startTag) {
-		regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*`
-		re := regexp.MustCompile(regexPattern)
-		match := re.FindStringSubmatch(description)
-		if len(match) != 2 {
-			log.Fatalf("Invalid <opcon:experimental:description> tag for %s", name)
+	tagset := []struct {
+		channel string
+		start   string
+		end     string
+	}{
+		{channel: ExperimentalChannel, start: "<opcon:standard:description>", end: "</opcon:standard:description>"},
+		{channel: StandardChannel, start: "<opcon:experimental:description>", end: "</opcon:experimental:description>"},
+	}
+	for _, ts := range tagset {
+		if channel == ts.channel && strings.Contains(description, ts.start) {
+			regexPattern := `\n*` + regexp.QuoteMeta(ts.start) + `(?s:(.*?))` + regexp.QuoteMeta(ts.end) + `\n*`
+			re := regexp.MustCompile(regexPattern)
+			match := re.FindStringSubmatch(description)
+			if len(match) != 2 {
+				log.Fatalf("Invalid <opcon:experimental:description> tag for %s", name)
+			}
+			description = re.ReplaceAllString(description, "\n\n")
+		} else {
+			description = strings.ReplaceAll(description, ts.start, "")
+			description = strings.ReplaceAll(description, ts.end, "")
 		}
-		description = re.ReplaceAllString(description, "\n\n")
-	} else {
-		description = strings.ReplaceAll(description, startTag, "")
-		description = strings.ReplaceAll(description, endTag, "")
 	}
 
 	// Comments within "opcon:util:excludeFromCRD" tag are not included in the generated CRD and all trailing \n operators before
 	// and after the tags are removed and replaced with three \n operators.
-	startTag = "<opcon:util:excludeFromCRD>"
-	endTag = "</opcon:util:excludeFromCRD>"
+	startTag := "<opcon:util:excludeFromCRD>"
+	endTag := "</opcon:util:excludeFromCRD>"
 	if strings.Contains(description, startTag) {
 		regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*`
 		re := regexp.MustCompile(regexPattern)

helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml

@@ -132,11 +132,8 @@ spec:
                   rule: self.matches("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$")
               serviceAccount:
                 description: |-
-                  serviceAccount is an optional field that references a ServiceAccount used to
+                  serviceAccount is a required field that references a ServiceAccount used to
                   perform all interactions with the cluster that are required to manage the extension.
-                  If not set, operator-controller will use its own ServiceAccount for extension management.
-                  The ServiceAccount must be configured with the necessary permissions to perform these interactions.
-                  The ServiceAccount must exist in the namespace referenced in the spec.
                 properties:
                   name:
                     description: |-
@@ -457,6 +454,7 @@ spec:
                     has(self.catalog) : !has(self.catalog)'
             required:
             - namespace
+            - serviceAccount
             - source
             type: object
           status:

manifests/experimental-e2e.yaml

@@ -1699,7 +1699,7 @@ metadata:
   labels:
     app.kubernetes.io/name: operator-controller
     app.kubernetes.io/part-of: olm
-  name: operator-controller-manager-admin-rolebinding
+  name: operator-controller-manager-rolebinding
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole

manifests/experimental.yaml

@@ -1664,7 +1664,7 @@ metadata:
   labels:
     app.kubernetes.io/name: operator-controller
     app.kubernetes.io/part-of: olm
-  name: operator-controller-manager-admin-rolebinding
+  name: operator-controller-manager-rolebinding
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole

manifests/standard-e2e.yaml

@@ -723,11 +723,8 @@ spec:
                   rule: self.matches("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$")
               serviceAccount:
                 description: |-
-                  serviceAccount is an optional field that references a ServiceAccount used to
+                  serviceAccount is a required field that references a ServiceAccount used to
                   perform all interactions with the cluster that are required to manage the extension.
-                  If not set, operator-controller will use its own ServiceAccount for extension management.
-                  The ServiceAccount must be configured with the necessary permissions to perform these interactions.
-                  The ServiceAccount must exist in the namespace referenced in the spec.
                 properties:
                   name:
                     description: |-
@@ -1048,6 +1045,7 @@ spec:
                     has(self.catalog) : !has(self.catalog)'
             required:
             - namespace
+            - serviceAccount
             - source
             type: object
           status:
@@ -1450,7 +1448,7 @@ metadata:
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: ClusterRole
-  name: operator-controller-manager-role
+  name: cluster-admin
 subjects:
   - kind: ServiceAccount
     name: operator-controller-controller-manager