Repository navigation
Restrict secrets to allowed namespace (manual backport 273) - #292
Merged
Merged
Conversation
Prep for a --pull-secret-namespaces flag. The operator namespace is always included so that setting the flag only adds namespaces, and the parser rejects invalid names up front: a namespaced Secret informer for one could never sync, which would leave the controller stuck at startup with an unclear error. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 8555d31)
Anyone who can create or edit a BootcNodePool could point pullSecretRef at a Secret in any namespace and have the operator use those credentials and ship them to managed nodes. Pools may now only reference pull secrets in the namespaces given by the new --pull-secret-namespaces flag, plus the operator namespace, which is always allowed. Other pools become Degraded with reason SecretError and a message naming the namespace and the allowed ones, and no nodes are created or updated for them. This is a behavior change for pools that use a secret outside the operator namespace: after upgrading they must move the secret or have its namespace added to the flag. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit c505530)
Watching Secrets made the informer cache every Secret in the cluster; secretTypePredicate only filters which events trigger a reconcile, not what is stored. Now that pools can only use secrets from the allowed namespaces, there is no reason to cache anything else. This is also what lets the controller drop its cluster-wide Secret RBAC: the informer no longer lists or watches Secrets outside the allowlist. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 023ab48)
The controller's ClusterRole let it read every Secret in the cluster, broader than ARCHITECTURE.md describes. With the Secret cache now limited to the allowed namespaces, cluster-wide access is no longer needed. Secret read access moves from the ClusterRole to a Role in the operator namespace, bound to the controller ServiceAccount, in both the Kustomize manifests and the generated Helm chart. Extra namespaces need their own Role, which the Helm chart grows next. controller-gen now emits role.yaml as two documents, a ClusterRole and a Role. sync-helm.sh read .rules from the whole file, which would merge them and put the secrets rule back into the Helm ClusterRole, so it now selects the document by kind. It also templates the namespace of the new RoleBinding, whose source carries the Kustomize "system" placeholder. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 1e57d3e)
Lets pools use pull secrets from namespaces other than the release
namespace, e.g. ones owned by other teams, set at install or upgrade
time:
helm upgrade ... --set 'pullSecretNamespaces={team-a,team-b}'
The one value drives both the controller's --pull-secret-namespaces flag
(always including the release namespace) and a Role and RoleBinding in
each listed namespace, so the allowlist and the permissions cannot drift.
The namespaces must already exist; the chart does not create them. A
values schema rejects anything that is not a list of valid namespace
names, as the controller would otherwise fail to start.
Assisted-by: AI
Signed-off-by: Alice Frosi <afrosi@redhat.com>
(cherry picked from commit 22d6ff6)
The chart's RBAC and controller args are generated or templated from several sources, and nothing checked the rendered result, so a mistake in sync-helm.sh or the templates would only show up on a real install. make helm-test renders the chart with and without extra pullSecretNamespaces and checks the controller flag, that each namespace gets one secrets Role bound to the controller, that the controller ClusterRole has no secrets access, and that the values schema rejects bad input. helm is installed with go install like the other tools. The unit CI job runs the target. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 569829a)
TestOperatorUpgrade uninstalls the operator and reinstalls it with Helm, passing back the image and controller.extraArgs but no other values. On a cluster deployed with pullSecretNamespaces, the extra namespaces were dropped, so pools using secrets there broke for the rest of the run. It now reads the allowed namespaces from the controller's --pull-secret-namespaces flag before uninstalling and sets them again whenever it moves to the current chart. The released chart does not know the value, so the install of the released version does not pass it. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 6c1b6e5)
deploy-bink now creates an extra namespace and adds it to the release's pullSecretNamespaces, and passes its name to the tests. Configuring it at deploy time, rather than from the test, avoids restarting the controller during the run, so the new test can run in parallel. In TestPullSecretNamespaces, a pool referencing a secret in a namespace the test creates must be Degraded with SecretError and claim no node; switched to a secret in the allowed namespace, its node must update through the authenticated registry. The test is skipped when no namespace is specified, but fails if the operator does not allow a specified namespace. Assisted-by: AI Signed-off-by: Alice Frosi <afrosi@redhat.com> (cherry picked from commit 10ccc8e)
alicefr
changed the title
HarshwardhanPatil07
left a comment
HarshwardhanPatil07
left a comment
Member
There was a problem hiding this comment.
I think we would need to increase CI timeout here, wdyt?
Collaborator
Author
The new e2e test is parallel so it shouldn't have a big impact. The problem is that annoying test for the |
Collaborator
Author
|
We should quarantine that test until we confirm that the bug has been solved |
HarshwardhanPatil07
approved these changes
Nov 5, 2026
alicefr
enabled auto-merge (rebase)
November 5, 2026 10:53
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.
Manual backport #273