You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2026/09/14 16:12:38 UTC
(superset-kubernetes-operator) branch main updated: fix(rbac): grant supersets/finalizers for owner references (#359)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/superset-kubernetes-operator.git
The following commit(s) were added to refs/heads/main by this push:
new ea2d200 fix(rbac): grant supersets/finalizers for owner references (#359)
ea2d200 is described below
commit ea2d200e9f910d00559622fb0504aa9e6a2730a1
Author: Ville Brofeldt <33...@users.noreply.github.com>
AuthorDate: Mon Sep 14 09:12:33 2026 -0700
fix(rbac): grant supersets/finalizers for owner references (#359)
* fix(rbac): grant supersets/finalizers for owner references
The operator stamps blockOwnerDeletion owner references on every child
object it creates via controllerutil.SetControllerReference. When the
OwnerReferencesPermissionEnforcement admission plugin is active — which
it is by default on OpenShift — the API server rejects those references
unless the operator can write the owning CR's finalizers subresource,
failing reconciliation with:
cannot set blockOwnerDeletion if an ownerReference refers to a
resource you can't set finalizers on
Grant `update` on `supersets/finalizers` via a kubebuilder marker
(regenerated into config/rbac/role.yaml) and mirror the rule in the
hand-maintained Helm managerRules helper so both the cluster-scoped
ClusterRole and per-namespace Roles carry it.
Add a regression test asserting the grant exists, and a drift guard that
compares the generated ClusterRole rules against the Helm managerRules
so the two independent RBAC sources can no longer silently diverge.
Fixes #357
* docs(changelog): note supersets/finalizers RBAC fix
* test(helm): update RBAC snapshots for supersets/finalizers
* docs(security): document supersets/finalizers RBAC grant
---
charts/superset-operator/templates/_helpers.tpl | 3 +
.../tests/__snapshot__/full_options_test.yaml.snap | 12 +++
.../tests/__snapshot__/minimal_test.yaml.snap | 6 ++
config/rbac/role.yaml | 6 ++
docs/reference/releases.md | 4 +
docs/reference/security.md | 1 +
internal/controller/security_test.go | 120 ++++++++++++++++++---
internal/controller/superset_controller.go | 1 +
8 files changed, 138 insertions(+), 15 deletions(-)
diff --git a/charts/superset-operator/templates/_helpers.tpl b/charts/superset-operator/templates/_helpers.tpl
index 0fbf7b3..8092a0f 100644
--- a/charts/superset-operator/templates/_helpers.tpl
+++ b/charts/superset-operator/templates/_helpers.tpl
@@ -150,6 +150,9 @@ desired indentation.
- apiGroups: [superset.apache.org]
resources: [supersets/status]
verbs: [get, patch, update]
+- apiGroups: [superset.apache.org]
+ resources: [supersets/finalizers]
+ verbs: [update]
- apiGroups: [superset.apache.org]
resources: [supersets]
verbs: [get, list, patch, watch]
diff --git a/charts/superset-operator/tests/__snapshot__/full_options_test.yaml.snap b/charts/superset-operator/tests/__snapshot__/full_options_test.yaml.snap
index db8c5af..a391e22 100644
--- a/charts/superset-operator/tests/__snapshot__/full_options_test.yaml.snap
+++ b/charts/superset-operator/tests/__snapshot__/full_options_test.yaml.snap
@@ -448,6 +448,12 @@ renders the full install manifests:
- get
- patch
- update
+ - apiGroups:
+ - superset.apache.org
+ resources:
+ - supersets/finalizers
+ verbs:
+ - update
- apiGroups:
- superset.apache.org
resources:
@@ -594,6 +600,12 @@ renders the full install manifests:
- get
- patch
- update
+ - apiGroups:
+ - superset.apache.org
+ resources:
+ - supersets/finalizers
+ verbs:
+ - update
- apiGroups:
- superset.apache.org
resources:
diff --git a/charts/superset-operator/tests/__snapshot__/minimal_test.yaml.snap b/charts/superset-operator/tests/__snapshot__/minimal_test.yaml.snap
index 6a69d8c..209ea3a 100644
--- a/charts/superset-operator/tests/__snapshot__/minimal_test.yaml.snap
+++ b/charts/superset-operator/tests/__snapshot__/minimal_test.yaml.snap
@@ -135,6 +135,12 @@ renders the default install manifests:
- get
- patch
- update
+ - apiGroups:
+ - superset.apache.org
+ resources:
+ - supersets/finalizers
+ verbs:
+ - update
- apiGroups:
- superset.apache.org
resources:
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
index 3ca369d..e250417 100644
--- a/config/rbac/role.yaml
+++ b/config/rbac/role.yaml
@@ -128,6 +128,12 @@ rules:
- list
- patch
- watch
+- apiGroups:
+ - superset.apache.org
+ resources:
+ - supersets/finalizers
+ verbs:
+ - update
- apiGroups:
- superset.apache.org
resources:
diff --git a/docs/reference/releases.md b/docs/reference/releases.md
index d5aa49e..f2bb747 100644
--- a/docs/reference/releases.md
+++ b/docs/reference/releases.md
@@ -27,6 +27,10 @@ This page tracks notable changes in Apache Superset Kubernetes Operator releases
- Kubernetes support now covers the three newest `kind`-published minor versions instead of two. CI tests Kubernetes 1.37, 1.36, and 1.35 natively, with the experimental `next` lane disabled again ([#317](https://github.com/apache/superset-kubernetes-operator/pull/317)).
+### Fixed
+
+- The operator now requests `update` on `supersets/finalizers`, so reconciliation no longer fails on clusters with the `OwnerReferencesPermissionEnforcement` admission plugin enabled (e.g. OpenShift), where the API server previously rejected the `blockOwnerDeletion` owner references the operator sets on child resources ([#359](https://github.com/apache/superset-kubernetes-operator/pull/359), [@villebro](https://github.com/villebro)).
+
### Security
- **Breaking:** CRs with `serviceAccount.create=true` (or unset) and `serviceAccount.name != metadata.name` are now rejected at admission. To use a pre-existing ServiceAccount with a different name, set `serviceAccount.create=false` ([#324](https://github.com/apache/superset-kubernetes-operator/pull/324), [@villebro](https://github.com/villebro)).
diff --git a/docs/reference/security.md b/docs/reference/security.md
index 1656258..17ebaf9 100644
--- a/docs/reference/security.md
+++ b/docs/reference/security.md
@@ -131,6 +131,7 @@ The operator runs with a `ClusterRole` to support managing `Superset` instances
| `tokenreviews`, `subjectaccessreviews` | create | Metrics endpoint auth/authz (controller-runtime secure metrics) |
| `supersets` | get, list, watch, patch | Reads `Superset` CRs and patches metadata to consume target-bound supervised-upgrade approval; Kubernetes RBAC is not field-scoped |
| `supersets/status` | get, update, patch | Updates reconciliation status only |
+| `supersets/finalizers` | update | Required to set `blockOwnerDeletion` owner references on parent-owned resources when the `OwnerReferencesPermissionEnforcement` admission plugin is enabled (e.g. OpenShift) |
The operator does **not** request:
diff --git a/internal/controller/security_test.go b/internal/controller/security_test.go
index 04c4c17..58b7fe9 100644
--- a/internal/controller/security_test.go
+++ b/internal/controller/security_test.go
@@ -20,7 +20,11 @@ package controller
import (
"context"
+ "fmt"
"os"
+ "regexp"
+ "slices"
+ "sort"
"strings"
"testing"
@@ -156,21 +160,7 @@ func TestReconcile_SecretsNeverLeakIntoConfigMaps(t *testing.T) {
// artifact that is actually deployed — so an accidental `+kubebuilder:rbac`
// marker that broadens scope is caught by `make codegen` + this test.
func TestManagerRole_GrantsNoSecretsAccessOrWildcards(t *testing.T) {
- // Tests run from the package directory; the role manifest lives at repo root.
- const rolePath = "../../config/rbac/role.yaml"
-
- raw, err := os.ReadFile(rolePath)
- if err != nil {
- t.Fatalf("read %s: %v", rolePath, err)
- }
-
- role := &rbacv1.ClusterRole{}
- if err := yaml.Unmarshal(raw, role); err != nil {
- t.Fatalf("unmarshal ClusterRole: %v", err)
- }
- if len(role.Rules) == 0 {
- t.Fatal("expected manager ClusterRole to define rules")
- }
+ role := loadManagerClusterRole(t)
for i, rule := range role.Rules {
for _, res := range rule.Resources {
@@ -193,3 +183,103 @@ func TestManagerRole_GrantsNoSecretsAccessOrWildcards(t *testing.T) {
}
}
}
+
+// TestManagerRole_GrantsFinalizersForOwnerReferences guards against a
+// regression of issue #357. The operator stamps blockOwnerDeletion owner
+// references on every child object via controllerutil.SetControllerReference;
+// the OwnerReferencesPermissionEnforcement admission plugin (enabled by default
+// on OpenShift) rejects those unless the operator can write the owning CR's
+// finalizers subresource, which requires this RBAC grant.
+func TestManagerRole_GrantsFinalizersForOwnerReferences(t *testing.T) {
+ role := loadManagerClusterRole(t)
+
+ if !ruleGrants(role.Rules, "superset.apache.org", "supersets/finalizers", "update") {
+ t.Errorf("manager ClusterRole must grant 'update' on supersets/finalizers so blockOwnerDeletion "+
+ "owner references are accepted (issue #357); rules: %+v", role.Rules)
+ }
+}
+
+// TestHelmManagerRulesMatchGeneratedClusterRole guards against RBAC drift
+// between the two independent sources of the operator's manager rules: the
+// generated config/rbac/role.yaml (from +kubebuilder:rbac markers) and the
+// hand-maintained Helm helper. They render into the deployed ClusterRole and
+// per-namespace Roles respectively, so a rule added to one but not the other
+// silently under- or over-privileges one install path.
+func TestHelmManagerRulesMatchGeneratedClusterRole(t *testing.T) {
+ generated := loadManagerClusterRole(t)
+
+ const helpersPath = "../../charts/superset-operator/templates/_helpers.tpl"
+ raw, err := os.ReadFile(helpersPath)
+ if err != nil {
+ t.Fatalf("read %s: %v", helpersPath, err)
+ }
+
+ // The managerRules define block is static YAML with no template directives,
+ // so it can be parsed directly without rendering the chart.
+ re := regexp.MustCompile(`(?s){{- define "superset-operator\.managerRules" -}}\n(.*?)\n{{- end }}`)
+ m := re.FindStringSubmatch(string(raw))
+ if m == nil {
+ t.Fatal("could not locate superset-operator.managerRules define block in _helpers.tpl")
+ }
+
+ var helmRules []rbacv1.PolicyRule
+ if err := yaml.Unmarshal([]byte(m[1]), &helmRules); err != nil {
+ t.Fatalf("unmarshal Helm managerRules: %v", err)
+ }
+
+ want := normalizeRules(generated.Rules)
+ got := normalizeRules(helmRules)
+ if !slices.Equal(want, got) {
+ t.Errorf("Helm managerRules drifted from the generated ClusterRole; run `make manifests` and reconcile _helpers.tpl.\ngenerated: %v\nhelm: %v", want, got)
+ }
+}
+
+// loadManagerClusterRole reads the generated manager ClusterRole — the
+// source-of-truth RBAC artifact that is actually deployed. Tests run from the
+// package directory; the role manifest lives at repo root.
+func loadManagerClusterRole(t *testing.T) *rbacv1.ClusterRole {
+ t.Helper()
+
+ const rolePath = "../../config/rbac/role.yaml"
+ raw, err := os.ReadFile(rolePath)
+ if err != nil {
+ t.Fatalf("read %s: %v", rolePath, err)
+ }
+
+ role := &rbacv1.ClusterRole{}
+ if err := yaml.Unmarshal(raw, role); err != nil {
+ t.Fatalf("unmarshal ClusterRole: %v", err)
+ }
+ if len(role.Rules) == 0 {
+ t.Fatal("expected manager ClusterRole to define rules")
+ }
+ return role
+}
+
+// ruleGrants reports whether any rule grants verb on group/resource.
+func ruleGrants(rules []rbacv1.PolicyRule, group, resource, verb string) bool {
+ for _, r := range rules {
+ if slices.Contains(r.APIGroups, group) && slices.Contains(r.Resources, resource) && slices.Contains(r.Verbs, verb) {
+ return true
+ }
+ }
+ return false
+}
+
+// normalizeRules renders rules into a set-comparable form, sorting fields within
+// each rule and the rules themselves so ordering differences (controller-gen
+// sorts; the Helm helper preserves authored order) don't cause false diffs.
+func normalizeRules(rules []rbacv1.PolicyRule) []string {
+ out := make([]string, 0, len(rules))
+ for _, r := range rules {
+ groups := slices.Clone(r.APIGroups)
+ resources := slices.Clone(r.Resources)
+ verbs := slices.Clone(r.Verbs)
+ sort.Strings(groups)
+ sort.Strings(resources)
+ sort.Strings(verbs)
+ out = append(out, fmt.Sprintf("apiGroups=%v resources=%v verbs=%v", groups, resources, verbs))
+ }
+ sort.Strings(out)
+ return out
+}
diff --git a/internal/controller/superset_controller.go b/internal/controller/superset_controller.go
index e39e2c5..ab09898 100644
--- a/internal/controller/superset_controller.go
+++ b/internal/controller/superset_controller.go
@@ -59,6 +59,7 @@ type SupersetReconciler struct {
// +kubebuilder:rbac:groups=superset.apache.org,resources=supersets,verbs=get;list;watch;patch
// +kubebuilder:rbac:groups=superset.apache.org,resources=supersets/status,verbs=get;update;patch
+// +kubebuilder:rbac:groups=superset.apache.org,resources=supersets/finalizers,verbs=update
// +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch;update
// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch