OCPBUGS-54574: Cilium on management cluster with KubeVirt - #81307
OCPBUGS-54574: Cilium on management cluster with KubeVirt#81307mgencur wants to merge 8 commits into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@mgencur: This pull request references Jira Issue OCPBUGS-54574, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
mgencur
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughThis PR replaces the OLM-based Cilium manifest generation with a cilium CLI dry-run approach that produces individual manifest files in SHARED_DIR, and adds CNI_PROVIDER support to the hypershift kubevirt create step to apply Cilium network policies, wiring the conformance-cilium workflow to use Cilium. ChangesCilium CLI Manifest Generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Hypershift Kubevirt Cilium Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant ConfStep as cilium-conf step
participant CLI as cilium CLI
participant SharedDir as SHARED_DIR
participant CreateStep as hypershift-kubevirt-create
participant Cluster as Target Cluster
CI->>ConfStep: run cilium-conf-commands.sh
ConfStep->>CLI: download & run cilium install --dry-run
CLI-->>ConfStep: multi-document manifest YAML
ConfStep->>SharedDir: write split manifest_cilium-*.yaml files
CI->>CreateStep: run hypershift-kubevirt-create-commands.sh (CNI_PROVIDER=cilium)
CreateStep->>Cluster: create control-plane namespace & NetworkAttachmentDefinition
CreateStep->>Cluster: check ciliumnetworkpolicies CRD exists
CreateStep->>Cluster: apply CiliumNetworkPolicy resources
Related PRs: None specified. Suggested labels: None specified. Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
![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
🤖 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 `@ci-operator/step-registry/cilium/conf/cilium-conf-commands.sh`:
- Around line 29-33: The Cilium CLI download in cilium-conf-commands.sh still
pulls the tarball directly into /tmp/bin without checksum verification, so
update the download flow to use a private temporary directory created with
mktemp -d. In the Cilium CLI install step, download the tarball and its
published cilium-linux-amd64.tar.gz.sha256sum, validate the archive before
extracting, then extract and run the binary from that temp path instead of
/tmp/bin. Keep the changes localized to the existing download/install commands
so the cilium CLI setup remains easy to find and maintain.
In
`@ci-operator/step-registry/cucushift/hypershift-extended/cilium/network-policies/cucushift-hypershift-extended-cilium-network-policies-commands.sh`:
- Around line 40-46: The namespace derivation in the cilium network-policies
script can silently produce an invalid CONTROL_PLANE_NS when HC_NAMESPACE is
empty. Add an explicit check right after the hostedcluster lookup in
cucushift-hypershift-extended-cilium-network-policies-commands.sh: if the oc get
hostedcluster/jsonpath result is empty, fail fast with a clear error mentioning
CLUSTER_NAME and the hostedcluster lookup, before building CONTROL_PLANE_NS or
continuing to later apply steps.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4c0b9294-2939-4ee1-873e-387ef608a785
📒 Files selected for processing (4)
ci-operator/step-registry/cilium/conf/cilium-conf-commands.shci-operator/step-registry/cilium/conf/cilium-conf-ref.yamlci-operator/step-registry/cucushift/hypershift-extended/cilium/network-policies/cucushift-hypershift-extended-cilium-network-policies-commands.shci-operator/step-registry/hypershift/kubevirt/baremetalds/conformance-cilium/hypershift-kubevirt-baremetalds-conformance-cilium-workflow.yaml
| # Workaround for OCPBUGS-54574: Cilium incorrectly blocks virt-launcher egress due to | ||
| # broken ipBlock.CIDR handling (cilium/cilium#9209). Allow egress to host entities so | ||
| # virt-launcher pods can reach the ignition server via the ingress VIP. | ||
| CLUSTER_NAME=$(cat "${SHARED_DIR}/cluster-name") | ||
| HC_NAMESPACE=$(oc get hostedcluster -A -o jsonpath="{.items[?(@.metadata.name=='${CLUSTER_NAME}')].metadata.namespace}") | ||
| CONTROL_PLANE_NS="${HC_NAMESPACE}-${CLUSTER_NAME}" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add validation for empty HC_NAMESPACE.
If no hostedcluster matches CLUSTER_NAME (e.g., not yet created, or a naming mismatch), the jsonpath filter returns an empty string silently, and CONTROL_PLANE_NS becomes -${CLUSTER_NAME} — an invalid Kubernetes namespace (leading hyphen). This surfaces as an opaque oc apply failure rather than a clear diagnostic.
🛡️ Proposed fix
CLUSTER_NAME=$(cat "${SHARED_DIR}/cluster-name")
HC_NAMESPACE=$(oc get hostedcluster -A -o jsonpath="{.items[?(@.metadata.name=='${CLUSTER_NAME}')].metadata.namespace}")
+if [[ -z "${HC_NAMESPACE}" ]]; then
+ echo "ERROR: could not find hostedcluster named '${CLUSTER_NAME}'" >&2
+ exit 1
+fi
CONTROL_PLANE_NS="${HC_NAMESPACE}-${CLUSTER_NAME}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Workaround for OCPBUGS-54574: Cilium incorrectly blocks virt-launcher egress due to | |
| # broken ipBlock.CIDR handling (cilium/cilium#9209). Allow egress to host entities so | |
| # virt-launcher pods can reach the ignition server via the ingress VIP. | |
| CLUSTER_NAME=$(cat "${SHARED_DIR}/cluster-name") | |
| HC_NAMESPACE=$(oc get hostedcluster -A -o jsonpath="{.items[?(@.metadata.name=='${CLUSTER_NAME}')].metadata.namespace}") | |
| CONTROL_PLANE_NS="${HC_NAMESPACE}-${CLUSTER_NAME}" | |
| # Workaround for OCPBUGS-54574: Cilium incorrectly blocks virt-launcher egress due to | |
| # broken ipBlock.CIDR handling (cilium/cilium#9209). Allow egress to host entities so | |
| # virt-launcher pods can reach the ignition server via the ingress VIP. | |
| CLUSTER_NAME=$(cat "${SHARED_DIR}/cluster-name") | |
| HC_NAMESPACE=$(oc get hostedcluster -A -o jsonpath="{.items[?(@.metadata.name=='${CLUSTER_NAME}')].metadata.namespace}") | |
| if [[ -z "${HC_NAMESPACE}" ]]; then | |
| echo "ERROR: could not find hostedcluster named '${CLUSTER_NAME}'" >&2 | |
| exit 1 | |
| fi | |
| CONTROL_PLANE_NS="${HC_NAMESPACE}-${CLUSTER_NAME}" |
🤖 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
`@ci-operator/step-registry/cucushift/hypershift-extended/cilium/network-policies/cucushift-hypershift-extended-cilium-network-policies-commands.sh`
around lines 40 - 46, The namespace derivation in the cilium network-policies
script can silently produce an invalid CONTROL_PLANE_NS when HC_NAMESPACE is
empty. Add an explicit check right after the hostedcluster lookup in
cucushift-hypershift-extended-cilium-network-policies-commands.sh: if the oc get
hostedcluster/jsonpath result is empty, fail fast with a clear error mentioning
CLUSTER_NAME and the hostedcluster lookup, before building CONTROL_PLANE_NS or
continuing to later apply steps.
There was a problem hiding this comment.
This is a minor issue, IMO. It will fail anyway.
There was a problem hiding this comment.
@mgencur, understood — since oc apply will fail anyway if CONTROL_PLANE_NS is malformed, the extra validation is just for a clearer error message. I'll leave it as-is per your preference.
f904fcc to
b0c35f3
Compare
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ci-operator/step-registry/cilium/conf/cilium-conf-commands.sh`:
- Around line 68-86: The Cilium SCC binding currently grants
system:openshift:scc:privileged to three service accounts and enables privileged
security context, so the exception needs to be narrowly scoped or replaced.
Update cilium-conf-commands.sh around the manifest generation for
manifest_03-cilium-scc-privileged.yaml and the Cilium securityContext/privileged
settings so only the minimum required service accounts are bound, and add inline
documentation justifying why cilium, cilium-operator, and cilium-envoy each need
privileged access; otherwise switch to a custom-scoped SCC or least-privilege
binding.
In
`@ci-operator/step-registry/hypershift/kubevirt/baremetalds/conformance-cilium/hypershift-kubevirt-baremetalds-conformance-cilium-workflow.yaml`:
- Around line 68-76: The workflow’s local registry setting is being overridden
to empty, which cancels the step’s default ENABLE_LOCAL_REGISTRY=true. Update
the hypershift-kubevirt-baremetalds-conformance-cilium workflow so the override
keeps ENABLE_LOCAL_REGISTRY enabled, either by setting
ENABLE_LOCAL_REGISTRY=true or removing the empty assignment, and verify the env
block in the conformance-cilium workflow remains consistent with the step’s
registry support.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0a779c52-52bf-4fa7-9dcb-8f0127775c67
📒 Files selected for processing (4)
ci-operator/step-registry/cilium/conf/cilium-conf-commands.shci-operator/step-registry/cilium/conf/cilium-conf-ref.yamlci-operator/step-registry/cucushift/hypershift-extended/cilium/network-policies/cucushift-hypershift-extended-cilium-network-policies-commands.shci-operator/step-registry/hypershift/kubevirt/baremetalds/conformance-cilium/hypershift-kubevirt-baremetalds-conformance-cilium-workflow.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- ci-operator/step-registry/cucushift/hypershift-extended/cilium/network-policies/cucushift-hypershift-extended-cilium-network-policies-commands.sh
- ci-operator/step-registry/cilium/conf/cilium-conf-ref.yaml
b0c35f3 to
0dacab5
Compare
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
0dacab5 to
f734471
Compare
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ci-operator/step-registry/cilium/conf/cilium-conf-commands.sh`:
- Line 127: The cilium install command in conf/cilium-conf-commands.sh ignores
the documented ENDPOINT_ROUTES and HUBBLE overrides from ref.yaml. Update the
command construction in the cilium-conf script so endpointRoutes.enabled is
driven by "${ENDPOINT_ROUTES}" instead of a hardcoded true, and add a matching
--set hubble.enabled="${HUBBLE}" entry in the same install path. Keep the change
aligned with the existing cilium-conf command builder so the env vars actually
control the generated manifests.
- Around line 137-150: The cilium manifest split logic in
cilium-conf-commands.sh writes to predictable /tmp paths before moving files
into ${SHARED_DIR}, which should be hardened. Update the install flow around the
/tmp/cilium-install-all.yaml write and the csplit loop to use a private
temporary directory created at runtime instead of fixed filenames. Ensure the
generated manifest chunks are created and processed only from that private dir,
then copied into ${SHARED_DIR} with the existing manifest_cilium-* naming
scheme.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9dfe82f4-ae4e-4a2e-9784-94cfc5ded767
📒 Files selected for processing (2)
ci-operator/step-registry/cilium/conf/cilium-conf-commands.shci-operator/step-registry/hypershift/kubevirt/baremetalds/conformance-cilium/hypershift-kubevirt-baremetalds-conformance-cilium-workflow.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- ci-operator/step-registry/hypershift/kubevirt/baremetalds/conformance-cilium/hypershift-kubevirt-baremetalds-conformance-cilium-workflow.yaml
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
f734471 to
cef1601
Compare
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@mgencur: This pull request references Jira Issue OCPBUGS-54574, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (mgencur@redhat.com), skipping review request. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
This seems to be blocked by https://redhat.atlassian.net/browse/MGMT-24854 |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: your |
The cilium-cni-override ConfigMap is no longer needed now that the upstream cniVersion issue is resolved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dc78205 to
4884e68
Compare
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
…ures Combined with kube-proxy-replacement=false, socketLB intercepted service traffic at the socket layer and caused cluster-wide "no route to host" errors for ClusterIPs (e.g. the kube-apiserver service), breaking many pods across the cluster.
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-e2e-kubevirt-metal-conformance-cilium |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
A total of 345 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@mgencur: The following test 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. |
|
This now depends on #82712 |
2 participants
Includes a workaround for https://redhat.atlassian.net/browse/OCPBUGS-54574, among other things.
from Cilium 1.13.9 to 1.19.4
traffic (OCPBUGS-54574)
step (OCPBUGS-54574)