From c08e67996c710ab2f9a731a63064cb6682bb2332 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 21 Nov 2025 08:29:18 -0500 Subject: [PATCH 1/4] Add crd-generator support for Required/Optional and standard descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit enhances the CRD generator with the following capabilities: - Support for and tags to control field requirements in the generated CRDs - Support for tags to provide channel-specific descriptions (complementing experimental descriptions) - Refactored opconTweaks functions to accept parent schema, enabling modification of the required fields list These changes provide more flexibility in defining CRD schemas with channel-specific behavior and field requirements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- hack/tools/crd-generator/main.go | 67 ++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/hack/tools/crd-generator/main.go b/hack/tools/crd-generator/main.go index 9687489f45..a34398fcee 100644 --- a/hack/tools/crd-generator/main.go +++ b/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) + version.Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema) } conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) @@ -179,10 +180,11 @@ func runGenerator(args ...string) { } } -func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { +func opconTweaksMap(channel string, obj *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { + props := obj.Properties for name := range props { jsonProps := props[name] - p := opconTweaks(channel, name, jsonProps) + p := opconTweaks(channel, name, obj, jsonProps) if p == nil { delete(props, name) } else { @@ -194,7 +196,7 @@ func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaP // Custom Opcon API Tweaks for tags prefixed with `") { return nil @@ -210,6 +212,24 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche numExpressions := strings.Count(jsonProps.Description, validationPrefix) numValid := 0 if numExpressions > 0 { + requiredRe := regexp.MustCompile(validationPrefix + "Required>") + requiredMatches := requiredRe.FindAllStringSubmatch(jsonProps.Description, -1) + for range requiredMatches { + numValid += 1 + if !slices.Contains(parent.Required, name) { + parent.Required = append(parent.Required, name) + } + slices.Sort(parent.Required) + } + + optionalRe := regexp.MustCompile(validationPrefix + "Optional>") + optionalMatches := optionalRe.FindAllStringSubmatch(jsonProps.Description, -1) + for range optionalMatches { + numValid += 1 + parent.Required = slices.DeleteFunc(parent.Required, func(s string) bool { return s == name }) + slices.Sort(parent.Required) + } + enumRe := regexp.MustCompile(validationPrefix + "Enum=([A-Za-z;]*)>") enumMatches := enumRe.FindAllStringSubmatch(jsonProps.Description, 64) for _, enumMatch := range enumMatches { @@ -246,19 +266,34 @@ 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 = opconTweaksMap(channel, &jsonProps) } 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, *jsonProps.Items.Schema) } return &jsonProps } func formatDescription(description string, channel string, name string) string { - startTag := "" - endTag := "" - if channel == StandardChannel && strings.Contains(description, startTag) { - regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` + startTagStandard := "" + endTagStandard := "" + if channel == ExperimentalChannel && strings.Contains(description, startTagStandard) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagStandard) + `(?s:(.*?))` + regexp.QuoteMeta(endTagStandard) + `\n*` + re := regexp.MustCompile(regexPattern) + match := re.FindStringSubmatch(description) + if len(match) != 2 { + log.Fatalf("Invalid tag for %s", name) + } + description = re.ReplaceAllString(description, "\n\n") + } else { + description = strings.ReplaceAll(description, startTagStandard, "") + description = strings.ReplaceAll(description, endTagStandard, "") + } + + startTagExperimental := "" + endTagExperimental := "" + if channel == StandardChannel && strings.Contains(description, startTagExperimental) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagExperimental) + `(?s:(.*?))` + regexp.QuoteMeta(endTagExperimental) + `\n*` re := regexp.MustCompile(regexPattern) match := re.FindStringSubmatch(description) if len(match) != 2 { @@ -266,16 +301,16 @@ func formatDescription(description string, channel string, name string) string { } description = re.ReplaceAllString(description, "\n\n") } else { - description = strings.ReplaceAll(description, startTag, "") - description = strings.ReplaceAll(description, endTag, "") + description = strings.ReplaceAll(description, startTagExperimental, "") + description = strings.ReplaceAll(description, endTagExperimental, "") } // 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 = "" - endTag = "" - if strings.Contains(description, startTag) { - regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` + startTagExclude := "" + endTagExclude := "" + if strings.Contains(description, startTagExclude) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagExclude) + `(?s:(.*?))` + regexp.QuoteMeta(endTagExclude) + `\n*` re := regexp.MustCompile(regexPattern) match := re.FindStringSubmatch(description) if len(match) != 2 { From 1094bc94bd4168ca45fd8f6aa55b0989729130a3 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 21 Nov 2025 08:49:53 -0500 Subject: [PATCH 2/4] Make ServiceAccount optional in experimental channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit updates the ClusterExtension API to make the serviceAccount field optional in the experimental channel while keeping it required in the standard channel. Changes to ClusterExtension API: - Added and tags to ServiceAccount field - Added channel-specific descriptions for serviceAccount behavior: - Standard: Service account is required - Experimental: If omitted, authenticates as synthetic user "olmv1:clusterextensions:" with group "olmv1:clusterextensions" - Added channel-specific descriptions for namespace field explaining the relationship with serviceAccount - Updated ServiceAccountReference documentation to remove namespace requirement text - Changed JSON tags to use omitzero for optional behavior Generated artifacts updated: - CRDs (standard and experimental channels) - API reference documentation - All manifest files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- api/v1/clusterextension_types.go | 35 ++++++++++++++----- docs/api-reference/olmv1-api-reference.md | 8 ++--- ...peratorframework.io_clusterextensions.yaml | 19 +++++----- ...peratorframework.io_clusterextensions.yaml | 11 +++--- .../clusterextension_admission_test.go | 2 +- manifests/experimental-e2e.yaml | 19 +++++----- manifests/experimental.yaml | 19 +++++----- manifests/standard-e2e.yaml | 11 +++--- manifests/standard.yaml | 11 +++--- 9 files changed, 82 insertions(+), 53 deletions(-) diff --git a/api/v1/clusterextension_types.go b/api/v1/clusterextension_types.go index 6de62b0e12..72dab3b28b 100644 --- a/api/v1/clusterextension_types.go +++ b/api/v1/clusterextension_types.go @@ -49,12 +49,19 @@ const ( // ClusterExtensionSpec defines the desired state of ClusterExtension type ClusterExtensionSpec struct { // namespace is a reference to a Kubernetes namespace. - // This is the namespace in which the provided ServiceAccount must exist. - // It also designates the default namespace where namespace-scoped resources + // + // This designates the default namespace where namespace-scoped resources // for the extension are applied to the cluster. // Some extensions may contain namespace-scoped resources to be applied in other namespaces. // This namespace must exist. // + // + // This is also the namespace of the referenced service account. + // + // + // This is also the namespace of the referenced service account, if specified. + // + // // namespace is required, immutable, and follows the DNS label standard // as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), // start and end with an alphanumeric character, and be no longer than 63 characters @@ -69,12 +76,24 @@ type ClusterExtensionSpec struct { // serviceAccount is a reference to a ServiceAccount used to perform all interactions // with the cluster that are required to manage the extension. + // // The ServiceAccount must be configured with the necessary permissions to perform these interactions. - // The ServiceAccount must exist in the namespace referenced in the spec. + // // serviceAccount is required. + // // - // +kubebuilder:validation:Required - ServiceAccount ServiceAccountReference `json:"serviceAccount"` + // + // If serviceAccount is specified, OLM will authenticate as that service account. + // Otherwise, operator-controller will authenticate as: + // - User: "olm:clusterextension:" + // - Group: "olm:clusterextensions" + // + // The authenticated user must be configured with the necessary permissions to perform these interactions. + // + // + // + // + ServiceAccount ServiceAccountReference `json:"serviceAccount,omitzero"` // source is a required field which selects the installation source of content // for this ClusterExtension. Selection is performed by setting the sourceType. @@ -374,14 +393,12 @@ type CatalogFilter struct { UpgradeConstraintPolicy UpgradeConstraintPolicy `json:"upgradeConstraintPolicy,omitempty"` } -// ServiceAccountReference identifies the serviceAccount used fo install a ClusterExtension. +// ServiceAccountReference identifies the serviceAccount name used to manage a ClusterExtension. type ServiceAccountReference struct { // name is a required, immutable reference to the name of the ServiceAccount // to be used for installation and management of the content for the package // specified in the packageName field. // - // This ServiceAccount must exist in the installNamespace. - // // name follows the DNS subdomain standard as defined in [RFC 1123]. // It must contain only lowercase alphanumeric characters, // hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -404,7 +421,7 @@ type ServiceAccountReference struct { // +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:Required - Name string `json:"name"` + Name string `json:"name,omitempty"` } // PreflightConfig holds the configuration for the preflight checks. If used, at least one preflight check must be non-nil. diff --git a/docs/api-reference/olmv1-api-reference.md b/docs/api-reference/olmv1-api-reference.md index 317b46a00c..b5f2207a81 100644 --- a/docs/api-reference/olmv1-api-reference.md +++ b/docs/api-reference/olmv1-api-reference.md @@ -339,8 +339,8 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `namespace` _string_ | namespace is a reference to a Kubernetes namespace.
This is the namespace in which the provided ServiceAccount must exist.
It also designates the default namespace where namespace-scoped resources
for the extension are applied to the cluster.
Some extensions may contain namespace-scoped resources to be applied in other namespaces.
This namespace must exist.
namespace is required, immutable, and follows the DNS label standard
as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),
start and end with an alphanumeric character, and be no longer than 63 characters
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63
Required: \{\}
| -| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.
The ServiceAccount must be configured with the necessary permissions to perform these interactions.
The ServiceAccount must exist in the namespace referenced in the spec.
serviceAccount is required. | | Required: \{\}
| +| `namespace` _string_ | namespace is a reference to a Kubernetes namespace.
This designates the default namespace where namespace-scoped resources
for the extension are applied to the cluster.
Some extensions may contain namespace-scoped resources to be applied in other namespaces.
This namespace must exist.

This is also the namespace of the referenced service account.


This is also the namespace of the referenced service account, if specified.

namespace is required, immutable, and follows the DNS label standard
as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),
start and end with an alphanumeric character, and be no longer than 63 characters
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63
Required: \{\}
| +| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.

The ServiceAccount must be configured with the necessary permissions to perform these interactions.
serviceAccount is required.


If serviceAccount is specified, OLM will authenticate as that service account.
Otherwise, operator-controller will authenticate as:
- User: "olm:clusterextension:"
- Group: "olm:clusterextensions"
The authenticated user must be configured with the necessary permissions to perform these interactions.


| | | | `source` _[SourceConfig](#sourceconfig)_ | source is a required field which selects the installation source of content
for this ClusterExtension. Selection is performed by setting the sourceType.
Catalog is currently the only implemented sourceType, and setting the
sourcetype to "Catalog" requires the catalog field to also be defined.
Below is a minimal example of a source definition (in yaml):
source:
sourceType: Catalog
catalog:
packageName: example-package | | Required: \{\}
| | `install` _[ClusterExtensionInstallConfig](#clusterextensioninstallconfig)_ | install is an optional field used to configure the installation options
for the ClusterExtension such as the pre-flight check configuration. | | | | `config` _[ClusterExtensionConfig](#clusterextensionconfig)_ | config is an optional field used to specify bundle specific configuration
used to configure the bundle. Configuration is bundle specific and a bundle may provide
a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
a configuration schema the final manifests will be derived on a best-effort basis. More information on how
to configure the bundle should be found in its end-user documentation.
| | | @@ -439,7 +439,7 @@ _Appears in:_ -ServiceAccountReference identifies the serviceAccount used fo install a ClusterExtension. +ServiceAccountReference identifies the serviceAccount name used to manage 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
to be used for installation and management of the content for the package
specified in the packageName field.
This ServiceAccount must exist in the installNamespace.
name follows the DNS subdomain standard as defined in [RFC 1123].
It must contain only lowercase alphanumeric characters,
hyphens (-) or periods (.), start and end with an alphanumeric character,
and be no longer than 253 characters.
Some examples of valid values are:
- some-serviceaccount
- 123-serviceaccount
- 1-serviceaccount-2
- someserviceaccount
- some.serviceaccount
Some examples of invalid values are:
- -some-serviceaccount
- some-serviceaccount-
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 253
Required: \{\}
| +| `name` _string_ | name is a required, immutable reference to the name of the ServiceAccount
to be used for installation and management of the content for the package
specified in the packageName field.
name follows the DNS subdomain standard as defined in [RFC 1123].
It must contain only lowercase alphanumeric characters,
hyphens (-) or periods (.), start and end with an alphanumeric character,
and be no longer than 253 characters.
Some examples of valid values are:
- some-serviceaccount
- 123-serviceaccount
- 1-serviceaccount-2
- someserviceaccount
- some.serviceaccount
Some examples of invalid values are:
- -some-serviceaccount
- some-serviceaccount-
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 253
Required: \{\}
| #### SourceConfig diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml index 1038b7fdf0..614f3c3c08 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml @@ -151,12 +151,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -173,9 +175,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -183,8 +189,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -498,7 +502,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: diff --git a/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml b/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml index a0983e41f9..240a8d831c 100644 --- a/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml +++ b/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml @@ -112,12 +112,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -134,8 +136,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -144,8 +147,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, diff --git a/internal/operator-controller/controllers/clusterextension_admission_test.go b/internal/operator-controller/controllers/clusterextension_admission_test.go index 38c6c60d41..414867909e 100644 --- a/internal/operator-controller/controllers/clusterextension_admission_test.go +++ b/internal/operator-controller/controllers/clusterextension_admission_test.go @@ -349,7 +349,7 @@ func TestClusterExtensionAdmissionServiceAccount(t *testing.T) { {"dot-separated", "dotted.name", ""}, {"longest valid service account name", strings.Repeat("x", 253), ""}, {"too long service account name", strings.Repeat("x", 254), tooLongError}, - {"no service account name", "", regexMismatchError}, + {"no service account name", "", ""}, {"spaces", "spaces spaces", regexMismatchError}, {"capitalized", "Capitalized", regexMismatchError}, {"camel case", "camelCase", regexMismatchError}, diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 6728302259..29a90cebf8 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -939,12 +939,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -961,9 +963,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -971,8 +977,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1286,7 +1290,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 199838eac5..53fc0e936d 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -904,12 +904,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -926,9 +928,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -936,8 +942,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1251,7 +1255,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: diff --git a/manifests/standard-e2e.yaml b/manifests/standard-e2e.yaml index 5c95907841..b5bdda4e35 100644 --- a/manifests/standard-e2e.yaml +++ b/manifests/standard-e2e.yaml @@ -703,12 +703,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -725,8 +727,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -735,8 +738,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, diff --git a/manifests/standard.yaml b/manifests/standard.yaml index 95e400c264..a1f02c7610 100644 --- a/manifests/standard.yaml +++ b/manifests/standard.yaml @@ -668,12 +668,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -690,8 +692,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -700,8 +703,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, From 189a05a5e11ab78921b8294fd542515cebf8f485 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 21 Nov 2025 09:15:37 -0500 Subject: [PATCH 3/4] Enable SyntheticPermissions feature gate for experimental channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit enables the SyntheticPermissions feature to allow ClusterExtensions to operate without specifying a ServiceAccount by authenticating as synthetic users. Feature gate changes: - Enabled SyntheticPermissions in experimental channel (helm) - Enabled SyntheticPermissions in tilt configuration RBAC changes: - Added conditional RBAC rules for impersonating users and the "olm:clusterextensions" group when SyntheticPermissions is enabled - Kept existing serviceaccounts/token permissions for token-based auth Logic changes: - Updated SyntheticUserRestConfigMapper to check for empty SA name instead of synthetic-user sentinel value - Modified NewRBACPreAuthorizer to accept a userInfoMapper function - Implemented userInfoMapper in main.go to return synthetic user info when SA name is empty and feature gate is enabled, otherwise returns standard service account user info - Updated authorization tests to work with new userInfoMapper pattern With these changes: - When ServiceAccount.Name is empty and SyntheticPermissions is enabled: authenticates as "olm:clusterextension:" with group "olm:clusterextensions" using impersonation - When ServiceAccount.Name is specified: uses existing token-based authentication (unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- cmd/operator-controller/main.go | 11 +++- docs/draft/howto/use-synthetic-permissions.md | 13 ++-- .../argocd-clusterextension.yaml | 2 - ...ynthetic-user-cluster-admin-demo-script.sh | 2 +- helm/experimental.yaml | 1 + ...rrole-operator-controller-manager-role.yml | 18 ++++- helm/tilt.yaml | 2 + .../operator-controller/action/restconfig.go | 7 +- .../action/restconfig_test.go | 21 +----- .../operator-controller/authorization/rbac.go | 12 +++- .../authorization/rbac_test.go | 12 ++-- manifests/experimental-e2e.yaml | 66 +------------------ manifests/experimental.yaml | 66 +------------------ 13 files changed, 63 insertions(+), 170 deletions(-) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 7425c7b66b..cd5e9824cf 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -36,6 +36,7 @@ import ( k8slabels "k8s.io/apimachinery/pkg/labels" k8stypes "k8s.io/apimachinery/pkg/types" apimachineryrand "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/discovery" "k8s.io/client-go/discovery/cached/memory" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -671,7 +672,15 @@ func setupHelm( // determine if PreAuthorizer should be enabled based on feature gate var preAuth authorization.PreAuthorizer if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient()) + preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient(), func(ext *ocv1.ClusterExtension) (*user.DefaultInfo, error) { + if ext.Spec.ServiceAccount.Name == "" && features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) { + syntheticConfig := authentication.SyntheticImpersonationConfig(*ext) + return &user.DefaultInfo{Name: syntheticConfig.UserName, Groups: syntheticConfig.Groups}, nil + } else if ext.Spec.ServiceAccount.Name == "" || ext.Spec.Namespace == "" { + return nil, errors.New("service account name and namespace must be specified") + } + return &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}, nil + }) } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) diff --git a/docs/draft/howto/use-synthetic-permissions.md b/docs/draft/howto/use-synthetic-permissions.md index 9820ad2b6e..38e1cf1313 100644 --- a/docs/draft/howto/use-synthetic-permissions.md +++ b/docs/draft/howto/use-synthetic-permissions.md @@ -8,7 +8,7 @@ Synthetic user permissions enables fine-grained configuration of ClusterExtensio User can not only configure RBAC permissions governing the management across all ClusterExtensions, but also on a case-by-case basis. -### Run OLM v1with Experimental Features Enabled +### Run OLM v1 with Experimental Features Enabled ```terminal title=Enable Experimental Features in a New Kind Cluster make run-experimental @@ -20,10 +20,11 @@ kubectl rollout status -n olmv1-system deployment/operator-controller-controller ### How does it work? -When managing a ClusterExtension, OLM will assume the identity of user "olm:clusterextensions:" -and group "olm:clusterextensions" limiting Kubernetes API access scope to those defined for this user and group. These -users and group do not exist beyond being defined in Cluster/RoleBinding(s) and can only be impersonated by clients with - `impersonate` verb permissions on the `users` and `groups` resources. +When managing a ClusterExtension that does not specify a service account, OLM will assume the identity of user +"olm:clusterextension:" and group "olm:clusterextensions" limiting Kubernetes API access scope +to those defined for this user and group. These users and group do not exist beyond being defined in +Cluster/RoleBinding(s) and can only be impersonated by clients with `impersonate` verb permissions on the `users` and +`groups` resources. ### Demo @@ -50,7 +51,7 @@ subjects: name: "olm:clusterextensions" ``` -##### Scoped olm:clusterextension group + Added perms on specific extensions +##### Scoped olm:clusterextensions group + Added perms on specific extensions Give ClusterExtension management group broad permissions to manage ClusterExtensions denying potentially dangerous permissions such as being able to read cluster wide secrets: diff --git a/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml b/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml index 7eb5a7082b..01bb6c52d5 100644 --- a/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml +++ b/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml @@ -4,8 +4,6 @@ metadata: name: argocd-operator spec: namespace: argocd-system - serviceAccount: - name: "olm.synthetic-user" source: sourceType: Catalog catalog: diff --git a/hack/demo/synthetic-user-cluster-admin-demo-script.sh b/hack/demo/synthetic-user-cluster-admin-demo-script.sh index 4790e46e77..910e708ec7 100755 --- a/hack/demo/synthetic-user-cluster-admin-demo-script.sh +++ b/hack/demo/synthetic-user-cluster-admin-demo-script.sh @@ -20,7 +20,7 @@ bat --style=plain ${DEMO_RESOURCE_DIR}/synthetic-user-perms/cegroup-admin-bindin # apply cluster role binding kubectl apply -f ${DEMO_RESOURCE_DIR}/synthetic-user-perms/cegroup-admin-binding.yaml -# install cluster extension - for now .spec.serviceAccount = "olm.synthetic-user" +# install cluster extension without specifying .spec.serviceAccount bat --style=plain ${DEMO_RESOURCE_DIR}/synthetic-user-perms/argocd-clusterextension.yaml # apply cluster extension diff --git a/helm/experimental.yaml b/helm/experimental.yaml index b14b1b3034..761837c7f1 100644 --- a/helm/experimental.yaml +++ b/helm/experimental.yaml @@ -13,6 +13,7 @@ options: - PreflightPermissions - HelmChartSupport - BoxcutterRuntime + - SyntheticPermissions disabled: - WebhookProviderOpenshiftServiceCA # List of enabled experimental features for catalogd diff --git a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml index 84f221003c..6fa1b61365 100644 --- a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml +++ b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml @@ -1,4 +1,4 @@ -{{- if and .Values.options.operatorController.enabled (not (has "BoxcutterRuntime" .Values.operatorConrollerFeatures)) }} +{{- if and .Values.options.operatorController.enabled (not (has "BoxcutterRuntime" .Values.options.operatorController.features.enabled)) }} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: @@ -15,6 +15,22 @@ rules: - serviceaccounts/token verbs: - create + {{- if (has "SyntheticPermissions" .Values.options.operatorController.features.enabled) }} + - apiGroups: + - "" + resources: + - users + verbs: + - impersonate + - apiGroups: + - "" + resources: + - groups + resourceNames: + - olm:clusterextensions + verbs: + - impersonate + {{- end }} - apiGroups: - apiextensions.k8s.io resources: diff --git a/helm/tilt.yaml b/helm/tilt.yaml index aaed7c71fb..0f94e56703 100644 --- a/helm/tilt.yaml +++ b/helm/tilt.yaml @@ -17,6 +17,8 @@ options: - SingleOwnNamespaceInstallSupport - PreflightPermissions - HelmChartSupport + - BoxcutterRuntime + - SyntheticPermissions disabled: - WebhookProviderOpenshiftServiceCA catalogd: diff --git a/internal/operator-controller/action/restconfig.go b/internal/operator-controller/action/restconfig.go index 05e25f707d..051eba219e 100644 --- a/internal/operator-controller/action/restconfig.go +++ b/internal/operator-controller/action/restconfig.go @@ -14,18 +14,15 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) -const syntheticServiceAccountName = "olm.synthetic-user" - // SyntheticUserRestConfigMapper returns an AuthConfigMapper that that impersonates synthetic users and groups for Object o. -// o is expected to be a ClusterExtension. If the service account defined in o is different from 'olm.synthetic-user', the -// defaultAuthMapper will be used +// o is expected to be a ClusterExtension. If the service account is defined in o, the defaultAuthMapper will be used. func SyntheticUserRestConfigMapper(defaultAuthMapper func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error)) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { cExt, err := validate(o, c) if err != nil { return nil, err } - if cExt.Spec.ServiceAccount.Name != syntheticServiceAccountName { + if cExt.Spec.ServiceAccount.Name != "" { return defaultAuthMapper(ctx, cExt, c) } cc := rest.CopyConfig(c) diff --git a/internal/operator-controller/action/restconfig_test.go b/internal/operator-controller/action/restconfig_test.go index 4c9f786719..7593838017 100644 --- a/internal/operator-controller/action/restconfig_test.go +++ b/internal/operator-controller/action/restconfig_test.go @@ -98,22 +98,10 @@ func Test_SyntheticUserRestConfigMapper_Fails(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - tokenGetter := &authentication.TokenGetter{} - saMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + saMapper := action.SyntheticUserRestConfigMapper(nil) actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg) - if tc.expectedError != nil { - require.Nil(t, actualCfg) - require.EqualError(t, err, tc.expectedError.Error()) - } else { - require.NoError(t, err) - transport, err := rest.TransportFor(actualCfg) - require.NoError(t, err) - require.NotNil(t, transport) - tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper) - require.True(t, ok) - require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter) - require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key) - } + require.Nil(t, actualCfg) + require.EqualError(t, err, tc.expectedError.Error()) }) } } @@ -150,9 +138,6 @@ func Test_SyntheticUserRestConfigMapper_UsesSyntheticAuthMapper(t *testing.T) { Name: "my-clusterextension", }, Spec: ocv1.ClusterExtensionSpec{ - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "olm.synthetic-user", - }, Namespace: "my-namespace", }, } diff --git a/internal/operator-controller/authorization/rbac.go b/internal/operator-controller/authorization/rbac.go index 357268615c..7e42f3f99b 100644 --- a/internal/operator-controller/authorization/rbac.go +++ b/internal/operator-controller/authorization/rbac.go @@ -55,13 +55,17 @@ var namespacedCollectionVerbs = []string{"create"} var clusterCollectionVerbs = []string{"list", "watch"} type rbacPreAuthorizer struct { + getUserInfo userInfoMapper authorizer authorizer.Authorizer ruleResolver validation.AuthorizationRuleResolver restMapper meta.RESTMapper } -func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer { +type userInfoMapper func(extension *ocv1.ClusterExtension) (*user.DefaultInfo, error) + +func NewRBACPreAuthorizer(cl client.Client, getUserInfo userInfoMapper) PreAuthorizer { return &rbacPreAuthorizer{ + getUserInfo: getUserInfo, authorizer: newRBACAuthorizer(cl), ruleResolver: newRBACRulesResolver(cl), restMapper: cl.RESTMapper(), @@ -84,7 +88,11 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, ext *ocv1.ClusterE if err != nil { return nil, err } - manifestManager := &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)} + + manifestManager, err := a.getUserInfo(ext) + if err != nil { + return nil, fmt.Errorf("error getting user info mapping from clusterextension: %v", err) + } attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager, ext) var preAuthEvaluationErrors []error diff --git a/internal/operator-controller/authorization/rbac_test.go b/internal/operator-controller/authorization/rbac_test.go index 8e3d47687d..ed76138e36 100644 --- a/internal/operator-controller/authorization/rbac_test.go +++ b/internal/operator-controller/authorization/rbac_test.go @@ -414,10 +414,14 @@ func setupFakeClient(role client.Object) client.Client { return fakeClientBuilder.Build() } +func saInfo(ext *ocv1.ClusterExtension) (*user.DefaultInfo, error) { + return &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}, nil +} + func TestPreAuthorize_Success(t *testing.T) { t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(privilegedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) @@ -427,7 +431,7 @@ func TestPreAuthorize_Success(t *testing.T) { func TestPreAuthorize_MissingRBAC(t *testing.T) { t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(limitedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, expectedSingleNamespaceMissingRules, missingRules) @@ -437,7 +441,7 @@ func TestPreAuthorize_MissingRBAC(t *testing.T) { func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) { t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) { fakeClient := setupFakeClient(limitedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace)) require.NoError(t, err) require.Equal(t, expectedMultiNamespaceMissingRules, missingRules) @@ -447,7 +451,7 @@ func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) { func TestPreAuthorize_CheckEscalation(t *testing.T) { t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(escalatingClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 29a90cebf8..f6f86486eb 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -1557,71 +1557,6 @@ rules: - list - watch --- -# Source: olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: operator-controller-manager-role - labels: - app.kubernetes.io/name: operator-controller - app.kubernetes.io/part-of: olm - annotations: - olm.operatorframework.io/feature-set: experimental-e2e -rules: - - apiGroups: - - "" - resources: - - serviceaccounts/token - verbs: - - create - - apiGroups: - - apiextensions.k8s.io - resources: - - customresourcedefinitions - verbs: - - get - - apiGroups: - - olm.operatorframework.io - resources: - - clustercatalogs - verbs: - - get - - list - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions - verbs: - - get - - list - - patch - - update - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/finalizers - verbs: - - update - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/status - verbs: - - patch - - update - - apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - - clusterroles - - rolebindings - - roles - verbs: - - list - - watch ---- # Source: olmv1/templates/rbac/clusterrolebinding-catalogd-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -2180,6 +2115,7 @@ spec: - --feature-gates=PreflightPermissions=true - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true + - --feature-gates=SyntheticPermissions=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 53fc0e936d..08cf9929f0 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -1522,71 +1522,6 @@ rules: - list - watch --- -# Source: olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: operator-controller-manager-role - labels: - app.kubernetes.io/name: operator-controller - app.kubernetes.io/part-of: olm - annotations: - olm.operatorframework.io/feature-set: experimental -rules: - - apiGroups: - - "" - resources: - - serviceaccounts/token - verbs: - - create - - apiGroups: - - apiextensions.k8s.io - resources: - - customresourcedefinitions - verbs: - - get - - apiGroups: - - olm.operatorframework.io - resources: - - clustercatalogs - verbs: - - get - - list - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions - verbs: - - get - - list - - patch - - update - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/finalizers - verbs: - - update - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/status - verbs: - - patch - - update - - apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - - clusterroles - - rolebindings - - roles - verbs: - - list - - watch ---- # Source: olmv1/templates/rbac/clusterrolebinding-catalogd-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -2091,6 +2026,7 @@ spec: - --feature-gates=PreflightPermissions=true - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true + - --feature-gates=SyntheticPermissions=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key From 324a970e76be8f813e8cec50b8095bf75ade28a7 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Fri, 21 Nov 2025 10:05:30 -0500 Subject: [PATCH 4/4] Switch from token-based auth to ServiceAccount impersonation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit replaces token-based authentication with ServiceAccount impersonation for better security and simpler token management. Changes: Authentication layer: - Added ServiceAccountImpersonationConfig() function that returns an ImpersonationConfig for a ServiceAccount user - Updated ServiceAccountRestConfigMapper() to use impersonation via NewImpersonatingRoundTripper instead of TokenInjectingRoundTripper - Removed TokenGetter parameter from ServiceAccountRestConfigMapper - Deleted tokengetter.go, tokengetter_test.go, and tripper.go as they are no longer needed RBAC: - Changed from serviceaccounts/token create to serviceaccounts impersonate permission Controller: - Removed ServiceAccountNotFoundError handling since impersonation doesn't require the ServiceAccount to exist beforehand - Removed authentication package import from controller Main setup: - Removed TokenGetter initialization - Updated userInfoMapper to use ServiceAccountImpersonationConfig Tests: - Updated tests to verify impersonation headers instead of token injection - Added tests for ServiceAccountImpersonationConfig - Updated controller tests that referenced TokenGetter Benefits of impersonation over tokens: - No need to manage token lifecycle or expiration - No need to check if ServiceAccount exists before use - Simpler code with fewer moving parts - More secure as no tokens are created or cached - More secure as it is no longer required to create highly privileged ServiceAccounts that could be used by workloads in the install namespace. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- cmd/operator-controller/main.go | 6 +- config/samples/olm_v1_clusterextension.yaml | 12 +- docs/howto/derive-service-account.md | 20 ++- docs/tutorials/upgrade-extension.md | 8 +- ...rrole-operator-controller-manager-role.yml | 4 +- .../operator-controller/action/restconfig.go | 21 +-- .../action/restconfig_test.go | 20 ++- .../authentication/serviceaccount.go | 17 +++ .../authentication/serviceaccount_test.go | 24 ++++ .../authentication/tokengetter.go | 128 ------------------ .../authentication/tokengetter_test.go | 89 ------------ .../authentication/tripper.go | 38 ------ .../authorization/rbac_test.go | 3 +- .../clusterextension_controller.go | 7 - .../clusterextension_controller_test.go | 56 -------- manifests/standard-e2e.yaml | 4 +- manifests/standard.yaml | 4 +- 17 files changed, 90 insertions(+), 371 deletions(-) create mode 100644 internal/operator-controller/authentication/serviceaccount.go create mode 100644 internal/operator-controller/authentication/serviceaccount_test.go delete mode 100644 internal/operator-controller/authentication/tokengetter.go delete mode 100644 internal/operator-controller/authentication/tokengetter_test.go delete mode 100644 internal/operator-controller/authentication/tripper.go diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index cd5e9824cf..e9c0b09576 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -644,8 +644,7 @@ func setupHelm( if err != nil { return fmt.Errorf("unable to create core client: %w", err) } - tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour)) - clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + clientRestConfigMapper := action.ServiceAccountRestConfigMapper() if features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) { clientRestConfigMapper = action.SyntheticUserRestConfigMapper(clientRestConfigMapper) } @@ -679,7 +678,8 @@ func setupHelm( } else if ext.Spec.ServiceAccount.Name == "" || ext.Spec.Namespace == "" { return nil, errors.New("service account name and namespace must be specified") } - return &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}, nil + saConfig := authentication.ServiceAccountImpersonationConfig(*ext) + return &user.DefaultInfo{Name: saConfig.UserName, Groups: saConfig.Groups}, nil }) } diff --git a/config/samples/olm_v1_clusterextension.yaml b/config/samples/olm_v1_clusterextension.yaml index 14c8e167e0..22f66158f2 100644 --- a/config/samples/olm_v1_clusterextension.yaml +++ b/config/samples/olm_v1_clusterextension.yaml @@ -4,11 +4,13 @@ kind: Namespace metadata: name: argocd --- -apiVersion: v1 -kind: ServiceAccount -metadata: - name: argocd-installer - namespace: argocd +# NOTE: The ServiceAccount resource is intentionally NOT created here. +# OLM v1 uses Kubernetes impersonation and does not require the ServiceAccount to exist. +# For security reasons, you should NOT create a ServiceAccount resource for the installer, +# as it would be highly privileged and could be mounted by other pods in the namespace. +# Instead, only the RBAC resources (ClusterRole, ClusterRoleBinding, Role, RoleBinding) +# are created, which reference the ServiceAccount name "argocd-installer". +# OLM will impersonate that ServiceAccount and be subject to these RBAC permissions. --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding diff --git a/docs/howto/derive-service-account.md b/docs/howto/derive-service-account.md index c1c1204da2..93bad8b975 100644 --- a/docs/howto/derive-service-account.md +++ b/docs/howto/derive-service-account.md @@ -300,12 +300,20 @@ Therefore, the following permissions must be given to the installer service acco Once the installer service account required cluster-scoped and namespace-scoped permissions have been collected: 1. Create the installation namespace -2. Create the installer `ServiceAccount` -3. Create the installer `ClusterRole` -4. Create the `ClusterRoleBinding` between the installer service account and its cluster role -5. Create the installer `Role` -6. Create the `RoleBinding` between the installer service account and its role -7. Create the `ClusterExtension` +2. Create the installer `ClusterRole` +3. Create the `ClusterRoleBinding` between the installer service account and its cluster role +4. Create the installer `Role` +5. Create the `RoleBinding` between the installer service account and its role +6. Create the `ClusterExtension` + +!!! important "Do Not Create the ServiceAccount Resource" + OLM v1 uses Kubernetes impersonation and does not require the ServiceAccount to exist as an actual resource. + For security reasons, you should **NOT** create a ServiceAccount resource for the installer, + as it would be highly privileged and could be mounted by other pods in the namespace. + + Instead, only create the RBAC resources (ClusterRole, ClusterRoleBinding, Role, RoleBinding) + that reference the ServiceAccount name. OLM will impersonate that ServiceAccount and be subject + to these RBAC permissions without the ServiceAccount resource actually existing in the cluster. A manifest with the full set of resources can be found [here](https://github.com/operator-framework/operator-controller/blob/main/config/samples/olm_v1_clusterextension.yaml). diff --git a/docs/tutorials/upgrade-extension.md b/docs/tutorials/upgrade-extension.md index b8b2aa5aad..9fb31aff2c 100644 --- a/docs/tutorials/upgrade-extension.md +++ b/docs/tutorials/upgrade-extension.md @@ -33,12 +33,6 @@ saving it to a local file and then running `kubectl apply -f FILENAME`: metadata: name: argocd --- - apiVersion: v1 - kind: ServiceAccount - metadata: - name: argocd-installer - namespace: argocd - --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: @@ -376,4 +370,4 @@ kubectl get clusterextension argocd -o jsonpath-as-json="{.status.install}" After your upgrade, the contents of the `kubectl.kubernetes.io/last-applied-configuration` annotation field will differ depending on your method of upgrade. If you apply a new ClusterExtension manifest as in the first method shown, the last applied configuration will show the new version since we replaced the existing manifest. If you use the patch - method or `kubectl edit clusterextension`, then the last applied configuration will show the old version. \ No newline at end of file + method or `kubectl edit clusterextension`, then the last applied configuration will show the old version. diff --git a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml index 6fa1b61365..0d30c77cdb 100644 --- a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml +++ b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml @@ -12,9 +12,9 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate {{- if (has "SyntheticPermissions" .Values.options.operatorController.features.enabled) }} - apiGroups: - "" diff --git a/internal/operator-controller/action/restconfig.go b/internal/operator-controller/action/restconfig.go index 051eba219e..d07e45eafb 100644 --- a/internal/operator-controller/action/restconfig.go +++ b/internal/operator-controller/action/restconfig.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "k8s.io/client-go/transport" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,25 +33,19 @@ func SyntheticUserRestConfigMapper(defaultAuthMapper func(ctx context.Context, o } // ServiceAccountRestConfigMapper returns an AuthConfigMapper scoped to the service account defined in o, which is expected to -// be a ClusterExtension -func ServiceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { +// be a ClusterExtension. It uses impersonation, which allows authentication as a ServiceAccount without requiring the +// ServiceAccount to actually exist or having to manage tokens. +func ServiceAccountRestConfigMapper() func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { cExt, err := validate(o, c) if err != nil { return nil, err } - saConfig := rest.AnonymousClientConfig(c) - saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { - return &authentication.TokenInjectingRoundTripper{ - Tripper: rt, - TokenGetter: tokenGetter, - Key: types.NamespacedName{ - Name: cExt.Spec.ServiceAccount.Name, - Namespace: cExt.Spec.Namespace, - }, - } + cc := rest.CopyConfig(c) + cc.Wrap(func(rt http.RoundTripper) http.RoundTripper { + return transport.NewImpersonatingRoundTripper(authentication.ServiceAccountImpersonationConfig(*cExt), rt) }) - return saConfig, nil + return cc, nil } } diff --git a/internal/operator-controller/action/restconfig_test.go b/internal/operator-controller/action/restconfig_test.go index 7593838017..bb676fd446 100644 --- a/internal/operator-controller/action/restconfig_test.go +++ b/internal/operator-controller/action/restconfig_test.go @@ -9,13 +9,11 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/action" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) func Test_ServiceAccountRestConfigMapper(t *testing.T) { @@ -55,21 +53,21 @@ func Test_ServiceAccountRestConfigMapper(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - tokenGetter := &authentication.TokenGetter{} - saMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + saMapper := action.ServiceAccountRestConfigMapper() actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg) if tc.expectedError != nil { require.Nil(t, actualCfg) require.EqualError(t, err, tc.expectedError.Error()) } else { require.NoError(t, err) - transport, err := rest.TransportFor(actualCfg) - require.NoError(t, err) - require.NotNil(t, transport) - tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper) - require.True(t, ok) - require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter) - require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key) + require.NotNil(t, actualCfg) + + // test that the impersonation headers are appropriately injected into the request + // nolint:bodyclose + _, _ = actualCfg.WrapTransport(fakeRoundTripper(func(req *http.Request) (*http.Response, error) { + require.Equal(t, "system:serviceaccount:my-namespace:my-service-account", req.Header.Get("Impersonate-User")) + return &http.Response{}, nil + })).RoundTrip(&http.Request{}) } }) } diff --git a/internal/operator-controller/authentication/serviceaccount.go b/internal/operator-controller/authentication/serviceaccount.go new file mode 100644 index 0000000000..cd28fbf8fd --- /dev/null +++ b/internal/operator-controller/authentication/serviceaccount.go @@ -0,0 +1,17 @@ +package authentication + +import ( + "fmt" + + "k8s.io/client-go/transport" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +// ServiceAccountImpersonationConfig returns an ImpersonationConfig for impersonating a ServiceAccount. +// This allows authentication as a ServiceAccount without requiring an actual token. +func ServiceAccountImpersonationConfig(ext ocv1.ClusterExtension) transport.ImpersonationConfig { + return transport.ImpersonationConfig{ + UserName: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name), + } +} diff --git a/internal/operator-controller/authentication/serviceaccount_test.go b/internal/operator-controller/authentication/serviceaccount_test.go new file mode 100644 index 0000000000..6087d0d4f7 --- /dev/null +++ b/internal/operator-controller/authentication/serviceaccount_test.go @@ -0,0 +1,24 @@ +package authentication_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" +) + +func TestServiceAccountImpersonationConfig(t *testing.T) { + config := authentication.ServiceAccountImpersonationConfig(ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ext", + }, + Spec: ocv1.ClusterExtensionSpec{Namespace: "my-namespace", ServiceAccount: ocv1.ServiceAccountReference{Name: "my-service-account"}}, + }) + require.Equal(t, "system:serviceaccount:my-namespace:my-service-account", config.UserName) + require.Nil(t, config.Groups) + require.Empty(t, config.UID) + require.Empty(t, config.Extra) +} diff --git a/internal/operator-controller/authentication/tokengetter.go b/internal/operator-controller/authentication/tokengetter.go deleted file mode 100644 index 7870dc8e83..0000000000 --- a/internal/operator-controller/authentication/tokengetter.go +++ /dev/null @@ -1,128 +0,0 @@ -package authentication - -import ( - "context" - "fmt" - "sync" - "time" - - authenticationv1 "k8s.io/api/authentication/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/utils/ptr" -) - -type TokenGetter struct { - client corev1.ServiceAccountsGetter - expirationDuration time.Duration - tokens map[types.NamespacedName]*authenticationv1.TokenRequestStatus - mu sync.RWMutex -} - -type ServiceAccountNotFoundError struct { - ServiceAccountName string - ServiceAccountNamespace string - Err error -} - -func (e *ServiceAccountNotFoundError) Unwrap() error { - return e.Err -} - -// Error implements the error interface for ServiceAccountNotFoundError. -func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf("service account \"%s\" not found in namespace \"%s\": unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace) -} - -type TokenGetterOption func(*TokenGetter) - -const ( - rotationThresholdFraction = 0.1 - DefaultExpirationDuration = 5 * time.Minute -) - -// Returns a token getter that can fetch tokens given a service account. -// The token getter also caches tokens which helps reduce the number of requests to the API Server. -// In case a cached token is expiring a fresh token is created. -func NewTokenGetter(client corev1.ServiceAccountsGetter, options ...TokenGetterOption) *TokenGetter { - tokenGetter := &TokenGetter{ - client: client, - expirationDuration: DefaultExpirationDuration, - tokens: map[types.NamespacedName]*authenticationv1.TokenRequestStatus{}, - } - - for _, opt := range options { - opt(tokenGetter) - } - - return tokenGetter -} - -func WithExpirationDuration(expirationDuration time.Duration) TokenGetterOption { - return func(tg *TokenGetter) { - tg.expirationDuration = expirationDuration - } -} - -// Get returns a token from the cache if available and not expiring, otherwise creates a new token -func (t *TokenGetter) Get(ctx context.Context, key types.NamespacedName) (string, error) { - t.mu.RLock() - token, ok := t.tokens[key] - t.mu.RUnlock() - - expireTime := time.Time{} - if ok { - expireTime = token.ExpirationTimestamp.Time - } - - // Create a new token if the cached token expires within rotationThresholdFraction of expirationDuration from now - rotationThresholdAfterNow := metav1.Now().Add(time.Duration(float64(t.expirationDuration) * (rotationThresholdFraction))) - if expireTime.Before(rotationThresholdAfterNow) { - var err error - token, err = t.getToken(ctx, key) - if err != nil { - return "", err - } - t.mu.Lock() - t.tokens[key] = token - t.mu.Unlock() - } - - // Delete tokens that have expired - t.reapExpiredTokens() - - return token.Token, nil -} - -func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (*authenticationv1.TokenRequestStatus, error) { - req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, - key.Name, - &authenticationv1.TokenRequest{ - Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, - }, metav1.CreateOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name, ServiceAccountNamespace: key.Namespace} - } - return nil, err - } - return &req.Status, nil -} - -func (t *TokenGetter) reapExpiredTokens() { - t.mu.Lock() - defer t.mu.Unlock() - for key, token := range t.tokens { - if metav1.Now().Sub(token.ExpirationTimestamp.Time) > 0 { - delete(t.tokens, key) - } - } -} - -func (t *TokenGetter) Delete(key types.NamespacedName) { - t.mu.Lock() - defer t.mu.Unlock() - delete(t.tokens, key) -} diff --git a/internal/operator-controller/authentication/tokengetter_test.go b/internal/operator-controller/authentication/tokengetter_test.go deleted file mode 100644 index 663e95f1b3..0000000000 --- a/internal/operator-controller/authentication/tokengetter_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package authentication - -import ( - "context" - "fmt" - "testing" - "time" - - "github.com/stretchr/testify/assert" - authenticationv1 "k8s.io/api/authentication/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/fake" - ctest "k8s.io/client-go/testing" -) - -func TestTokenGetterGet(t *testing.T) { - fakeClient := fake.NewSimpleClientset() - fakeClient.PrependReactor("create", "serviceaccounts/token", - func(action ctest.Action) (bool, runtime.Object, error) { - act, ok := action.(ctest.CreateActionImpl) - if !ok { - return false, nil, nil - } - tokenRequest := act.GetObject().(*authenticationv1.TokenRequest) - var err error - if act.Name == "test-service-account-1" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-1", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(DefaultExpirationDuration)), - } - } - if act.Name == "test-service-account-2" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-2", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(1 * time.Second)), - } - } - if act.Name == "test-service-account-3" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-3", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(-10 * time.Second)), - } - } - if act.Name == "test-service-account-4" { - tokenRequest = nil - err = fmt.Errorf("error when fetching token") - } - return true, tokenRequest, err - }) - - tg := NewTokenGetter(fakeClient.CoreV1(), - WithExpirationDuration(DefaultExpirationDuration)) - - tests := []struct { - testName string - serviceAccountName string - namespace string - want string - errorMsg string - }{ - {"Testing getting token with fake client", "test-service-account-1", - "test-namespace-1", "test-token-1", "failed to get token"}, - {"Testing getting token from cache", "test-service-account-1", - "test-namespace-1", "test-token-1", "failed to get token"}, - {"Testing getting short lived token from fake client", "test-service-account-2", - "test-namespace-2", "test-token-2", "failed to get token"}, - {"Testing getting nearly expired token from cache", "test-service-account-2", - "test-namespace-2", "test-token-2", "failed to refresh token"}, - {"Testing token that expired 10 seconds ago", "test-service-account-3", - "test-namespace-3", "test-token-3", "failed to get token"}, - {"Testing error when getting token from fake client", "test-service-account-4", - "test-namespace-4", "error when fetching token", "error when fetching token"}, - {"Testing service account not found", "missing-sa", - "test-namespace-5", "", "service account \"missing-sa\" not found in namespace \"test-namespace-5\": unable to authenticate with the Kubernetes cluster."}, - } - - for _, tc := range tests { - got, err := tg.Get(context.Background(), types.NamespacedName{Namespace: tc.namespace, Name: tc.serviceAccountName}) - if err != nil { - t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, err) - assert.EqualError(t, err, tc.errorMsg, "Error message should match expected output") - } else { - t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, got) - assert.Equal(t, tc.want, got, tc.errorMsg) - } - } -} diff --git a/internal/operator-controller/authentication/tripper.go b/internal/operator-controller/authentication/tripper.go deleted file mode 100644 index 77bc1175c0..0000000000 --- a/internal/operator-controller/authentication/tripper.go +++ /dev/null @@ -1,38 +0,0 @@ -package authentication - -import ( - "fmt" - "net/http" - - "k8s.io/apimachinery/pkg/types" - utilnet "k8s.io/apimachinery/pkg/util/net" -) - -var _ http.RoundTripper = (*TokenInjectingRoundTripper)(nil) - -type TokenInjectingRoundTripper struct { - Tripper http.RoundTripper - TokenGetter *TokenGetter - Key types.NamespacedName -} - -func (tt *TokenInjectingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := tt.do(req) - if resp != nil && resp.StatusCode == http.StatusUnauthorized { - tt.TokenGetter.Delete(tt.Key) - resp, err = tt.do(req) - } - return resp, err -} - -func (tt *TokenInjectingRoundTripper) do(req *http.Request) (*http.Response, error) { - reqClone := utilnet.CloneRequest(req) - token, err := tt.TokenGetter.Get(reqClone.Context(), tt.Key) - if err != nil { - return nil, err - } - - // Always set the Authorization header to our retrieved token - reqClone.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - return tt.Tripper.RoundTrip(reqClone) -} diff --git a/internal/operator-controller/authorization/rbac_test.go b/internal/operator-controller/authorization/rbac_test.go index ed76138e36..bb8ac92ee4 100644 --- a/internal/operator-controller/authorization/rbac_test.go +++ b/internal/operator-controller/authorization/rbac_test.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) var ( @@ -415,7 +416,7 @@ func setupFakeClient(role client.Object) client.Client { } func saInfo(ext *ocv1.ClusterExtension) (*user.DefaultInfo, error) { - return &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)}, nil + return &user.DefaultInfo{Name: authentication.ServiceAccountImpersonationConfig(*ext).UserName}, nil } func TestPreAuthorize_Success(t *testing.T) { diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index d8d9d8de0e..5d9f797d5a 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -49,7 +49,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" @@ -218,12 +217,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl revisionStates, err := r.RevisionStatesGetter.GetRevisionStates(ctx, ext) if err != nil { setInstallStatus(ext, nil) - var saerr *authentication.ServiceAccountNotFoundError - if errors.As(err, &saerr) { - setInstalledStatusConditionUnknown(ext, saerr.Error()) - setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) - return ctrl.Result{}, err - } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) return ctrl.Result{}, err diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 20761aec91..3efbf5f064 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -28,7 +28,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/finalizers" @@ -346,61 +345,6 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } -func TestClusterExtensionServiceAccountNotFound(t *testing.T) { - cl, reconciler := newClientAndReconciler(t) - reconciler.RevisionStatesGetter = &MockRevisionStatesGetter{ - Err: &authentication.ServiceAccountNotFoundError{ - ServiceAccountName: "missing-sa", - ServiceAccountNamespace: "default", - }} - - ctx := context.Background() - extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - - t.Log("Given a cluster extension with a missing service account") - clusterExtension := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, - Spec: ocv1.ClusterExtensionSpec{ - Source: ocv1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1.CatalogFilter{ - PackageName: "test-package", - }, - }, - Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "missing-sa", - }, - }, - } - - require.NoError(t, cl.Create(ctx, clusterExtension)) - - t.Log("When reconciling the cluster extension") - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - - require.Equal(t, ctrl.Result{}, res) - require.Error(t, err) - var saErr *authentication.ServiceAccountNotFoundError - require.ErrorAs(t, err, &saErr) - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) - - t.Log("By checking the status conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionUnknown, installedCond.Status) - require.Contains(t, installedCond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.", - "missing-sa", "default")) - - progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason) - require.Contains(t, progressingCond.Message, "installation cannot proceed due to missing ServiceAccount") - require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) -} - func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { cl, reconciler := newClientAndReconciler(t) reconciler.ImagePuller = &imageutil.MockPuller{ diff --git a/manifests/standard-e2e.yaml b/manifests/standard-e2e.yaml index b5bdda4e35..0f81669e2d 100644 --- a/manifests/standard-e2e.yaml +++ b/manifests/standard-e2e.yaml @@ -1333,9 +1333,9 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate - apiGroups: - apiextensions.k8s.io resources: diff --git a/manifests/standard.yaml b/manifests/standard.yaml index a1f02c7610..a5df5e3eb1 100644 --- a/manifests/standard.yaml +++ b/manifests/standard.yaml @@ -1298,9 +1298,9 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate - apiGroups: - apiextensions.k8s.io resources: