Configure the operator using a ConfigMap instead of a custom resource#41
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors controller reconciliation and cleanup behavior by moving “stale user/role” deletion policy to the singleton Controller settings (instead of per-resource PostgresAccess.spec.cleanupPolicy), while also reorganizing and expanding the e2e test utilities and updating tests to validate the new default behavior.
Changes:
- Add controller-scoped stale-user deletion policies for Postgres/Redis/RabbitMQ and update controllers/finalizers to honor them (defaulting to safe “Restrict”).
- Remove
PostgresAccess.spec.cleanupPolicyfrom API/CRD and adjust controller + unit/e2e tests accordingly. - Refactor shared reconciliation/secret/connection helpers and reorganize e2e helper code into
test/e2e/utils(including renamingrabbitMQpackage torabbitmq).
Reviewed changes
Copilot reviewed 33 out of 41 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit_utils.go | Moves shared unit test helpers to package test and reformats fake client setup. |
| test/e2e/utils/utils.go | New shared e2e command/cluster utility helpers (Run, cert-manager install, manifest editing). |
| test/e2e/utils/e2e.go | New e2e helpers for kubectl apply/wait, secret polling/decoding, token requests, reconciliation triggers. |
| test/e2e/utils/e2e_redis.go | New Redis-specific e2e helper functions (deploy, ACL checks, resource creation/deletion). |
| test/e2e/utils/e2e_rabbitmq.go | New RabbitMQ-specific e2e helper functions (deploy, management ops, permissions checks). |
| test/e2e/utils/e2e_postgres.go | Adjusts Postgres e2e helpers for new policy model and adds table-missing helper. |
| test/e2e/test_helpers_test.go | Switches e2e helper usage to new test/e2e/utils package. |
| test/e2e/redis_e2e_test.go | Updates Redis e2e tests to new utils package and default “retain stale user” behavior. |
| test/e2e/redis_controller_policy_e2e_test.go | Adds/updates Redis controller-policy e2e scenarios, including stale-user deletion policy. |
| test/e2e/rabbitmq_e2e_test.go | Updates RabbitMQ e2e tests for new default behavior and adds stale-user deletion policy coverage. |
| test/e2e/postgres_e2e_test.go | Updates Postgres e2e tests to controller-scoped stale-user policy; adds schema rejection test for removed field. |
| test/e2e/parallel_support_test.go | Updates parallel support helpers to use the new e2e utils package. |
| test/e2e/manager_e2e_test.go | Updates manager e2e tests to use the new e2e utils package. |
| test/e2e/e2e_suite_test.go | Updates suite setup/teardown to use new e2e utils; keeps cert-manager orchestration. |
| README.md | Documents controller-scoped stale-user cleanup policies and their semantics. |
| internal/controller/shared_secret.go | Adds shared reconciliation helpers for generated secrets and existing-secret connection resolution. |
| internal/controller/shared_reconciliation.go | Introduces generic reconcile/finalize/status+event framework used by access controllers. |
| internal/controller/shared_config.go | Adds shared config utilities for singleton Controller settings and connection secret namespace policy. |
| internal/controller/internal_shared.go | Consolidates shared helpers (singleton Controller resolution, secret value resolution, host qualification, etc.). |
| internal/controller/redis/redisaccess_controller.go | Applies stale-user deletion policy to Redis reconciliation and finalization behavior. |
| internal/controller/redis/redis_connection.go | Adds Redis stale-user deletion policy resolution from Controller settings. |
| internal/controller/redis/redisaccess_controller_test.go | Updates tests to new test helpers and adds stale-user deletion policy coverage. |
| internal/controller/rabbitmq/suite_test.go | Renames package to rabbitmq for consistency. |
| internal/controller/rabbitmq/rabbitmq.go | Renames package to rabbitmq. |
| internal/controller/rabbitmq/rabbitmq_connection.go | Renames package and adds stale-user deletion policy resolution. |
| internal/controller/rabbitmq/rabbitmqaccess_controller.go | Renames package and applies stale-user deletion policy to reconciliation/finalization. |
| internal/controller/rabbitmq/rabbitmqaccess_controller_test.go | Updates tests to new test helpers and adds stale-user deletion policy coverage. |
| internal/controller/postgres/postgresaccess_controller.go | Moves cleanup behavior to controller-scoped policy and updates stale-user handling logic. |
| internal/controller/postgres/postgresaccess_connection.go | Adds Postgres stale-user deletion policy resolution from Controller settings. |
| internal/controller/postgres/postgresaccess_controller_test.go | Updates tests to new test helpers and adds extensive stale-user policy coverage. |
| internal/controller/postgres/db.go | Changes DropUser policy type to PostgresCleanupPolicy and adds explicit None handling. |
| internal/controller/postgres/mock_db.go | Updates mock DB DropUser signature and records last cleanup policy used. |
| internal/controller/controller_controller_test.go | Updates tests to use new test helper package for fake clients/event collection. |
| config/samples/access_v1_controller.yaml | Extends sample Controller with new stale-user deletion policy fields. |
| config/crd/bases/access.k8s.delta10.nl_postgresaccesses.yaml | Removes spec.cleanupPolicy from PostgresAccess CRD schema. |
| config/crd/bases/access.k8s.delta10.nl_controllers.yaml | Adds stale-user deletion policy fields to Controller CRD schema. |
| cmd/main.go | Updates import path after RabbitMQ controller package rename. |
| api/v1/postgresaccess_types.go | Replaces CleanupPolicy type with PostgresCleanupPolicy and removes PostgresAccess.spec.cleanupPolicy. |
| api/v1/controller_types.go | Adds StaleUserDeletionPolicy and new controller settings fields for Redis/RabbitMQ/Postgres. |
| api/v1/controller_deepcopy.go | Fixes deep copy coverage for added Controller settings pointer fields. |
| api/v1/zz_generated.deepcopy.go | Updates generated deep-copies for removed/added API fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+34
| excludedUsers: | ||
| - postgres | ||
| # Default is Restrict. | ||
| # Controls whether stale PostgreSQL roles are deleted or retained. | ||
| # None disables stale-user cleanup outside of finalization for the | ||
| # specific PostgresAccess being deleted. | ||
| staleUserDeletionPolicy: Restrict | ||
| # Configure RabbitMQ-specific controller behavior. | ||
| rabbitmq: | ||
| # Default is Restrict. | ||
| staleUserDeletionPolicy: Restrict | ||
| # Default is Retain. | ||
| staleVhostDeletionPolicy: Retain | ||
| # Configure Redis-specific controller behavior. | ||
| redis: | ||
| # Default is Restrict. | ||
| staleUserDeletionPolicy: Restrict |
There was a problem hiding this comment.
The generated sample YAML has inconsistent indentation under spec.settings (e.g., excludedUsers is indented one extra space), which can make the sample invalid or misleading to users. Align nested keys to consistent 2-space indentation so the file parses cleanly.
Suggested change
| excludedUsers: | |
| - postgres | |
| # Default is Restrict. | |
| # Controls whether stale PostgreSQL roles are deleted or retained. | |
| # None disables stale-user cleanup outside of finalization for the | |
| # specific PostgresAccess being deleted. | |
| staleUserDeletionPolicy: Restrict | |
| # Configure RabbitMQ-specific controller behavior. | |
| rabbitmq: | |
| # Default is Restrict. | |
| staleUserDeletionPolicy: Restrict | |
| # Default is Retain. | |
| staleVhostDeletionPolicy: Retain | |
| # Configure Redis-specific controller behavior. | |
| redis: | |
| # Default is Restrict. | |
| staleUserDeletionPolicy: Restrict | |
| excludedUsers: | |
| - postgres | |
| # Default is Restrict. | |
| # Controls whether stale PostgreSQL roles are deleted or retained. | |
| # None disables stale-user cleanup outside of finalization for the | |
| # specific PostgresAccess being deleted. | |
| staleUserDeletionPolicy: Restrict | |
| # Configure RabbitMQ-specific controller behavior. | |
| rabbitmq: | |
| # Default is Restrict. | |
| staleUserDeletionPolicy: Restrict | |
| # Default is Retain. | |
| staleVhostDeletionPolicy: Retain | |
| # Configure Redis-specific controller behavior. | |
| redis: | |
| # Default is Restrict. | |
| staleUserDeletionPolicy: Restrict |
Comment on lines
6
to
14
| import ( | ||
| "fmt" | ||
| "os/exec" | ||
| "strings" | ||
| "time" | ||
| "fmt" | ||
| "os/exec" | ||
| "strings" | ||
| "time" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
|
|
||
| "github.com/delta10/access-operator/test/utils" | ||
| utils2 "github.com/delta10/access-operator/test/e2e/utils" | ||
| . "github.com/onsi/gomega" | ||
| ) |
There was a problem hiding this comment.
This import block isn’t gofmt-formatted (spaces instead of tabs/alignment). Since the repo’s Makefile runs go fmt ./..., these files will be rewritten locally/CI; please run gofmt on the updated e2e files before merging to keep diffs stable.
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
reviewed
Apr 14, 2026
bartjkdp
requested changes
Apr 14, 2026
Member
|
Ziet er goed uit. Paar kleine suggesties en een follow-up issue gemaakt in #44. |
bartjkdp
reviewed
Apr 30, 2026
| managerAppNameLabelKey = "app.kubernetes.io/name" | ||
| managerAppNameLabelValue = "access-operator" | ||
|
|
||
| defaultManagerDeploymentNamespace = "system" |
Member
There was a problem hiding this comment.
Suggested change
| defaultManagerDeploymentNamespace = "system" | |
| defaultManagerDeploymentNamespace = "access-operator-system" |
bartjkdp
reviewed
Apr 30, 2026
|
|
||
| It("should parse settings from ConfigMap payload", func() { | ||
| fakeClient := newFakeClientWithScheme( | ||
| newControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| newControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| newControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| }, | ||
| It("should normalize excluded usernames from settings ConfigMap", func() { | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| It("should resolve stale user deletion policy from settings ConfigMap", func() { | ||
| deletePolicy := accessv1.StaleUserDeletionPolicyDelete | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| spec: | ||
| settings: | ||
| name: access-operator-settings | ||
| namespace: access-operator-demo |
Member
There was a problem hiding this comment.
Suggested change
| namespace: access-operator-demo | |
| namespace: access-operator-system |
bartjkdp
reviewed
Apr 30, 2026
| Name: "cluster-settings", | ||
| Namespace: "access-operator-system", | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| ExcludedUsers: []string{"excluded-user"}, | ||
| }, | ||
| }, | ||
| controllerSettings := test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| controllerSettings := test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| controllerSettings := test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| }, | ||
| }, | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| Settings: accessv1.ControllerSettings{ExistingSecretNamespace: true}, | ||
| It("should normalize excluded usernames from settings ConfigMap", func() { | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| It("should resolve stale user deletion policy from settings ConfigMap", func() { | ||
| deletePolicy := accessv1.StaleUserDeletionPolicyDelete | ||
| fakeClient, _ := test.NewFakeClientWithScheme( | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
bartjkdp
reviewed
Apr 30, 2026
| ExcludedUsers: []string{"default"}, | ||
| }, | ||
| }, | ||
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ |
Member
There was a problem hiding this comment.
Suggested change
| test.NewControllerSettingsConfigMap("system", accessv1.ControllerSettings{ | |
| test.NewControllerSettingsConfigMap("access-operator-system", accessv1.ControllerSettings{ |
Member
|
Ziet er goed uit. Suggestie om te standaardiseren op |
bartjkdp
requested changes
Apr 30, 2026
bartjkdp
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.