fix(maas): keep payload-processing in gateway namespace after namespace transform#3545
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughbuildMaasOperatorInstallManifests now calls a new restoreGatewayNamespaceResources(resMap) step immediately after the blanket namespace transform to reset allowlisted payload-processing resources to DefaultGatewayNamespace. An unexported allowlist gatewayNamespaceResourceNames lists resource names to keep in the gateway namespace. The invocation returns a wrapped error if restoration fails. Security note (actionable): validate both resource kind and name when matching to avoid namespace-escape or privilege misassignment (CWE-269, CWE-732). Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
❌ E2E Update Requirement Check FailedNo e2e tests were added/updated as part of this PR. Action required from the PR author: Please either:
For more info, please refer to: workflow run details Why this check exists:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/components/modelsasservice/modelsasservice_support.go`:
- Around line 254-274: The restoreGatewayNamespaceResources function currently
uses a name-only allowlist (gatewayNamespaceResourceNames) and only calls
res.SetNamespace(), which misses ClusterRoleBinding.subjects[].namespace and can
mis-target resources with the same name; change the allowlist to a kind+name
selector (e.g., map[string]map[string]bool or a set of composite keys using
res.GetKind()+"/"+res.GetName()) and in restoreGatewayNamespaceResources check
both kind and name (use res.GetKind() and res.GetName()); when the resource is a
ClusterRoleBinding (res.GetKind() == "ClusterRoleBinding"), parse the object and
explicitly set each subject's namespace to DefaultGatewayNamespace in addition
to calling res.SetNamespace(), so subjects[].namespace is restored for the
payload-processing-reader binding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6135b0d6-27f9-47b0-a3dc-c85be57db6b5
📒 Files selected for processing (1)
internal/controller/components/modelsasservice/modelsasservice_support.go
| var gatewayNamespaceResourceNames = map[string]bool{ | ||
| "payload-processing": true, // Deployment, Service, ServiceAccount | ||
| "payload-processing-plugins": true, // ConfigMap | ||
| "payload-processing-reader": true, // ClusterRoleBinding (subjects namespace) | ||
| } | ||
|
|
||
| // restoreGatewayNamespaceResources moves resources that belong in the gateway | ||
| // namespace back from the application namespace. The blanket NamespaceApplierPlugin | ||
| // moves everything to appNs, but payload-processing resources must stay in the | ||
| // gateway namespace for the EnvoyFilter to attach to the Gateway. | ||
| func restoreGatewayNamespaceResources(resMap resmap.ResMap) error { | ||
| for _, res := range resMap.Resources() { | ||
| if !gatewayNamespaceResourceNames[res.GetName()] { | ||
| continue | ||
| } | ||
| if err := res.SetNamespace(DefaultGatewayNamespace); err != nil { | ||
| return fmt.Errorf("set namespace on %s %s: %w", res.GetKind(), res.GetName(), err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In sigs.k8s.io/kustomize/api, does Resource.SetNamespace update ClusterRoleBinding subjects[].namespace, or only metadata.namespace? Please include source links for the exact version behavior.
💡 Result:
No, Resource.SetNamespace in sigs.k8s.io/kustomize/api only updates metadata.namespace. It sets the namespace tracked in the Resource struct for identification (ResId), but does not traverse or modify nested fields like ClusterRoleBinding subjects[].namespace.[1][2] The namespace transformation for nested fields in ClusterRoleBinding subjects is handled separately by the Namespace Filter (api/filters/namespace.Filter), applied via Resource.ApplyFilter during the NamespaceTransformer.[3][4] This filter specifically targets subjects: - For DefaultSubjectsOnly (default behavior): Updates subjects.namespace if subject name is "default" via setSubjectsNamedDefault, which calls setNamespaceField on the subject.[3] - For AllServiceAccountSubjects: Updates subjects.namespace for all ServiceAccount kind subjects via setServiceAccountNamespaces.[3] Direct calls to SetNamespace (e.g., in CopyMergeMetaDataFieldsFrom or after ApplyFilter) only affect the top-level metadata.namespace.[1] This behavior is consistent across versions, as seen in master branch source (latest as of 2026).[3][1] GitHub issues confirm subjects handling is special-cased and not automatic for non-default namespaces without the SA in the resource set.[5][6][7]
Citations:
- 1: https://github.com/kubernetes-sigs/kustomize/blob/master/api/resource/resource.go
- 2: https://pkg.go.dev/sigs.k8s.io/kustomize/api/resource
- 3: https://github.com/kubernetes-sigs/kustomize/blob/master/api/filters/namespace/namespace.go
- 4: https://github.com/kubernetes-sigs/kustomize/blob/master/plugin/builtin/namespacetransformer/NamespaceTransformer.go
- 5: namespace of ClusterRoleBinding subject only overwritten if default kubernetes-sigs/kustomize#4015
- 6: Control modification of namespace in [Cluster]RoleBinding subjects with annotation kubernetes-sigs/kustomize#4301
- 7: [Question] How to avoid mutating namespace field of default ServiceAccount in RoleBinding.subjects ? kubernetes-sigs/kustomize#3127
🏁 Script executed:
# Examine the call site and full context of restoreGatewayNamespaceResources
cd /tmp && find . -type f -name "modelsasservice_support.go" 2>/dev/null | head -1 | xargs cat -n | sed -n '100,150p'Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check the full function and any post-processing
cd /tmp && find . -type f -name "modelsasservice_support.go" 2>/dev/null | head -1 | xargs cat -n | sed -n '250,300p'Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Look for DefaultGatewayNamespace definition and any additional ClusterRoleBinding handling
cd /tmp && find . -type f -name "modelsasservice_support.go" 2>/dev/null | head -1 | xargs grep -n "DefaultGatewayNamespace\|ClusterRoleBinding" | head -20Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
git ls-files "**/modelsasservice_support.go"Repository: opendatahub-io/opendatahub-operator
Length of output: 153
🏁 Script executed:
# Once located, examine the context around the call and function
fd -type f "modelsasservice_support.go" | head -1 | xargs cat -n | sed -n '100,150p'Repository: opendatahub-io/opendatahub-operator
Length of output: 250
🏁 Script executed:
# Check the function and surrounding context
fd -type f "modelsasservice_support.go" | head -1 | xargs cat -n | sed -n '240,290p'Repository: opendatahub-io/opendatahub-operator
Length of output: 250
🏁 Script executed:
# Search for DefaultGatewayNamespace and any ClusterRoleBinding handling
fd -type f "modelsasservice_support.go" | head -1 | xargs grep -n "DefaultGatewayNamespace\|ClusterRoleBinding\|setSubjectsNamedDefault"Repository: opendatahub-io/opendatahub-operator
Length of output: 252
🏁 Script executed:
cat -n internal/controller/components/modelsasservice/modelsasservice_support.go | sed -n '100,150p'Repository: opendatahub-io/opendatahub-operator
Length of output: 2518
🏁 Script executed:
cat -n internal/controller/components/modelsasservice/modelsasservice_support.go | sed -n '240,290p'Repository: opendatahub-io/opendatahub-operator
Length of output: 2667
🏁 Script executed:
grep -n "DefaultGatewayNamespace\|ClusterRoleBinding\|payload-processing" internal/controller/components/modelsasservice/modelsasservice_support.goRepository: opendatahub-io/opendatahub-operator
Length of output: 813
🏁 Script executed:
# Search for any additional namespace handling or filters after restoreGatewayNamespaceResources
grep -n "ClusterRoleBinding\|payload-processing-reader" internal/controller/components/modelsasservice/modelsasservice_support.go -A 3 -B 3Repository: opendatahub-io/opendatahub-operator
Length of output: 448
🏁 Script executed:
# Check if CreateNamespaceApplierPlugin or any namespace filter handles subjects
rg "CreateNamespaceApplierPlugin|NamespaceTransformer|subjects" internal/controller/components/modelsasservice/ -A 2 -B 2Repository: opendatahub-io/opendatahub-operator
Length of output: 1204
🏁 Script executed:
# Search entire codebase for any post-processing of ClusterRoleBinding subjects in relation to this component
rg "payload-processing-reader|restoreGatewayNamespaceResources" --type go -A 5Repository: opendatahub-io/opendatahub-operator
Length of output: 2399
🏁 Script executed:
# Check the base manifests to understand what namespace payload-processing-reader ServiceAccount is in
find . -type f -path "*maas*" -name "*.yaml" -o -path "*maas*" -name "*.yml" | xargs grep -l "payload-processing-reader" 2>/dev/null | head -5Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Look at the actual kustomization to see if there's a namespace overlay for the gateway resources
find . -type f -path "*maas*" -name "kustomization.yaml" 2>/dev/null | head -3 | xargs catRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if payload-processing ServiceAccount exists in gateway namespace (openshift-ingress) in the manifests
find . -path "*maas*" -name "*.yaml" -o -path "*maas*" -name "*.yml" | xargs grep -l "payload-processing" 2>/dev/null | head -5Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Search for any manifest files or kustomization references related to maas
find . -type f -name "*maas*" | head -20Repository: opendatahub-io/opendatahub-operator
Length of output: 169
🏁 Script executed:
# Search for payload-processing in any files
rg "payload-processing" --type yaml --type yml | head -20Repository: opendatahub-io/opendatahub-operator
Length of output: 107
🏁 Script executed:
# Look for the kPath used in the code - check what manifests directory structure exists
find . -type d -name "*maas*" 2>/dev/nullRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Search for where kPath is defined - look for maas-controller bundle path
rg "maas-controller install bundle\|kPath.*=" internal/controller/components/modelsasservice/ -B 3 -A 3Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Look for the bundle or manifests directory reference
rg "GetKustomizePath\|manifests\|kPath" internal/controller/components/modelsasservice/modelsasservice_support.go -B 2 -A 2Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's a test file that shows what manifests are expected
find . -name "*modelsasservice*test*" -o -name "*test*modelsasservice*" | head -5Repository: opendatahub-io/opendatahub-operator
Length of output: 289
🏁 Script executed:
# Search across entire repo for payload-processing-reader definition
rg "payload-processing-reader" --type yaml | head -10Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check the test file to understand what resources are involved
cat internal/controller/components/modelsasservice/modelsasservice_test.go | head -100Repository: opendatahub-io/opendatahub-operator
Length of output: 3764
🏁 Script executed:
# Search more broadly for bundle or manifest references
rg "Bundle\|Manifest\|openshift-ingress" internal/controller/components/modelsasservice/modelsasservice_support.go | head -15Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check imports to see what resmap.ResMap is and verify the SetNamespace behavior
head -50 internal/controller/components/modelsasservice/modelsasservice_support.goRepository: opendatahub-io/opendatahub-operator
Length of output: 1764
🏁 Script executed:
# Verify SetNamespace behavior in kustomize directly by checking the actual implementation
rg "func.*SetNamespace" --type go | head -10Repository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Check if there's any custom handling for ClusterRoleBinding in the plugins module
rg "ClusterRoleBinding\|subjects" internal/controller/components/modelsasservice/ --type goRepository: opendatahub-io/opendatahub-operator
Length of output: 61
🏁 Script executed:
# Look at the plugins module to see what CreateNamespaceApplierPlugin does
find . -path "*plugins*" -name "*.go" | xargs grep -l "CreateNamespaceApplierPlugin\|NamespaceApplier" | head -3Repository: opendatahub-io/opendatahub-operator
Length of output: 112
🏁 Script executed:
cat ./pkg/plugins/namespacePlugin.goRepository: opendatahub-io/opendatahub-operator
Length of output: 1983
ClusterRoleBinding subjects namespace not restored; breaks RBAC binding across namespaces
The payload-processing-reader ClusterRoleBinding subjects are moved to the application namespace by CreateNamespaceApplierPlugin(appNs) (which updates subjects[].namespace), but restoreGatewayNamespaceResources only calls res.SetNamespace(), which updates only metadata.namespace. This leaves the binding in openshift-ingress with subjects pointing to the application namespace, breaking authorization for the ServiceAccount—RBAC bindings cannot resolve cross-namespace subject references.
Additionally, the name-only allowlist is brittle; any future resource with name payload-processing-reader (different kind) will be unintentionally moved to openshift-ingress.
Use a kind+name selector and explicitly restore ClusterRoleBinding subjects to the gateway namespace:
Suggested fix
-var gatewayNamespaceResourceNames = map[string]bool{
- "payload-processing": true, // Deployment, Service, ServiceAccount
- "payload-processing-plugins": true, // ConfigMap
- "payload-processing-reader": true, // ClusterRoleBinding (subjects namespace)
-}
+type resourceKey struct {
+ kind string
+ name string
+}
+
+var gatewayNamespaceResourceKeys = map[resourceKey]bool{
+ {kind: "Deployment", name: "payload-processing"}: true,
+ {kind: "Service", name: "payload-processing"}: true,
+ {kind: "ServiceAccount", name: "payload-processing"}: true,
+ {kind: "ConfigMap", name: "payload-processing-plugins"}: true,
+ {kind: "ClusterRoleBinding", name: "payload-processing-reader"}: true,
+}
func restoreGatewayNamespaceResources(resMap resmap.ResMap) error {
for _, res := range resMap.Resources() {
- if !gatewayNamespaceResourceNames[res.GetName()] {
+ k := resourceKey{kind: res.GetKind(), name: res.GetName()}
+ if !gatewayNamespaceResourceKeys[k] {
continue
}
+
+ // For ClusterRoleBinding, explicitly restore subject namespace
+ if res.GetKind() == "ClusterRoleBinding" {
+ if err := restoreClusterRoleBindingSubjects(res, DefaultGatewayNamespace); err != nil {
+ return fmt.Errorf("restore subjects namespace on ClusterRoleBinding %s: %w", res.GetName(), err)
+ }
+ }
+
if err := res.SetNamespace(DefaultGatewayNamespace); err != nil {
return fmt.Errorf("set namespace on %s %s: %w", res.GetKind(), res.GetName(), err)
}
}
return nil
+}
+
+func restoreClusterRoleBindingSubjects(res *resource.Resource, namespace string) error {
+ obj, err := res.Map()
+ if err != nil {
+ return err
+ }
+
+ subjects, ok := obj["subjects"].([]interface{})
+ if !ok {
+ return nil
+ }
+
+ for _, s := range subjects {
+ if subject, ok := s.(map[string]interface{}); ok {
+ subject["namespace"] = namespace
+ }
+ }
+ return res.SetMap(obj)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/components/modelsasservice/modelsasservice_support.go`
around lines 254 - 274, The restoreGatewayNamespaceResources function currently
uses a name-only allowlist (gatewayNamespaceResourceNames) and only calls
res.SetNamespace(), which misses ClusterRoleBinding.subjects[].namespace and can
mis-target resources with the same name; change the allowlist to a kind+name
selector (e.g., map[string]map[string]bool or a set of composite keys using
res.GetKind()+"/"+res.GetName()) and in restoreGatewayNamespaceResources check
both kind and name (use res.GetKind() and res.GetName()); when the resource is a
ClusterRoleBinding (res.GetKind() == "ClusterRoleBinding"), parse the object and
explicitly set each subject's namespace to DefaultGatewayNamespace in addition
to calling res.SetNamespace(), so subjects[].namespace is restored for the
payload-processing-reader binding.
✅ Addressed in commit bf0e90d
There was a problem hiding this comment.
As mentioned in the other comment, I'm not convinced that these issues have actually been resolved?
There was a problem hiding this comment.
🧩 Analysis chain
Let me inspect the commit to verify what was actually changed.
🏁 Script executed:
#!/bin/bash
# Fetch the diff of commit bf0e90d to verify what was changed
gh api repos/opendatahub-io/opendatahub-operator/commits/bf0e90d --jq '.commit.message'
echo "---"
gh api repos/opendatahub-io/opendatahub-operator/commits/bf0e90d/files --jq '.[].filename'
echo "---"
# Get the patch for the relevant file
gh api repos/opendatahub-io/opendatahub-operator/commits/bf0e90d/files --jq '.[] | select(.filename | contains("modelsasservice_support")) | .patch'
Length of output: 1041
🏁 Script executed:
#!/bin/bash
# Get the current state of the file on the PR branch
gh api repos/opendatahub-io/opendatahub-operator/pulls/3545 --jq '.head.sha'
echo "---"
# List commits on the PR
gh api repos/opendatahub-io/opendatahub-operator/pulls/3545/commits --jq '.[].sha + " " + .[].commit.message[0:60]'
Length of output: 147
@grdryn — you're right to question this. The "✅ Addressed" tag was incorrectly applied; bf0e90d is just the original PR commit (c359481), not a subsequent fix. The PR has only one commit and the file content hasn't changed from what was reviewed.
Both issues remain open in the current code:
1. ClusterRoleBinding subjects[].namespace is not restored (still broken)
pkg/plugins/namespacePlugin.go (confirmed by the code) sets SetRoleBindingSubjects: namespace.AllServiceAccountSubjects, which means CreateNamespaceApplierPlugin(appNs) explicitly rewrites every ClusterRoleBinding's subjects[].namespace to appNs. The subsequent res.SetNamespace(DefaultGatewayNamespace) call only sets metadata.namespace — it does not touch subjects[].namespace. After reconcile, payload-processing-reader will have:
metadata.namespace = openshift-ingress✅subjects[0].namespace = redhat-ods-applications❌
The ServiceAccount lookup will fail, breaking RBAC for the ext_proc sidecar.
2. Name-only allowlist is still kind-blind
gatewayNamespaceResourceNames still keys only on resource name. Any future resource named payload-processing or payload-processing-reader (e.g., a different Kind) will be silently relocated to openshift-ingress.
The fix from the original comment stands — needs a kind+name composite key and explicit subjects[].namespace restoration for the ClusterRoleBinding. Here's a concrete, minimal fix using the existing unstructured helpers already imported:
type resourceKey struct {
kind string
name string
}
var gatewayNamespaceResourceKeys = map[resourceKey]bool{
{kind: "Deployment", name: "payload-processing"}: true,
{kind: "Service", name: "payload-processing"}: true,
{kind: "ServiceAccount", name: "payload-processing"}: true,
{kind: "ConfigMap", name: "payload-processing-plugins"}: true,
{kind: "ClusterRoleBinding",name: "payload-processing-reader"}: true,
}
func restoreGatewayNamespaceResources(resMap resmap.ResMap) error {
for _, res := range resMap.Resources() {
k := resourceKey{kind: res.GetKind(), name: res.GetName()}
if !gatewayNamespaceResourceKeys[k] {
continue
}
if res.GetKind() == "ClusterRoleBinding" {
obj, err := res.Map()
if err != nil {
return fmt.Errorf("map ClusterRoleBinding %s: %w", res.GetName(), err)
}
if subjects, ok := obj["subjects"].([]interface{}); ok {
for _, s := range subjects {
if subject, ok := s.(map[string]interface{}); ok {
subject["namespace"] = DefaultGatewayNamespace
}
}
}
if err := res.SetMap(obj); err != nil {
return fmt.Errorf("set map on ClusterRoleBinding %s: %w", res.GetName(), err)
}
}
if err := res.SetNamespace(DefaultGatewayNamespace); err != nil {
return fmt.Errorf("set namespace on %s %s: %w", res.GetKind(), res.GetName(), err)
}
}
return nil
}🧬 Code Graph Analysis Results
- File: internal/controller/status/status.go
- Lines: 82-82
ConditionDeploymentsAvailable = "DeploymentsAvailable"
- File: internal/controller/status/status.go
- Lines: 119-119
ReadySuffix = "Ready"
- File: api/components/v1alpha1/modelsasservice_types.go
- Lines: 27-27
ModelsAsServiceComponentName = "modelsasservice"
- File: api/components/v1alpha1/modelsasservice_types.go
- Lines: 30-30
ModelsAsServiceKind = "ModelsAsService"
- File: pkg/cluster/resources.go
- Lines: 142-151
func ApplicationNamespace(ctx context.Context, cli client.Client) (string, error) {
if ns := GetRHAIApplicationsNamespace(); ns != "" {
return ns, nil
}
dsci, err := GetDSCI(ctx, cli)
if err != nil {
return "", fmt.Errorf("failed to get DSCInitialization: %w", err)
}
return dsci.Spec.ApplicationsNamespace, nil
}
- File: pkg/metadata/labels/types.go
- Lines: 42-50
ODH = struct {
OwnedNamespace string
Component func(string) string
}{
OwnedNamespace: "opendatahub.io/generated-namespace",
Component: func(name string) string {
return ODHAppPrefix + "/" + name
},
}
- File: pkg/metadata/labels/types.go
- Lines: 16-16
True = "true"
- File: pkg/metadata/labels/types.go
- Lines: 24-30
K8SCommon = struct {
PartOf string
Component string
}{
PartOf: "app.kubernetes.io/part-of",
Component: "app.kubernetes.io/component",
}
- File: pkg/plugins/addLabelsplugin.go
- Lines: 25-46
func CreateSetLabelsPlugin(labels map[string]string) *builtins.LabelTransformerPlugin {
return &builtins.LabelTransformerPlugin{
Labels: labels,
FieldSpecs: []types.FieldSpec{
{
Gvk: resid.Gvk{Kind: "Deployment"},
Path: "spec/template/metadata/labels",
CreateIfNotPresent: true,
},
{
Gvk: resid.Gvk{Kind: "Deployment"},
Path: "spec/selector/matchLabels",
CreateIfNotPresent: true,
},
{
Gvk: resid.Gvk{},
Path: "metadata/labels",
CreateIfNotPresent: true,
},
},
}
}
- File: pkg/controller/types/types.go
- Lines: 63-67
ManifestInfo struct {
Path string
ContextDir string
SourcePath string
}
- File: pkg/plugins/namespacePlugin.go
- Lines: 11-67
func CreateNamespaceApplierPlugin(targetNamespace string) *builtins.NamespaceTransformerPlugin {
return &builtins.NamespaceTransformerPlugin{
ObjectMeta: types.ObjectMeta{
Name: "odh-namespace-plugin",
Namespace: targetNamespace,
},
FieldSpecs: []types.FieldSpec{
{
Gvk: resid.Gvk{},
Path: "metadata/namespace",
CreateIfNotPresent: true,
},
{
Gvk: resid.Gvk{
Group: "rbac.authorization.k8s.io",
Kind: "ClusterRoleBinding",
},
Path: "subjects/namespace",
CreateIfNotPresent: true,
},
{
Gvk: resid.Gvk{
Group: "rbac.authorization.k8s.io",
Kind: "RoleBinding",
},
Path: "subjects/namespace",
CreateIfNotPresent: true,
},
{
Gvk: resid.Gvk{
Group: "admissionregistration.k8s.io",
Kind: "ValidatingWebhookConfiguration",
},
Path: "webhooks/clientConfig/service/namespace",
CreateIfNotPresent: false,
},
{
Gvk: resid.Gvk{
Group: "admissionregistration.k8s.io",
Kind: "MutatingWebhookConfiguration",
},
Path: "webhooks/clientConfig/service/namespace",
CreateIfNotPresent: false,
},
{
Gvk: resid.Gvk{
Group: "apiextensions.k8s.io",
Kind: "CustomResourceDefinition",
},
Path: "spec/conversion/webhook/clientConfig/service/namespace",
CreateIfNotPresent: false,
},
},
UnsetOnly: false,
SetRoleBindingSubjects: namespace.AllServiceAccountSubjects,
}
}
There was a problem hiding this comment.
@liangwen12year the second issue here about the allowlist being based on names only also seems legitimate, especially now that you've removed the related CRB from the list: anything of any type added to the manifests in future with the name "payload-processing" or "payload-processing-plugins" will get created in the gateway namespace implicitly, even if that is not intentional. The fact that you've moved the "payload-processing-reader" back to the applications namespace makes me think that other payload-processing related things could potentially be expected in that namespace in future, rather than in the gateway namespace.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
2 times, most recently
from
0e0c64d to
bf0e90d
Compare
|
/retest |
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
2 times, most recently
from
fe665e6 to
6c74642
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3545 +/- ##
==========================================
+ Coverage 55.61% 55.63% +0.01%
==========================================
Files 209 209
Lines 15352 15381 +29
==========================================
+ Hits 8538 8557 +19
- Misses 6006 6012 +6
- Partials 808 812 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
from
6c74642 to
c359481
Compare
| var gatewayNamespaceResourceNames = map[string]bool{ | ||
| "payload-processing": true, // Deployment, Service, ServiceAccount | ||
| "payload-processing-plugins": true, // ConfigMap | ||
| "payload-processing-reader": true, // ClusterRoleBinding (subjects namespace) |
There was a problem hiding this comment.
It looks like the namespace applier plugin will have changed the CRB's subject's namespace also, but the new restoreGatewayNamespaceResources will only change the metadata.namespace (and not subjects[].namespace)
| var gatewayNamespaceResourceNames = map[string]bool{ | ||
| "payload-processing": true, // Deployment, Service, ServiceAccount | ||
| "payload-processing-plugins": true, // ConfigMap | ||
| "payload-processing-reader": true, // ClusterRoleBinding (subjects namespace) | ||
| } | ||
|
|
||
| // restoreGatewayNamespaceResources moves resources that belong in the gateway | ||
| // namespace back from the application namespace. The blanket NamespaceApplierPlugin | ||
| // moves everything to appNs, but payload-processing resources must stay in the | ||
| // gateway namespace for the EnvoyFilter to attach to the Gateway. | ||
| func restoreGatewayNamespaceResources(resMap resmap.ResMap) error { | ||
| for _, res := range resMap.Resources() { | ||
| if !gatewayNamespaceResourceNames[res.GetName()] { | ||
| continue | ||
| } | ||
| if err := res.SetNamespace(DefaultGatewayNamespace); err != nil { | ||
| return fmt.Errorf("set namespace on %s %s: %w", res.GetKind(), res.GetName(), err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
As mentioned in the other comment, I'm not convinced that these issues have actually been resolved?
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
2 times, most recently
from
0c67cf9 to
a3159b1
Compare
grdryn
left a comment
grdryn
left a comment
There was a problem hiding this comment.
I think that removing the payload-processing-reader CRB from the gatewayNamespaceResourceNames map is not a complete fix to the issue raised by coderabbit: the namespace applier plugin will have set both the metadata.namespace and subjects[].namespace to the applications namespace, and now restoreGatewayNamespaceResources is changing the namespace of the payload-processing ServiceAccount (which is the subject in the CRB), so now I think there's a disconnect between where the SA is and where the CRB is declaring its subject.
| var gatewayNamespaceResourceNames = map[string]bool{ | ||
| "payload-processing": true, // Deployment, Service, ServiceAccount | ||
| "payload-processing-plugins": true, // ConfigMap | ||
| "payload-processing-reader": true, // ClusterRoleBinding (subjects namespace) | ||
| } | ||
|
|
||
| // restoreGatewayNamespaceResources moves resources that belong in the gateway | ||
| // namespace back from the application namespace. The blanket NamespaceApplierPlugin | ||
| // moves everything to appNs, but payload-processing resources must stay in the | ||
| // gateway namespace for the EnvoyFilter to attach to the Gateway. | ||
| func restoreGatewayNamespaceResources(resMap resmap.ResMap) error { | ||
| for _, res := range resMap.Resources() { | ||
| if !gatewayNamespaceResourceNames[res.GetName()] { | ||
| continue | ||
| } | ||
| if err := res.SetNamespace(DefaultGatewayNamespace); err != nil { | ||
| return fmt.Errorf("set namespace on %s %s: %w", res.GetKind(), res.GetName(), err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
@liangwen12year the second issue here about the allowlist being based on names only also seems legitimate, especially now that you've removed the related CRB from the list: anything of any type added to the manifests in future with the name "payload-processing" or "payload-processing-plugins" will get created in the gateway namespace implicitly, even if that is not intentional. The fact that you've moved the "payload-processing-reader" back to the applications namespace makes me think that other payload-processing related things could potentially be expected in that namespace in future, rather than in the gateway namespace.
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
4 times, most recently
from
16de6f9 to
d770452
Compare
grdryn
left a comment
grdryn
left a comment
There was a problem hiding this comment.
Linter failures indicate that this PR is currently not ready for review. Please re-request a review when this is ready. Some sort of tests would be a good addition also.
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
from
d770452 to
bb269b1
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
3 times, most recently
from
9e1f460 to
539d4e8
Compare
…ce transform The NamespaceApplierPlugin moves ALL resources to the app namespace, but payload-processing resources (Deployment, Service, EnvoyFilter, DestinationRule) must remain in the gateway namespace (openshift-ingress) because the EnvoyFilter must run in the same namespace as the Gateway. After the blanket namespace transform, restore payload-processing resources back to the gateway namespace. This ensures the EnvoyFilter cluster_name FQDN matches the actual service location. Ref: RHOAIENG-59726 Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Remove payload-processing-reader from gatewayNamespaceResourceNames. ClusterRoleBindings are cluster-scoped so SetNamespace() does not apply, and the NamespaceApplierPlugin changes subjects[].namespace which this function does not restore. The CRB is also not part of the maas-controller install bundle (deployment/base/maas-controller/default), so it would never be encountered by this code path. Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Switch from name-only string matching to kind+name struct keys so the allowlist cannot accidentally match unrelated resources that share a name. Also restore payload-processing-reader ClusterRoleBinding to the allowlist with subjects[].namespace restoration — the NamespaceApplierPlugin rewrites subjects, not just metadata.namespace, so the CRB must be fixed separately from namespaced resources. Signed-off-by: Wen Liang <wenliang@redhat.com>
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Verify restoreGatewayNamespaceResources and restoreCRBSubjectsNamespace: - Payload-processing resources are moved to gateway namespace - ClusterRoleBinding subjects[].namespace is restored - Unrelated resources remain in app namespace - Kind+name matching prevents false positives Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
liangwen12year
force-pushed
the
fix/payload-processing-namespace
branch
from
539d4e8 to
9e32609
Compare
2 participants
Summary
restoreGatewayNamespaceResources()to move payload-processing resources back to the gateway namespace (openshift-ingress) after the blanketNamespaceApplierPlugintransformProblem
The operator's
NamespaceApplierPluginmoves ALL resources in the maas-controller install bundle to the application namespace (e.g.redhat-ods-applications). This incorrectly moves payload-processing resources out ofopenshift-ingress, causing the EnvoyFiltercluster_nameFQDN (payload-processing.openshift-ingress.svc.cluster.local) to not match the actual service location, breaking the ext_proc filter chain.Fix
After the blanket namespace transform, restore payload-processing resources (
payload-processing,payload-processing-plugins,payload-processing-reader) back toDefaultGatewayNamespace(openshift-ingress).Supersedes #3487 which was auto-generated and stale.
Ref: RHOAIENG-59726
Test plan
openshift-ingressafter operator reconcile🤖 Generated with Claude Code
Summary by CodeRabbit