support deny list in operator by AdheipSingh · Pull Request #729 · StarRocks/starrocks-kubernetes-operator

AdheipSingh · 2025-12-03T16:53:00Z

Description

Introduce Deny List #728

Checklist

For operator, please complete the following checklist:

run make generate to generate the code.
run golangci-lint run to check the code style.
run make test to run UT.
run make manifests to update the yaml files of CRD.

For helm chart, please complete the following checklist:

make sure you have updated the values.yaml
file of starrocks chart.
In scripts directory, run bash create-parent-chart-values.sh to update the values.yaml file of the parent
chart( kube-starrocks chart).

CLAassistant · 2025-12-03T16:53:08Z

All committers have signed the CLA.

Signed-off-by: AdheipSingh <adheip1222@gmail.com>

AdheipSingh · 2025-12-17T09:19:47Z

Hi @yandongxiao can you help in reviewing this.

AdheipSingh · 2026-01-09T02:02:13Z

Hi @yandongxiao any review on this :)

kevincai · 2026-01-21T08:29:53Z

pkg/predicates/predicates.go

+
+// ignoreNamespacePredicate returns false if the object is in a denied namespace
+func ignoreNamespacePredicate(obj client.Object) bool {
+	namespaces := getEnvAsSlice("DENY_LIST", nil, ",")


wondering why choose env vars to pass down this info to operator? how about commandline option?

Also would like to know the usage scenario for this deny list, are there too many sr cluster managed by the same operator?

Hi @kevincai , sure we can do cli option also. Yes, there too many sr cluster managed by the same operator.

Copilot

Pull request overview

This pull request introduces deny list functionality to the StarRocks Kubernetes operator, allowing administrators to exclude specific namespaces from reconciliation. The implementation uses a new predicates package with event filters that check both namespace deny lists and an ignored annotation.

Changes:

Added new pkg/predicates package with GenericPredicates event filter that respects namespace deny lists and ignored annotations
Integrated predicates into both StarRocksCluster and StarRocksWarehouse controllers via WithEventFilter
Updated Helm charts to support configuring the deny list via the denyList value and DENY_LIST environment variable

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
pkg/predicates/predicates.go	New package implementing predicate logic for filtering events based on namespace deny list and ignored annotation
pkg/predicates/predicates_test.go	Comprehensive test coverage for predicate functionality including Create, Update, Delete, and Generic events
pkg/controllers/controllers.go	Integration of GenericPredicates event filter into both cluster and warehouse reconcilers
helm-charts/charts/kube-starrocks/values.yaml	Added denyList configuration field with documentation
helm-charts/charts/kube-starrocks/charts/operator/values.yaml	Added denyList configuration field with documentation
helm-charts/charts/kube-starrocks/charts/operator/templates/deployment.yaml	Added DENY_LIST environment variable configuration
Makefile	Added predicates package to test coverage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T08:31:20Z

pkg/predicates/predicates_test.go

+				os.Setenv("DENY_LIST", tt.envDenyList)
+				defer os.Unsetenv("DENY_LIST")


Tests that modify environment variables may have race conditions when run in parallel. Consider using t.Setenv() instead of os.Setenv() and os.Unsetenv(), as t.Setenv() automatically handles cleanup and prevents parallel execution of tests that modify the same environment variable. This is the recommended approach in Go 1.17+ for test isolation.

Copilot · 2026-01-21T08:31:20Z

pkg/predicates/predicates.go

+}
+
+// ignoreIgnoredObjectPredicate returns false if the object has the ignored annotation set to "true"
+func ignoreIgnoredObjectPredicate(obj client.Object) bool {


The function name "ignoreIgnoredObjectPredicate" is confusing because it returns true when the object should NOT be ignored (i.e., when reconciliation should proceed). The double negative ("ignore" + "ignored") makes this particularly hard to understand. Consider renaming to "shouldReconcileObject" or "isObjectAllowed" to better reflect its actual behavior and improve code readability.

Copilot · 2026-01-21T08:31:20Z

pkg/predicates/predicates.go

+
+// ignoreNamespacePredicate returns false if the object is in a denied namespace
+func ignoreNamespacePredicate(obj client.Object) bool {
+	namespaces := getEnvAsSlice("DENY_LIST", nil, ",")


Reading and parsing the environment variable on every predicate call may impact performance, as predicates are invoked frequently. Consider caching the parsed deny list at initialization time (e.g., in a package-level variable or during GenericPredicates construction) and only re-reading if a reload mechanism is needed. This would avoid repeated string parsing on every reconciliation event.

Copilot · 2026-01-21T08:31:21Z

pkg/predicates/predicates_test.go

+			// Set environment variable if needed
+			if tt.envDenyList != "" {
+				os.Setenv("DENY_LIST", tt.envDenyList)
+				defer os.Unsetenv("DENY_LIST")


Tests should explicitly clean up the DENY_LIST environment variable even when envDenyList is empty to prevent pollution from previous tests. Consider adding an unconditional defer os.Unsetenv("DENY_LIST") or using t.Setenv() which handles this automatically. This ensures test isolation regardless of execution order.

Suggested change

// Set environment variable if needed

if tt.envDenyList != "" {

os.Setenv("DENY_LIST", tt.envDenyList)

defer os.Unsetenv("DENY_LIST")

// Ensure DENY_LIST is cleaned up after each subtest

defer os.Unsetenv("DENY_LIST")

// Set environment variable if needed

if tt.envDenyList != "" {

os.Setenv("DENY_LIST", tt.envDenyList)

Copilot · 2026-01-21T08:31:21Z

helm-charts/charts/kube-starrocks/values.yaml

    # the operator watching all namespaces uses too many memory resources, you can set this value.
    # Defaults to all namespaces.
    watchNamespace: ""
+    # Comma-separated list of namespaces to exclude from reconciliation


The documentation should clarify how denyList interacts with watchNamespace. When watchNamespace is set, it restricts the operator to a single namespace, so the denyList would have no effect (or vice versa, if a namespace is both watched and denied). Consider adding a comment explaining this relationship to help users understand the interaction between these two configuration options.

Suggested change

# Comma-separated list of namespaces to exclude from reconciliation

# Comma-separated list of namespaces to exclude from reconciliation when the operator watches

# multiple namespaces (i.e. when watchNamespace is empty and all namespaces are watched).

# Note: If watchNamespace is set to a non-empty namespace, the operator only watches that single

# namespace and denyList has no additional effect. If that same namespace is included in denyList,

# the operator will effectively not reconcile any namespaces.

Copilot · 2026-01-21T08:31:22Z

pkg/predicates/predicates.go

+}
+
+// ignoreNamespacePredicate returns false if the object is in a denied namespace
+func ignoreNamespacePredicate(obj client.Object) bool {


The function name "ignoreNamespacePredicate" is confusing because it returns true when the namespace should NOT be ignored (i.e., when reconciliation should proceed). Consider renaming to "shouldReconcileNamespace" or "isNamespaceAllowed" to better reflect its actual behavior and make the code more readable.

Copilot · 2026-01-21T08:31:22Z

pkg/predicates/predicates.go

+// Delete returns true if the Delete event should be processed
+func (GenericPredicates) Delete(e event.DeleteEvent) bool {
+	return shouldReconcile(e.Object)
+}
+
+// Generic returns true if the Generic event should be processed
+func (GenericPredicates) Generic(e event.GenericEvent) bool {
+	return shouldReconcile(e.Object)


The Delete and Generic event handling is not thoroughly tested. While TestShouldReconcile_WithNilObject tests nil objects, there are no tests that verify Delete and Generic events properly respect the deny list and ignored annotation. Consider adding comprehensive test cases for these event types similar to the Create and Update tests.

Copilot · 2026-01-21T08:31:22Z

pkg/predicates/predicates_test.go

+			// Set environment variable if needed
+			if tt.envDenyList != "" {
+				os.Setenv("DENY_LIST", tt.envDenyList)
+				defer os.Unsetenv("DENY_LIST")
+			}


Tests should explicitly clean up the DENY_LIST environment variable even when envDenyList is empty to prevent pollution from previous tests. Consider adding an unconditional defer os.Unsetenv("DENY_LIST") or using t.Setenv() which handles this automatically. This ensures test isolation regardless of execution order.

Suggested change

// Set environment variable if needed

if tt.envDenyList != "" {

os.Setenv("DENY_LIST", tt.envDenyList)

defer os.Unsetenv("DENY_LIST")

}

// Ensure DENY_LIST is controlled for this subtest

t.Setenv("DENY_LIST", tt.envDenyList)

Copilot · 2026-01-21T08:31:23Z

pkg/predicates/predicates_test.go

+			os.Setenv("TEST_ENV", tt.envValue)
+			defer os.Unsetenv("TEST_ENV")


Tests should explicitly clean up the TEST_ENV environment variable even when the test completes to prevent pollution. Consider using t.Setenv() instead which handles cleanup automatically and is the recommended approach in Go 1.17+ for test isolation.

Suggested change

os.Setenv("TEST_ENV", tt.envValue)

defer os.Unsetenv("TEST_ENV")

t.Setenv("TEST_ENV", tt.envValue)

Copilot · 2026-01-21T08:31:23Z

helm-charts/charts/kube-starrocks/charts/operator/values.yaml

+  # Comma-separated list of namespaces to exclude from reconciliation
+  # Example: "kube-system,kube-public,monitoring"
+  denyList: ""
  # Additional operator container environment variables
  # You specify this manually like you would a raw deployment manifest.
  # Ref: https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/
  # operator supports the following environment variables:
  # KUBE_STARROCKS_UNSUPPORTED_ENVS: "XXX,YYY" # The environment variables that will not be passed to starrocks container.
+  # DENY_LIST: "namespace1,namespace2" # Comma-separated list of namespaces to exclude from reconciliation


The documentation should clarify how denyList interacts with watchNamespace. When watchNamespace is set, it restricts the operator to a single namespace, so the denyList would have no effect (or vice versa, if a namespace is both watched and denied). Consider adding a comment explaining this relationship to help users understand the interaction between these two configuration options.

Suggested change

# Comma-separated list of namespaces to exclude from reconciliation

# Example: "kube-system,kube-public,monitoring"

denyList: ""

# Additional operator container environment variables

# You specify this manually like you would a raw deployment manifest.

# Ref: https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/

# operator supports the following environment variables:

# KUBE_STARROCKS_UNSUPPORTED_ENVS: "XXX,YYY" # The environment variables that will not be passed to starrocks container.

# DENY_LIST: "namespace1,namespace2" # Comma-separated list of namespaces to exclude from reconciliation

# Comma-separated list of namespaces to exclude from reconciliation.

# This only applies to namespaces that the operator would otherwise watch.

# When watchNamespace is empty (default, watch all namespaces), denyList can be

# used to skip specific namespaces. When watchNamespace is set to a single

# namespace, excluding other namespaces via denyList has no additional effect.

# If the same namespace is specified in both watchNamespace and denyList, the

# watchNamespace setting takes precedence and the denyList entry is effectively

# redundant.

denyList: ""

# Additional operator container environment variables

# You specify this manually like you would a raw deployment manifest.

# Ref: https://kubernetes.io/docs/tasks/inject-data-application/define-environment-variable-container/

# operator supports the following environment variables:

# KUBE_STARROCKS_UNSUPPORTED_ENVS: "XXX,YYY" # The environment variables that will not be passed to starrocks container.

# DENY_LIST: "namespace1,namespace2" # Comma-separated list of namespaces to exclude from reconciliation.

# This environment variable has the same effect as .Values.starrocksOperator.denyList:

# it only excludes namespaces that the operator would otherwise watch. When

# watchNamespace is set to a single namespace, DENY_LIST entries for other

# namespaces have no additional effect, and listing the watched namespace

# itself is redundant.

AdheipSingh · 2026-02-03T11:28:33Z

@kevincai i have addressed your comments.

Signed-off-by: AdheipSingh <adheip1222@gmail.com>

AdheipSingh added 2 commits December 3, 2025 22:45

support deny list in operator

305889b

Signed-off-by: AdheipSingh <adheip1222@gmail.com>

fix: use controller-runtime logging and update parent chart values

0269941

Signed-off-by: AdheipSingh <adheip1222@gmail.com>

AdheipSingh force-pushed the Fix-728-DenyList branch from e0c8806 to 0269941 Compare December 3, 2025 17:16

kevincai requested a review from Copilot January 21, 2026 08:26

Copilot started reviewing on behalf of kevincai January 21, 2026 08:27 View session

kevincai reviewed Jan 21, 2026

View reviewed changes

Copilot AI reviewed Jan 21, 2026

View reviewed changes

use CLI flag instead of env var for deny list

26a4a1a

Signed-off-by: AdheipSingh <adheip1222@gmail.com>

AdheipSingh force-pushed the Fix-728-DenyList branch from 668eb59 to 26a4a1a Compare February 3, 2026 11:30

		os.Setenv("DENY_LIST", tt.envDenyList)
		defer os.Unsetenv("DENY_LIST")

-    # Comma-separated list of namespaces to exclude from reconciliation
+    # Comma-separated list of namespaces to exclude from reconciliation when the operator watches
+    # multiple namespaces (i.e. when watchNamespace is empty and all namespaces are watched).
+    # Note: If watchNamespace is set to a non-empty namespace, the operator only watches that single
+    # namespace and denyList has no additional effect. If that same namespace is included in denyList,
+    # the operator will effectively not reconcile any namespaces.

		os.Setenv("TEST_ENV", tt.envValue)
		defer os.Unsetenv("TEST_ENV")

	os.Setenv("TEST_ENV", tt.envValue)
	defer os.Unsetenv("TEST_ENV")
	t.Setenv("TEST_ENV", tt.envValue)

Conversation

AdheipSingh commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

CLAassistant commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdheipSingh commented Dec 17, 2025

Uh oh!

AdheipSingh commented Jan 9, 2026

Uh oh!

kevincai Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

AdheipSingh Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

AdheipSingh commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AdheipSingh commented Dec 3, 2025 •

edited

Loading

CLAassistant commented Dec 3, 2025 •

edited

Loading