CNF-22194 Core: ensure one copy of MachineConfigs in Core RDS#825
CNF-22194 Core: ensure one copy of MachineConfigs in Core RDS#825kchennag wants to merge 2 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kchennag 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 |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @kchennag. Thanks for your PR. I'm waiting for a openshift-kni member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughMachineConfig CRs are relocated from ChangesMachineConfig CR Relocation and Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
telco-core/configuration/compare.sh (1)
117-158: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winMaintenance contract is implicit in hardcoded pairs array.
The
pairsarray (lines 122-130) acts as an explicit contract: all MachineConfig files underinstall/extra-manifeststhat require validation must be listed here and have corresponding entries inreference-crs-kube-compare/. If a new file is added toinstall/extra-manifests/, it must be:
- Added to
reference-crs-kube-compare/- Added to the
pairsarrayWithout this update, the new file won't be diff-validated (though
check_no_machineconfig_in_reference_crswill still catch strays inreference-crs). This is reasonable for a controlled migration, but document this maintenance contract in a comment or the README to avoid future confusion.For now, the validation logic itself is solid: lines 145–151 positively assert version 3.2.0 and
data:,sctp, while lines 153–155 negatively assert no 2.2.0 or legacyfilesystemfields.Add comment documenting the pairs array contract
local -a pairs=( + # Contract: All MachineConfig files in install/extra-manifests that require + # validation against kube-compare-reference must be listed here. + # New files added to install/extra-manifests/ must be added to this list, + # reference-crs-kube-compare/, and reference-crs/custom-manifests/ (MCPs only). "control-plane-load-kernel-modules.yaml:optional/other/control-plane-load-kernel-modules.yaml"🤖 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 `@telco-core/configuration/compare.sh` around lines 117 - 158, The hardcoded pairs array in the compare_install_extra_manifests() function implements an implicit maintenance contract that is not documented. Add a comment above the pairs array declaration explaining that this array defines the validation contract: any new MachineConfig files added to install/extra-manifests/ must be explicitly added to the pairs array with their corresponding path in reference-crs-kube-compare/, otherwise they will not be diff-validated by this comparison function.
🤖 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.
Nitpick comments:
In `@telco-core/configuration/compare.sh`:
- Around line 117-158: The hardcoded pairs array in the
compare_install_extra_manifests() function implements an implicit maintenance
contract that is not documented. Add a comment above the pairs array declaration
explaining that this array defines the validation contract: any new
MachineConfig files added to install/extra-manifests/ must be explicitly added
to the pairs array with their corresponding path in reference-crs-kube-compare/,
otherwise they will not be diff-validated by this comparison function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: efa75f82-4a15-45cd-8a8f-ef965a66aa22
📒 Files selected for processing (28)
telco-core/configuration/Makefiletelco-core/configuration/compare.shtelco-core/configuration/core-overlay.yamltelco-core/configuration/reference-crs-kube-compare/compare_ignoretelco-core/configuration/reference-crs-kube-compare/metadata.yamltelco-core/configuration/reference-crs-kube-compare/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yamltelco-core/configuration/reference-crs-kube-compare/optional/other/control-plane-load-kernel-modules.yamltelco-core/configuration/reference-crs-kube-compare/optional/other/worker-load-kernel-modules.yamltelco-core/configuration/reference-crs/custom-manifests/README.mdtelco-core/configuration/reference-crs/custom-manifests/mcp-worker-1.yamltelco-core/configuration/reference-crs/custom-manifests/mcp-worker-2.yamltelco-core/configuration/reference-crs/custom-manifests/mcp-worker-3.yamltelco-core/configuration/reference-crs/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yamltelco-core/configuration/reference-crs/optional/other/control-plane-load-kernel-modules.yamltelco-core/configuration/reference-crs/optional/other/kdump-master.yamltelco-core/configuration/reference-crs/optional/other/kdump-worker.yamltelco-core/configuration/reference-crs/optional/other/mount_namespace_config_master.yamltelco-core/configuration/reference-crs/optional/other/mount_namespace_config_worker.yamltelco-core/configuration/reference-crs/optional/other/sctp_module_mc.yamltelco-core/configuration/reference-crs/optional/other/worker-load-kernel-modules.yamltelco-core/install/README.mdtelco-core/install/custom-manifests/README.mdtelco-core/install/example-standard-clusterinstance.yamltelco-core/install/extra-manifests/mcp-worker-1.yamltelco-core/install/extra-manifests/mcp-worker-2.yamltelco-core/install/extra-manifests/mcp-worker-3.yamltelco-core/install/extra-manifests/sctp_module_mc.yamltelco-core/install/kustomization.yaml
💤 Files with no reviewable changes (9)
- telco-core/configuration/reference-crs/optional/other/mount_namespace_config_master.yaml
- telco-core/install/example-standard-clusterinstance.yaml
- telco-core/configuration/reference-crs/optional/networking/multus/tap_cni/mc_rootless_pods_selinux.yaml
- telco-core/configuration/reference-crs/optional/other/kdump-master.yaml
- telco-core/configuration/reference-crs/optional/other/sctp_module_mc.yaml
- telco-core/configuration/reference-crs/optional/other/kdump-worker.yaml
- telco-core/configuration/reference-crs/optional/other/worker-load-kernel-modules.yaml
- telco-core/configuration/reference-crs/optional/other/mount_namespace_config_worker.yaml
- telco-core/configuration/reference-crs/optional/other/control-plane-load-kernel-modules.yaml
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@telco-core/configuration/compare.sh`:
- Around line 142-147: The grep commands on lines checking for 'version: 3.2.0'
and 'path: /etc/modules-load.d/kernel-load.conf' are using regex semantics where
the dot (.) character acts as a wildcard matching any character, which allows
incorrect versions or paths to pass validation. Fix this by either using grep -F
flag for fixed string matching instead of regex, or by escaping the dot
characters in the patterns with backslashes (e.g., 3\.2\.0 and
/etc/modules-load\.d/kernel-load\.conf) to ensure they match literal dots only.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2bd232a3-8d36-42a1-ace0-41a73699c58a
📒 Files selected for processing (1)
telco-core/configuration/compare.sh
| if ! grep -q 'version: 3.2.0' "${install}/${inst}"; then | ||
| echo "ERROR: ${install}/${inst} must use ignition version 3.2.0" >&2 | ||
| fail=1 | ||
| fi | ||
| if ! grep -q 'path: /etc/modules-load.d/kernel-load.conf' "${install}/${inst}"; then | ||
| echo "ERROR: ${install}/${inst} must configure kernel-load.conf" >&2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Harden invariant checks to avoid regex false positives.
On Line 142 and Line 146, grep -q is using regex semantics, so . acts as a wildcard. That can let malformed manifest values pass this check gate.
Proposed fix
- if ! grep -q 'version: 3.2.0' "${install}/${inst}"; then
+ if ! grep -Eq '^[[:space:]]*version:[[:space:]]*3\.2\.0([[:space:]]|$)' "${install}/${inst}"; then
echo "ERROR: ${install}/${inst} must use ignition version 3.2.0" >&2
fail=1
fi
- if ! grep -q 'path: /etc/modules-load.d/kernel-load.conf' "${install}/${inst}"; then
+ if ! grep -Eq '^[[:space:]]*path:[[:space:]]*/etc/modules-load\.d/kernel-load\.conf([[:space:]]|$)' "${install}/${inst}"; then
echo "ERROR: ${install}/${inst} must configure kernel-load.conf" >&2
fail=1
fi📝 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.
| if ! grep -q 'version: 3.2.0' "${install}/${inst}"; then | |
| echo "ERROR: ${install}/${inst} must use ignition version 3.2.0" >&2 | |
| fail=1 | |
| fi | |
| if ! grep -q 'path: /etc/modules-load.d/kernel-load.conf' "${install}/${inst}"; then | |
| echo "ERROR: ${install}/${inst} must configure kernel-load.conf" >&2 | |
| if ! grep -Eq '^[[:space:]]*version:[[:space:]]*3\.2\.0([[:space:]]|$)' "${install}/${inst}"; then | |
| echo "ERROR: ${install}/${inst} must use ignition version 3.2.0" >&2 | |
| fail=1 | |
| fi | |
| if ! grep -Eq '^[[:space:]]*path:[[:space:]]*/etc/modules-load\.d/kernel-load\.conf([[:space:]]|$)' "${install}/${inst}"; then | |
| echo "ERROR: ${install}/${inst} must configure kernel-load.conf" >&2 |
🤖 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 `@telco-core/configuration/compare.sh` around lines 142 - 147, The grep
commands on lines checking for 'version: 3.2.0' and 'path:
/etc/modules-load.d/kernel-load.conf' are using regex semantics where the dot
(.) character acts as a wildcard matching any character, which allows incorrect
versions or paths to pass validation. Fix this by either using grep -F flag for
fixed string matching instead of regex, or by escaping the dot characters in the
patterns with backslashes (e.g., 3\.2\.0 and
/etc/modules-load\.d/kernel-load\.conf) to ensure they match literal dots only.
3 participants
Core of CNF-22194.