OCPBUGS-56173: Align frrk8s to upstream #2833
Conversation
fedepaol
commented
Nov 10, 2025
fedepaol
commented
- Adds status cleaner job that acts as webhook backend plus takes care of removing frrstatus instances related to those nodes where an frrk8s instance is not running anymore
- Adds readOnlyFileSystem=true and allowPrivilegeEscalation=false to the frrk8s controller container
|
/testwith openshift/frr#107 |
WalkthroughAdded two emptyDir volumes and mounts to the FRR DaemonSet and hardened the frr and controller containers. Introduced a new Deployment Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (1)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fedepaol The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/network/frr-k8s/frr-k8s.yaml(3 hunks)bindata/network/frr-k8s/node-status-cleaner.yaml(1 hunks)bindata/network/frr-k8s/webhook.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/network/frr-k8s/node-status-cleaner.yamlbindata/network/frr-k8s/webhook.yamlbindata/network/frr-k8s/frr-k8s.yaml
🪛 YAMLlint (1.37.1)
bindata/network/frr-k8s/frr-k8s.yaml
[error] 179-179: syntax error: expected , but found '-'
(syntax)
🔇 Additional comments (4)
bindata/network/frr-k8s/frr-k8s.yaml (2)
162-163: Verify readOnlyRootFilesystem=true compatibility with FRR runtime behavior.The
frrcontainer now hasreadOnlyRootFilesystem: trueadded alongside the new emptyDir volumes for/var/lib/frrand/var/tmp/frr. Confirm that FRR daemon can successfully initialize and write all necessary state/logs to these temporary volumes and doesn't require write access to other root filesystem paths.If the frr daemon requires write access to paths outside the mounted emptyDirs (e.g.,
/etc/frr), the readOnlyRootFilesystem setting may need adjustment or additional volume mounts.
48-51: Security hardening additions approved for frr container.The addition of
allowPrivilegeEscalation: falseandreadOnlyRootFilesystem: true, combined with emptyDir volumes for scratch space at/var/lib/frrand/var/tmp/frr, strengthens the security posture of the FRR daemon container. This aligns well with the PR's security hardening objectives.Also applies to: 162-163
bindata/network/frr-k8s/webhook.yaml (1)
14-14: Service selector correctly routes to status cleaner deployment.The selector change from
frr-k8s-webhook-servertofrr-k8s-statuscleaneraligns with the architectural refactoring to consolidate webhook serving into the status cleaner component. Ensure that the status cleaner deployment innode-status-cleaner.yamlincludes the labelcomponent: frr-k8s-statuscleanerin its pod template to match this selector.bindata/network/frr-k8s/node-status-cleaner.yaml (1)
1-82: Webhook configuration and security posture review.The status cleaner deployment is appropriately configured for webhook serving:
- HTTPS probes at
/healthzwith proper initial delays and timeouts (lines 48-62)- TLS certificate mounting from secret at
/tmp/k8s-webhook-server/serving-certs(lines 75-78)--disable-cert-rotation=trueflag (line 29) is appropriate since certificate lifecycle is managed externally via the secret- Tolerations for control-plane nodes (lines 68-73) enable webhook availability on cluster-critical nodes
privilegedSCC annotation (line 21) matches the frr-k8s DaemonSet requirementResource requests are minimal but appropriate for a webhook component (10m CPU, 50Mi memory).
fedepaol
force-pushed
the
alignfrrk8supstream
branch
4 times, most recently
from
5872e43 to
62f6e7f
Compare
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
bindata/network/frr-k8s/frr-k8s.yaml(3 hunks)bindata/network/frr-k8s/node-status-cleaner.yaml(1 hunks)bindata/network/frr-k8s/webhook.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- bindata/network/frr-k8s/webhook.yaml
- bindata/network/frr-k8s/frr-k8s.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/network/frr-k8s/node-status-cleaner.yaml
🔇 Additional comments (3)
bindata/network/frr-k8s/node-status-cleaner.yaml (3)
53-68: Probe configuration is sound.Liveness and readiness probes are properly configured with HTTPS and reasonable timing. ✓
12-18: Selector and template labels are now aligned.The Deployment selector (line 14) and pod template labels (line 18) both correctly use
component: frr-k8s-statuscleaner. This resolves the label mismatch flagged in the prior review.
73-88: Security configuration appropriate for privileged workload.The pod uses
hostNetwork: truewithopenshift.io/required-scc: privileged, which is necessary for network webhook functionality. The certificate volume is mounted read-only (line 72), and the service account is appropriately scoped. The security settings align with the workload's networking requirements.
| args: | ||
| - $(LOG_LEVEL) | ||
| - --disable-cert-rotation=true | ||
| - --namespace=$(NAMESPACE) | ||
| - --webhook-port=9123 | ||
| - --frrk8s-selector=component=frr-k8s |
There was a problem hiding this comment.
CRITICAL: Environment variables won't expand in args array.
Kubernetes does not perform shell-style variable expansion on args arrays. Lines 27 and 29 use $(LOG_LEVEL) and $(NAMESPACE) syntax, which will be passed literally to the container as the strings "$(LOG_LEVEL)" and "--namespace=$(NAMESPACE)", not their resolved values. This will cause the application to receive malformed arguments and fail.
To fix this, use a shell wrapper to enable variable expansion:
containers:
- command:
- - /statuscleaner
+ - sh
+ - -c
+ - /statuscleaner $(LOG_LEVEL) --disable-cert-rotation=true --namespace=$(NAMESPACE) --webhook-port=9123 --frrk8s-selector=component=frr-k8s
- args:
- - $(LOG_LEVEL)
- - --disable-cert-rotation=true
- - --namespace=$(NAMESPACE)
- - --webhook-port=9123
- - --frrk8s-selector=component=frr-k8sAlternatively, if the application supports reading from environment variables directly, refactor it to read LOG_LEVEL and NAMESPACE from the environment instead of command-line arguments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In bindata/network/frr-k8s/node-status-cleaner.yaml around lines 26 to 31 the
args array uses $(LOG_LEVEL) and $(NAMESPACE) which Kubernetes does not expand,
so those tokens will be passed literally; fix by either (A) switching to a shell
wrapper command that performs expansion (e.g. set command to run /bin/sh -c and
build the full CLI string using $LOG_LEVEL and $NAMESPACE so the shell expands
them at container start — ensure the image contains a shell), or (B) remove
those args and let the app read LOG_LEVEL and NAMESPACE from environment
variables (export them in env: and update the app startup code to read process
env instead); pick one approach and update the manifest accordingly.
|
/testwith openshift/cluster-network-operator/master/frrk8s-e2e openshift/frr#107 |
|
/testwith openshift/cluster-network-operator/master/e2e-metal-ipi-ovn-dualstack-bgp openshift/frr#107 |
|
/testwith openshift/cluster-network-operator/master/e2e-metal-ipi-ovn-dualstack-bgp openshift/frr#107 openshift/release#71624 |
|
/testwith openshift/cluster-network-operator/master/e2e-metal-ipi-ovn-dualstack-bgp openshift/frr#107 |
|
/testwith openshift/cluster-network-operator/master/frrk8s-e2e openshift/frr#107 |
1 similar comment
|
/testwith openshift/cluster-network-operator/master/frrk8s-e2e openshift/frr#107 |
|
/testwith openshift/cluster-network-operator/master/e2e-metal-ipi-ovn-dualstack-bgp openshift/frr#107 |
fedepaol
changed the title
openshift-ci-robot
added
jira/severity-important
|
@fedepaol: This pull request references Jira Issue OCPBUGS-56173, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/jira refresh |
|
@fedepaol: This pull request references Jira Issue OCPBUGS-56173, which is invalid:
Comment In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@fedepaol: This pull request references Jira Issue OCPBUGS-56173, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/override ci/prow/e2e-metal-ipi-ovn-dualstack-bgp override temporarily |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/e2e-metal-ipi-ovn-dualstack-bgp, ci/prow/e2e-metal-ipi-ovn-dualstack-bgp-local-gw In response to this:
Instructions 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. |
|
/retest |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
The status cleaner pod is in charge of removing stale frrnodestates resources from those nodes where frrk8s is not running anymore. At the same time, it acts as the backend for the validation webhook, so the configuration of the webhook now refers to it. Signed-off-by: Federico Paolinelli <[email protected]>
readOnlyRootFilesystem prevents containers from writing to the root filesystem, reducing attack surface and improving security posture by limiting potential malicious file modifications and ensuring immutable container runtime. allowPrivilegeEscalation=false prevents containers from gaining additional privileges beyond those initially granted, further hardening the security posture by blocking privilege escalation attacks. Signed-off-by: Federico Paolinelli <[email protected]>
fedepaol
force-pushed
the
alignfrrk8supstream
branch
from
62f6e7f to
fdff55f
Compare
|
/override ci/prow/security |
|
@jcaamano: Overrode contexts on behalf of jcaamano: ci/prow/security In response to this:
Instructions 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. |
|
/retest |
|
@fedepaol: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
3 participants
