[DNM] Use custom layered image to test nm-libreswan fix #2856
Conversation
WalkthroughUpdates two IPsec MachineConfig manifests to add an annotation and an osImageURL, change an ipsecenabler.service ExecStartPre to use systemd-tmpfiles, and remove the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test ? |
|
@pperiyasamy: The following commands are available to trigger required jobs: The following commands are available to trigger optional jobs: Use DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pperiyasamy 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 |
pperiyasamy
force-pushed
the
dummy-pr-2
branch
from
447f9b8 to
ebd1e8b
Compare
|
@pperiyasamy: The following tests failed, say
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. |
test duplicate key 'phase2alg' fix for nncp deployment Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>
pperiyasamy
force-pushed
the
dummy-pr-2
branch
from
ebd1e8b to
b5ebec3
Compare
![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
🧹 Nitpick comments (1)
pkg/network/ovn_kubernetes.go (1)
651-651: Remove commented-out code and unused function.The commented line at 651 should be deleted rather than left in the codebase. Additionally, the function
isUserDefinedIPsecMachineConfigPresent(lines 1604-1615) is no longer called anywhere and should be removed to eliminate dead code.Apply this diff to remove the commented line:
- //isUserDefinedIPsecMachineConfigPresent := isUserDefinedIPsecMachineConfigPresent(bootstrapResult.Infra)Consider removing the unused function
isUserDefinedIPsecMachineConfigPresentat lines 1604-1615 as well.
📜 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/ovn-kubernetes/common/80-ipsec-master-extensions.yaml(2 hunks)bindata/network/ovn-kubernetes/common/80-ipsec-worker-extensions.yaml(2 hunks)pkg/network/ovn_kubernetes.go(2 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:
pkg/network/ovn_kubernetes.gobindata/network/ovn-kubernetes/common/80-ipsec-worker-extensions.yamlbindata/network/ovn-kubernetes/common/80-ipsec-master-extensions.yaml
🔇 Additional comments (4)
bindata/network/ovn-kubernetes/common/80-ipsec-worker-extensions.yaml (1)
26-26: ExecStartPre change aligns with layered image approach.The change from removing
/etc/ipsec.d/cno.confto usingsystemd-tmpfiles --createis appropriate for the layered image workflow, ensuring libreswan is properly initialized from the rpm-ostree tmpfiles configuration.bindata/network/ovn-kubernetes/common/80-ipsec-master-extensions.yaml (1)
26-26: ExecStartPre change aligns with layered image approach.The change from removing
/etc/ipsec.d/cno.confto usingsystemd-tmpfiles --createis appropriate for the layered image workflow, ensuring libreswan is properly initialized from the rpm-ostree tmpfiles configuration.pkg/network/ovn_kubernetes.go (2)
659-659: IPsec activation logic simplified appropriately.The change to
isOVNIPsecActiveremoves the requirement for user-defined IPsec MachineConfigs, simplifying the activation logic to depend only on CNO-managed IPsec MachineConfigs or hypershift hosting. This aligns with the PR's objective to ignore user-defined IPsec MachineConfigs.
670-670: Rendering logic updated consistently.The change to
renderCNOIPsecMachineConfigappropriately removes the dependency on user-defined IPsec MachineConfigs, consistent with the broader changes in this PR to simplify IPsec MachineConfig management.
| annotations: | ||
| user-ipsec-machine-config: "true" |
There was a problem hiding this comment.
Misleading annotation name for CNO-managed resource.
The annotation user-ipsec-machine-config: "true" is misleading since this MachineConfig is managed by CNO, not user-defined. This could cause confusion about resource ownership and lifecycle management.
Consider renaming to reflect CNO ownership, e.g., cno-ipsec-machine-config: "true" or network.operator.openshift.io/ipsec-machine-config: "true".
🤖 Prompt for AI Agents
In bindata/network/ovn-kubernetes/common/80-ipsec-master-extensions.yaml around
lines 7-8, the annotation key user-ipsec-machine-config is misleading for a
CNO-managed MachineConfig; replace or rename it to indicate CNO ownership (for
example cno-ipsec-machine-config: "true" or
network.operator.openshift.io/ipsec-machine-config: "true") so the annotation
accurately reflects resource ownership and lifecycle; update any consumers or
documentation that reference the old key to use the new annotation name.
| user-ipsec-machine-config: "true" | ||
| name: 80-ipsec-master-extensions | ||
| spec: | ||
| osImageURL: quay.io/pepalani/ipsec-rhcos-layered-image:nm-libreswan-fix |
There was a problem hiding this comment.
Hardcoded test image URL must not be merged.
The osImageURL points to a personal registry (quay.io/pepalani/...) which is not suitable for production. Since this PR is marked [DNM] for testing the nm-libreswan fix, ensure this reference is replaced with an official image registry before any production use.
🤖 Prompt for AI Agents
In bindata/network/ovn-kubernetes/common/80-ipsec-master-extensions.yaml around
line 11, the osImageURL is hardcoded to a personal test image
(quay.io/pepalani/ipsec-rhcos-layered-image:nm-libreswan-fix); replace this test
registry reference with the official, approved image registry and tag used by
the project (or parameterize the value via config/CI variables), ensure no
personal quay.io references remain, and update any documentation/tests to use
the official image before merging to production.
| annotations: | ||
| user-ipsec-machine-config: "true" |
There was a problem hiding this comment.
Misleading annotation name for CNO-managed resource.
The annotation user-ipsec-machine-config: "true" is misleading since this MachineConfig is managed by CNO, not user-defined. This could cause confusion about resource ownership and lifecycle management.
Consider renaming to reflect CNO ownership, e.g., cno-ipsec-machine-config: "true" or network.operator.openshift.io/ipsec-machine-config: "true".
🤖 Prompt for AI Agents
In bindata/network/ovn-kubernetes/common/80-ipsec-worker-extensions.yaml around
lines 7-8, the annotation key `user-ipsec-machine-config: "true"` is misleading
for a CNO-managed MachineConfig; rename the annotation to indicate CNO ownership
(for example `cno-ipsec-machine-config: "true"` or
`network.operator.openshift.io/ipsec-machine-config: "true"`) and update any
consuming code, documentation, and tests to use the new annotation key so
ownership and lifecycle intent are unambiguous.
| user-ipsec-machine-config: "true" | ||
| name: 80-ipsec-worker-extensions | ||
| spec: | ||
| osImageURL: quay.io/pepalani/ipsec-rhcos-layered-image:nm-libreswan-fix |
There was a problem hiding this comment.
Hardcoded test image URL must not be merged.
The osImageURL points to a personal registry (quay.io/pepalani/...) which is not suitable for production. Since this PR is marked [DNM] for testing the nm-libreswan fix, ensure this reference is replaced with an official image registry before any production use.
🤖 Prompt for AI Agents
In bindata/network/ovn-kubernetes/common/80-ipsec-worker-extensions.yaml around
line 11 the osImageURL is hardcoded to a personal test registry
(quay.io/pepalani/ipsec-rhcos-layered-image:nm-libreswan-fix); replace this
value with the official, approved production image reference (or a configurable
variable) before merging — update the field to point to the canonical
registry/image tag used by the project (or read from environment/params) and
remove any personal/test registry references.
1 participant
test duplicate key 'phase2alg' fix for nncp deployment