feat(namespacesync): add NamespaceSyncController for per-namespace resource management - #3581
Conversation
tekton-robot
added
release-note
tekton-robot
added
the
size/XXL
c1be9cb to
c2fc182
Compare
jkhelil
changed the title
c2fc182 to
481eaa9
Compare
tekton-robot
removed
the
do-not-merge/work-in-progress
a66cbfa to
ba1e9f7
Compare
tekton-robot
added
the
needs-rebase
|
@jkhelil - If you are still planning to pursue this PR, please rebase. |
|
@waveywaves @pratap0007 Would you mind review this please ? |
|
/assign @waveywaves |
ba1e9f7 to
903059b
Compare
tekton-robot
removed
the
needs-rebase
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3581 +/- ##
==========================================
+ Coverage 27.26% 27.64% +0.38%
==========================================
Files 475 477 +2
Lines 25428 25458 +30
==========================================
+ Hits 6932 7039 +107
+ Misses 17745 17696 -49
+ Partials 751 723 -28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return err | ||
| } | ||
|
|
||
| cfg := tc.Spec.Platforms.OpenShift.NamespaceSync |
There was a problem hiding this comment.
Older TektonConfig objects will not have this field, so namespace setup may stop after upgrade. Could we handle existing configurations and add an upgrade test?
| if cfg.NamespaceSelector == nil { | ||
| return true | ||
| } | ||
| sel, err := metav1.LabelSelectorAsSelector(cfg.NamespaceSelector) |
There was a problem hiding this comment.
{} matches every namespace, not none, and invalid selectors also match every namespace. Could we fix and test both cases?
| }) | ||
|
|
||
| // Namespace Add/Update → reconcile that namespace. | ||
| if _, err := nsInf.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ |
There was a problem hiding this comment.
Deleting a namespace leaves its pipeline entry in the cluster-wide permission binding. Could we remove it on deletion?
| // To avoid a thundering-herd on clusters that don't use secret bindings, | ||
| // we only enqueue when NamespaceSync has SecretBindings configured AND the | ||
| // secret name/labels match at least one binding rule. | ||
| if _, err := secretInf.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ |
There was a problem hiding this comment.
Secret label changes are not watched. Could we handle updates so matching Secrets are added and removed?
| } | ||
| } | ||
|
|
||
| if len(cfg.SecretBindings) > 0 { |
There was a problem hiding this comment.
Changing or removing secretBindings leaves old Secret references. Could we remove references added by this controller?
| return nil | ||
| } | ||
|
|
||
| return retry.RetryOnConflict(retry.DefaultRetry, func() error { |
There was a problem hiding this comment.
Each retry uses the same outdated ServiceAccount, so it cannot resolve a conflict. Could we fetch it again on each retry?
Address review feedback on PR tektoncd#3581 (RFE-7814 namespace sync): - Apply TektonConfig defaulting in-memory on every reconcile so pre-existing CRs created before this feature keep syncing without requiring a spec edit. - Correct RoleBinding/OwnerReference drift instead of silently ignoring it, and always run secret binding cleanup even when secretBindings is empty. - Track secrets this controller has bound via an annotation on the pipeline SA so stale references are removed when a binding rule (or the whole secretBindings list) is changed or removed, for both named and labelSelector bindings. - Strip Secret payloads from the informer cache and watch for label changes that add/remove a Secret from a labelSelector binding. - Treat an explicit empty namespaceSelector as "match nothing" and fail malformed selectors closed. - Simplify ensureSecretBindings into smaller, single-purpose helpers. Add e2e coverage for the two scenarios PR tektoncd#3581 flagged as missing before merge: named/labelSelector secret binding lifecycle, and CRB subject cleanup on namespace deletion. Adds WaitForServiceAccountImagePullSecret and WaitForClusterRoleBindingSubject helpers to test/resources/rbac.go. Also fixes two test fixture gaps found while re-running the suite: a missing "edit" ClusterRole fixture, and a namespace-deletion test that never deleted the pipeline SA it was supposed to simulate away. Signed-off-by: Jawed khelil <jkhelil@redhat.com> Assisted-by: Claude Sonnet 5 (via Cursor)
903059b to
ca9814b
Compare
ca9814b to
7672846
Compare
…amespaceSync migration SetDefaults() already migrates createRbacResource/createCABundleConfigMaps/ legacyPipelineRbac in-memory on every reconcile, but tektoncd/operator#3581 also added a one-time pre-upgrade job that persists this migration onto the stored TektonConfig CR. Add a spec that forces that job to re-run (by resetting the pre-upgrade-version status annotation, as if the operator had just been upgraded) and asserts the legacy params are actually removed from spec.params rather than just recomputed in memory.
NamespaceSyncController E2E Test ProofResult: 15/15 specs PASSED, 0 failed, 0 errors.
Cluster under testSee
Per-spec results
Artifacts in this folder
How this was runPATH="/tmp/faux-bin:$PATH" go run github.com/onsi/ginkgo/v2/ginkgo -v \
--focus "NamespaceSyncController" \
--junit-report=nssync-junit.xml \
--json-report=nssync-report.json \
./tests/operator/...( Note: an earlier run against this same cluster caught a real test-code bug (two legacy-params specs issued the |
tekton-robot
added
the
needs-rebase
| spec: | ||
| platforms: | ||
| openshift: | ||
| namespaceSync: |
There was a problem hiding this comment.
@jkhelil Any specific reason why we allow user's to make these choices for all these reconciliations? In legacy batch RBAC mode users/admins did not have the option to manually intervene this behavior. If we still consider manual intervention is required, could you consider addressing the following concerns:
- namespaceSelector misconfig fails silently,
A typo or malformed selector turns provisioning OFF everywhere. When the selector can't be parsed, namespaceMatchesSelector returns false (match nothing). So a small YAML mistake means every new namespace silently gets no pipeline SA, no SCC binding, no CA bundles — and pipelines there fail with permission errors. TektonConfig still shows Ready, so there's no hint anything is wrong.
- {} means the opposite of standard Kubernetes.
namespaceSelector: {} means "match nothing" here, but in every other Kubernetes API an empty selector means "match everything." Someone will almost certainly write {} expecting "all namespaces" and get the exact opposite.
The current design hardens only one direction, "a typo can't accidentally widen sync to the whole cluster" but leaves the more likely mistake (accidentally turning everything off) both easy to hit and invisible.
There was a problem hiding this comment.
thanks @anithapriyanatarajan
1.and 2 fixed
the reason for adding this is to avoid watching selected namespaces, eventually all, if the user dont want to opt in rbac provisioning through operator
df1befe to
c6c30c9
Compare
tekton-robot
removed
the
needs-rebase
| imagePullSecrets := sa.ImagePullSecrets | ||
| secretRefs := sa.Secrets | ||
|
|
||
| return retry.RetryOnConflict(retry.DefaultRetry, func() error { |
There was a problem hiding this comment.
The retry re-fetches the SA but then writes back the imagePullSecrets and secretRefs lists that were computed before the closure. So when the first update hits a 409, the retry overwrites whatever the other writer changed.
This happens on OpenShift as soon as the pipeline SA is created. The registry controller adds pipeline-dockercfg-* to the SA at the same time we update it. If we lose that race, the retry removes the pull secret. A user running oc secrets link at the same moment loses their secret the same way.
I reproduced this with a test. A reactor appends pipeline-dockercfg-abcde to the SA on the first update and returns a conflict. After Reconcile, the SA only has quay-robot.
I also tested a fix. Move the Get and the stale/bind merge inside the RetryOnConflict func, and remove the two captured lists. resolveSecretBindingTargets can stay outside. With this change the repro passes and the existing package tests still pass. Please add the conflict case to the tests so this does not regress.
| return o.PipelinesAsCode != nil || o.SCC != nil || o.NamespaceSync != nil | ||
| } | ||
|
|
||
| func (ns *NamespaceSyncConfig) validate(path string) *apis.FieldError { |
There was a problem hiding this comment.
A malformed namespaceSelector (invalid operator, bad label key, or In with empty values) passes CRD schema validation and gets stored, because NamespaceSyncConfig.validate() only checks secretBindings — it never validates the selector. At reconcile time LabelSelectorAsSelector returns an error and we treat that as "match nothing," so every namespace is silently skipped (no pipeline SA, SCC, or CA bundles) while TektonConfig still reports Ready.
Could we validate namespaceSelector (and secretBindings[].labelSelector) in validate() and reject on parse error, so oc apply fails fast instead of quietly disabling sync everywhere?
tekton-robot
added
the
needs-rebase
c6c30c9 to
8896212
Compare
tekton-robot
removed
the
needs-rebase
8896212 to
08fe33b
Compare
Replace the O(N) scan-based per-namespace reconciliation in rbac.go with a watch-based NamespaceSyncController that reacts to namespace, ServiceAccount, Secret, and TektonConfig events instead of re-scanning every namespace on each TektonConfig reconcile. New typed API under TektonConfig.spec.platforms.openShift.namespaceSync: - createPipelineSA: manage the pipeline SA (replaces createRbacResource) - createCABundles: manage CA bundle ConfigMaps (replaces createCABundleConfigMaps) - createEditRoleBinding: manage openshift-pipelines-edit RoleBinding (replaces legacyPipelineRbac) - createSCCRoleBinding: manage pipelines-scc-rolebinding (default true) - secretBindings: bind registry secrets (by name or label selector) to the pipeline SA - namespaceSelector: restrict sync to namespaces matching labels Signed-off-by: Jawed khelil <jkhelil@redhat.com> Assisted-by: Claude Sonnet 4.6 (via Cursor) Assisted-by: Claude Sonnet 5 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
SetDefaults already migrates the deprecated createRbacResource, createCABundleConfigMaps, and legacyPipelineRbac spec.params entries onto the typed spec.platforms.openshift.namespaceSync fields, but only in-memory on a deep copy on every reconcile. It never writes the result back, so upgraded CRs kept carrying the legacy params forward indefinitely with no way to tell they were deprecated. Add migrateLegacyNamespaceSyncParams, a pre-upgrade job (following the existing removeHubFromTektonConfig/removeDeprecatedDisableAffinity Assistant pattern) that persists the same migration once per upgrade, so the stored CR ends up on the typed schema and the legacy params are actually removed from spec.params. Signed-off-by: Jawed khelil <jkhelil@redhat.com> Assisted-by: Claude Sonnet 5 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
08fe33b to
c58c9e9
Compare
…with Kubernetes
An empty namespaceSelector ({}) previously matched nothing, which is the
opposite of standard Kubernetes label-selector behaviour where {} means
'match everything'. This would silently surprise any operator who writes
namespaceSelector: {} expecting all namespaces to be synced.
Remove the special-case early-return so that {} and nil both delegate to
metav1.LabelSelectorAsSelector, which correctly returns labels.Everything()
for an empty selector. The 'disable sync entirely' pattern is now achieved
by setting individual feature flags to false rather than abusing an empty
selector.
Updates:
- reconciler.go: remove empty-selector special case in namespaceMatchesSelector
- openshift_platform.go: correct API field comment
- reconciler_test.go: rename/invert empty-selector test case
- docs/TektonConfig.md: remove misleading namespaceSelector: {} example
Signed-off-by: Jawed khelil <jkhelil@redhat.com>
c58c9e9 to
6fcda85
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anithapriyanatarajan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
6 participants
Changes
Introduces a new
NamespaceSyncControllerthat replaces thescan-based batch processing in
rbac.gowith an event-driven,per-namespace reconciler. Addresses RFE-7814 (flexible service
account binding for Quay Robot credentials).
What this does
NamespaceSyncController— a watch-based controller thatreacts to Namespace, ServiceAccount, Secret, and TektonConfig
events to ensure per-namespace Tekton resources are present and
up to date in every user namespace
pipelineSA, SCC RoleBinding, edit RoleBinding, andCA bundle ConfigMaps in each namespace
openshift-pipelines-clusterinterceptorsCRBincrementally (add/remove only the affected namespace's subject)
spec.platforms.openshift .namespaceSync.secretBindings(the core RFE-7814 use case)spec.platforms.openshift.namespaceSyncAPI field toTektonConfig, replacing legacy
spec.paramsentriesrbac.goLeaderAwareFuncsembedding (required by Knative sharedmain)namespaceSyncschema to the TektonConfig CRDStatus
Work in progress — more e2e tests needed before merge:
make test lintfull passVerified on cluster
All per-namespace resources (pipeline SA, SCC RoleBinding, edit
RoleBinding, CA bundles, CRB subject) created correctly in new
namespaces within seconds. Established namespaces unaffected.
Submitter Checklist
make test lintbefore submitting a PRRelease Notes
Made with Cursor