Adding additional roles and network policy configs#588
Adding additional roles and network policy configs#588chamalabey wants to merge 6 commits intoopenshift:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chamalabey 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 |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a NetworkPolicy, three Roles and three RoleBindings, inserts a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
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 Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy_pko/NetworkPolicy-muo.yaml`:
- Around line 10-15: The NetworkPolicy currently allows all pods in the target
namespace (podSelector: {}) to be contacted from any pod in the
openshift-managed-upgrade-operator namespace; tighten it by replacing the empty
top-level podSelector with a specific label selector that matches only the
intended target pods (e.g., app: my-app), add a podSelector under the
ingress->from block (next to namespaceSelector) to restrict source pods in the
openshift-managed-upgrade-operator namespace to only those with the required
labels, and add an explicit ports array under ingress to enumerate only the
required ports/protocols instead of allowing all ports; update the YAML fields
ingress, podSelector, namespaceSelector, and ports accordingly.
In `@deploy_pko/Role-muo-other.yaml`:
- Around line 45-52: The Role currently grants broad access to all secrets via
apiGroups: "" and resources: secrets with verbs get, list, watch; restrict the
muo-pullsecret-reader role to only the named secret by adding resourceNames:
[pull-secret] under the secrets resource and remove unused verbs so verbs
contains only get (no list or watch) to match util/access_token.go which only
reads the pull-secret; update the Role definition in Role-muo-other.yaml
accordingly.
- Around line 29-34: The Role currently allows create on serviceaccounts/token
for any ServiceAccount; restrict token minting by adding a resourceNames stanza
listing only the ServiceAccount(s) that should be allowed (e.g. add
resourceNames: - managed-upgrade-operator under the resources: -
serviceaccounts/token entry in the Role in Role-muo-other.yaml). Locate the
entry that mentions resources: - serviceaccounts/token and modify it to include
the specific service account name(s) (names only, since the Role is namespaced)
to limit token creation to those SAs.
In `@deploy_pko/RoleBinding-muo-other.yaml`:
- Around line 9-12: Add the missing roleRef.apiGroup fields to the RoleBinding
manifests: for the roleRef blocks (e.g., roleRef with kind: Role and name:
muo-monitoring-interactor) add roleRef.apiGroup: rbac.authorization.k8s.io so
the RoleBinding objects validate against rbac.authorization.k8s.io/v1; update
both occurrences (the roleRef at lines shown and the other roleRef block
referencing the same role/name) to include this apiGroup entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20d878b5-20ce-482d-80de-b9ff841edd19
📒 Files selected for processing (3)
deploy_pko/NetworkPolicy-muo.yamldeploy_pko/Role-muo-other.yamldeploy_pko/RoleBinding-muo-other.yaml
| ingress: | ||
| - from: | ||
| - namespaceSelector: | ||
| matchLabels: | ||
| kubernetes.io/metadata.name: openshift-managed-upgrade-operator | ||
| podSelector: {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat deploy_pko/NetworkPolicy-muo.yamlRepository: openshift/managed-upgrade-operator
Length of output: 557
Ingress rule allows any pod from source namespace on all ports.
Lines 10–15 apply this policy to every pod in the target namespace (podSelector: {}) and allow traffic from any pod in openshift-managed-upgrade-operator on all ports. Tighten the rule by specifying:
podSelectorwithin thefromclause to restrict which source pods need accesspodSelectorin the policy target to restrict which pods in the target namespace need this ingress- Explicit
portsarray to allow only required ports
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy_pko/NetworkPolicy-muo.yaml` around lines 10 - 15, The NetworkPolicy
currently allows all pods in the target namespace (podSelector: {}) to be
contacted from any pod in the openshift-managed-upgrade-operator namespace;
tighten it by replacing the empty top-level podSelector with a specific label
selector that matches only the intended target pods (e.g., app: my-app), add a
podSelector under the ingress->from block (next to namespaceSelector) to
restrict source pods in the openshift-managed-upgrade-operator namespace to only
those with the required labels, and add an explicit ports array under ingress to
enumerate only the required ports/protocols instead of allowing all ports;
update the YAML fields ingress, podSelector, namespaceSelector, and ports
accordingly.
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - serviceaccounts/token | ||
| verbs: | ||
| - create |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Role-muo-other.yaml" -o -name "Role-muo-other.yml"Repository: openshift/managed-upgrade-operator
Length of output: 110
🏁 Script executed:
git ls-files | grep -i "muo-other"Repository: openshift/managed-upgrade-operator
Length of output: 146
🏁 Script executed:
git ls-files | grep -i "role.*yaml" | head -20Repository: openshift/managed-upgrade-operator
Length of output: 857
🏁 Script executed:
cat -n deploy_pko/Role-muo-other.yamlRepository: openshift/managed-upgrade-operator
Length of output: 1812
🏁 Script executed:
cat -n deploy_pko/RoleBinding-muo-other.yamlRepository: openshift/managed-upgrade-operator
Length of output: 1708
🏁 Script executed:
rg "serviceaccounts/token" --type yamlRepository: openshift/managed-upgrade-operator
Length of output: 306
🏁 Script executed:
rg "openshift-monitoring" --type yaml -A 2 -B 2 | head -50Repository: openshift/managed-upgrade-operator
Length of output: 2929
🏁 Script executed:
rg "managed-upgrade-operator" deploy_pko/ --type yaml -C 3 | grep -A 5 -B 5 "serviceaccounts"Repository: openshift/managed-upgrade-operator
Length of output: 60
🏁 Script executed:
rg "token" deploy_pko/Role-muo-other.yaml -B 10 -A 10Repository: openshift/managed-upgrade-operator
Length of output: 417
🏁 Script executed:
rg "managed-upgrade-operator" . --type yaml | grep -i tokenRepository: openshift/managed-upgrade-operator
Length of output: 60
🏁 Script executed:
find . -name "*.md" -o -name "*.txt" | xargs rg -l "token\|serviceaccount" 2>/dev/null | head -5Repository: openshift/managed-upgrade-operator
Length of output: 60
🏁 Script executed:
git log --oneline --all -S "serviceaccounts/token" | head -10Repository: openshift/managed-upgrade-operator
Length of output: 164
Restrict token minting permission to specific ServiceAccounts using resourceNames.
Lines 29–34 grant create on serviceaccounts/token without resourceNames, allowing the managed-upgrade-operator to mint tokens for any ServiceAccount in the openshift-monitoring namespace. This is a broad permission that increases risk if the operator pod is compromised. Restrict this to only the specific ServiceAccount(s) that require token creation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy_pko/Role-muo-other.yaml` around lines 29 - 34, The Role currently
allows create on serviceaccounts/token for any ServiceAccount; restrict token
minting by adding a resourceNames stanza listing only the ServiceAccount(s) that
should be allowed (e.g. add resourceNames: - managed-upgrade-operator under the
resources: - serviceaccounts/token entry in the Role in Role-muo-other.yaml).
Locate the entry that mentions resources: - serviceaccounts/token and modify it
to include the specific service account name(s) (names only, since the Role is
namespaced) to limit token creation to those SAs.
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the file in the repository
find . -name "Role-muo-other.yaml" -o -name "*muo-other*"Repository: openshift/managed-upgrade-operator
Length of output: 150
🏁 Script executed:
# List files in deploy_pko directory to understand structure
ls -la deploy_pko/ 2>/dev/null || echo "Directory not found, searching for it..."
find . -type d -name "deploy_pko" 2>/dev/nullRepository: openshift/managed-upgrade-operator
Length of output: 1389
🏁 Script executed:
# Search for Role or role-related files to understand the structure
fd "\.yaml$|\.yml$" | grep -i "role\|pko" | head -20Repository: openshift/managed-upgrade-operator
Length of output: 879
🏁 Script executed:
# Read the Role-muo-other.yaml file to see full context
cat -n deploy_pko/Role-muo-other.yamlRepository: openshift/managed-upgrade-operator
Length of output: 1812
🏁 Script executed:
# Also check the RoleBinding to understand what subjects are bound to this role
cat -n deploy_pko/RoleBinding-muo-other.yamlRepository: openshift/managed-upgrade-operator
Length of output: 1708
🏁 Script executed:
# Search for any references to "pull-secret" or "pullsecret" in the codebase
rg -i "pull-secret|pullsecret" --type yamlRepository: openshift/managed-upgrade-operator
Length of output: 1053
🏁 Script executed:
# Look for any code that uses this role or interacts with secrets
rg "muo-other|muo-pullsecret"Repository: openshift/managed-upgrade-operator
Length of output: 1053
🏁 Script executed:
# Search for code that uses this role or interacts with openshift-config secrets
rg "openshift-config" -A 2 -B 2 --type go --type python | head -50Repository: openshift/managed-upgrade-operator
Length of output: 108
🏁 Script executed:
# Look for code that references pull-secret or reads secrets
rg -i "pull-secret|openshift-config.*secret" --type go --type python | head -30Repository: openshift/managed-upgrade-operator
Length of output: 108
🏁 Script executed:
# Check the Go code to understand what secret the operator needs to access
find . -name "*.go" -type f | xargs rg "openshift-config" | grep -i secretRepository: openshift/managed-upgrade-operator
Length of output: 1999
🏁 Script executed:
# Look for any documentation or comments about why this role needs these verbs
rg "muo-pullsecret-reader|pullsecret" --type go -B 5 -A 5Repository: openshift/managed-upgrade-operator
Length of output: 60
🏁 Script executed:
# Check if there's any usage of list or watch on the pull-secret
rg -i "list|watch" util/access_token.go pkg/ocm/builder*.go | grep -i secretRepository: openshift/managed-upgrade-operator
Length of output: 60
🏁 Script executed:
# Examine the actual usage in access_token.go to confirm it's only "Get"
cat -n util/access_token.go | grep -A 10 -B 10 "pull-secret"Repository: openshift/managed-upgrade-operator
Length of output: 995
🏁 Script executed:
# Double-check if there are any other usages of list or watch in the openshift-config namespace
rg "List|Watch" util/access_token.go pkg/ocm/builder.goRepository: openshift/managed-upgrade-operator
Length of output: 60
Restrict muo-pullsecret-reader role to the specific pull-secret resource.
Lines 45–52 grant access to all secrets in openshift-config namespace. The code only reads the pull-secret by name (see util/access_token.go). Restrict this role using resourceNames: [pull-secret] and reduce verbs to [get] only, since list and watch are not used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy_pko/Role-muo-other.yaml` around lines 45 - 52, The Role currently
grants broad access to all secrets via apiGroups: "" and resources: secrets with
verbs get, list, watch; restrict the muo-pullsecret-reader role to only the
named secret by adding resourceNames: [pull-secret] under the secrets resource
and remove unused verbs so verbs contains only get (no list or watch) to match
util/access_token.go which only reads the pull-secret; update the Role
definition in Role-muo-other.yaml accordingly.
deploy_pko/NetworkPolicy-muo.yaml
Outdated
| name: allow-from-openshift-managed-upgrade-operator | ||
| namespace: openshift-deployment-validation-operator | ||
| annotations: | ||
| package-operator.run/phase: network |
There was a problem hiding this comment.
there is no such network phase.
Only available phases are crds|namespace|rbac|deploy
You can use deploy here.
There was a problem hiding this comment.
Thank you Tony. I could use policies as per ref https://github.com/package-operator/package-operator/blob/main/internal/packages/internal/packagekickstart/presets/phases.go#L22
WDYT ?
There was a problem hiding this comment.
Sounds good. Looks better. However, in this case, you need to update the manifest.yaml as well. Because manifest.yaml only defines 4 phases.
There was a problem hiding this comment.
Good catch Tony. I have added policies phase to manifest.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #588 +/- ##
=======================================
Coverage 53.59% 53.59%
=======================================
Files 123 123
Lines 6165 6165
=======================================
Hits 3304 3304
Misses 2668 2668
Partials 193 193 🚀 New features to boost your workflow:
|
|
/test validate |
|
/retest |
|
@chamalabey: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Thank you for the update. |
|
Have you tested the new change locally? |
|
/retest |
3 participants
What type of PR is this?
refactor
What this PR does / why we need it?
Moving configuration in SSS template to PKO packaging.
Which Jira/Github issue(s) this PR fixes?
_Fixes #_https://redhat.atlassian.net/browse/SREP-3990
Special notes for your reviewer:
Pre-checks (if applicable):