refactor(cilium-conf): migrate from OLM to cilium install --dry-run#81398
refactor(cilium-conf): migrate from OLM to cilium install --dry-run#81398mgencur wants to merge 1 commit into
Conversation
Replace deprecated isovalent/olm-for-cilium OLM manifest download with cilium CLI's install --dry-run to generate day-0 manifests. This aligns with the cucushift reference script's approach while preserving the day-0 nature of the step (manifests stored in SHARED_DIR for installer). Changes: - Download cilium CLI v0.19.2 and use it to render Helm chart manifests - Add cniVersion override ConfigMap manifest (OCPBUGS-86033 workaround) - Add SCC ClusterRoleBinding for cilium service accounts - Update CILIUM_VERSION from 1.13.9 to 1.19.4 - Add CILIUM_CLI_VERSION env var (default 0.19.2) to ref.yaml Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughThe Cilium configuration step script and its reference YAML were updated. The script now installs cilium-cli and generates Cilium CNI manifests dynamically via ChangesCilium CNI Manifest Generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Script as cilium-conf-commands.sh
participant CLI as cilium-cli
participant SharedDir as SHARED_DIR
Script->>CLI: install cilium-cli tarball
Script->>SharedDir: write namespace, ConfigMap, NetworkPolicy manifests
Script->>SharedDir: write ClusterRoleBinding manifest
Script->>CLI: cilium install --dry-run
CLI-->>Script: multi-document YAML
Script->>Script: split YAML into per-document files
Script->>SharedDir: write per-manifest files named by kind/name
Related Issues: None found Related PRs: None found Suggested labels: ci-operator, step-registry, cilium Suggested reviewers: None specified 🐰💨
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/pj-rehearse periodic-ci-openshift-hypershift-release-4.22-periodics-mce-e2e-agent-connected-cilium-ipv4-metal-conformance |
|
@mgencur: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mgencur 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 |
|
[REHEARSALNOTIFIER]
A total of 55 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: |
![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 32-37: Install cilium-cli into an isolated temporary directory
instead of the fixed /tmp/bin path in the cilium-conf-commands.sh setup block.
Update the install flow around the cilium-cli download/extract commands to
create a unique directory with mktemp -d, extract the tarball there, chmod the
resulting cilium binary, and export PATH using that generated directory so the
install cannot collide with existing files or symlinks.
- Around line 136-139: The Helm --set arguments in the cilium-conf-commands.sh
command block are vulnerable to shell globbing because the array-style keys are
unquoted. Update the helm invocation around the extraVolumes and
extraVolumeMounts settings so the keys like extraVolumes[0],
extraVolumes[0].configMap.name, extraVolumeMounts[0], and
extraVolumeMounts[0].mountPath are quoted consistently, using the same command
block that builds the Cilium install arguments.
🪄 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: 278b0c5f-91f5-420a-aa65-42067951d109
📒 Files selected for processing (2)
ci-operator/step-registry/cilium/conf/cilium-conf-commands.shci-operator/step-registry/cilium/conf/cilium-conf-ref.yaml
| --set extraVolumes[0].name=cni-override \ | ||
| --set extraVolumes[0].configMap.name=cilium-cni-override \ | ||
| --set extraVolumeMounts[0].name=cni-override \ | ||
| --set extraVolumeMounts[0].mountPath=/etc/cilium-cni \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
tmp="$(mktemp -d)"
cd "$tmp"
touch 'extraVolumes0.name=cni-override'
printf '<%s>\n' --set extraVolumes[0].name=cni-override
printf '<%s>\n' --set 'extraVolumes[0].name=cni-override'Repository: openshift/release
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="ci-operator/step-registry/cilium/conf/cilium-conf-commands.sh"
# Show the relevant section with line numbers
sed -n '120,150p' "$file" | cat -n
# Check whether the script is using shell globbing-sensitive Helm --set keys in a command context
rg -n --fixed-strings --context 2 'extraVolumes[0].name=cni-override|extraVolumeMounts[0].name=cni-override|extraVolumes[0].configMap.name=cilium-cni-override|extraVolumeMounts[0].mountPath=/etc/cilium-cni' "$file"Repository: openshift/release
Length of output: 1813
Quote the Helm array keys here. extraVolumes[0] and extraVolumeMounts[0] are subject to shell globbing, so a matching filename in the working directory can change the helm --set arguments.
🤖 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/cilium/conf/cilium-conf-commands.sh` around lines
136 - 139, The Helm --set arguments in the cilium-conf-commands.sh command block
are vulnerable to shell globbing because the array-style keys are unquoted.
Update the helm invocation around the extraVolumes and extraVolumeMounts
settings so the keys like extraVolumes[0], extraVolumes[0].configMap.name,
extraVolumeMounts[0], and extraVolumeMounts[0].mountPath are quoted
consistently, using the same command block that builds the Cilium install
arguments.
|
@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. |
1 participant
Replace deprecated isovalent/olm-for-cilium OLM manifest download with cilium CLI's install --dry-run to generate day-0 manifests. This aligns with the cucushift reference script's approach while preserving the day-0 nature of the step (manifests stored in SHARED_DIR for installer).
Changes:
Summary by CodeRabbit
This updates the Cilium day-0 manifest generation used by OpenShift CI so it no longer depends on downloading the deprecated OLM-for-Cilium bundle. Instead, the step now uses
cilium-cliwithcilium install --dry-runto render the manifests, stores them inSHARED_DIR, and aligns the flow with the cucushift reference approach.Practically, this changes the CI bootstrap assets for Cilium installation by:
1.19.40.19.2cniVersionoverride workaround for the affected cluster setup