DNM: add tls and pqc checks for RHWA operators - #81090
Conversation
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
|
/hold |
WalkthroughThe PR adds a ChangesMedik8s TLS scanner CI configs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@weshayutin: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8__5.0.yaml (1)
128-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPeriodics in a release-branch config are fragile and duplicate the main config's periodics.
Release-branch configs are derived from
mainbyconfig-brancher, so manually-added tests here risk being overwritten, and these cron periodics run the same code already scheduled bymedik8s-fence-agents-remediation-main__5.0.yaml. Move release-specific periodics into a__periodics.yamlrather than the branch config.As per coding guidelines: "we cannot put them in the default release branch CI configurations because that's owned by config-brancher enforcing that the release branch config is derived from main config."
Also applies to: 193-225
🤖 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/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8__5.0.yaml` around lines 128 - 159, The release-branch config contains manually added cron periodics that duplicate the main branch’s scheduled tests and will be overwritten by config-brancher. Remove the periodic test block(s) from the branch config and move them into the corresponding __periodics.yaml, keeping only branch-derived config in the release file and preserving the existing tls-pqc-readiness-periodic definition there if it belongs in the periodics-only config.Source: Coding guidelines
ci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main__5.0.yaml (1)
128-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider placing cron periodics in a dedicated
__periodics.yamlinstead of the main config.This repo's convention separates periodic (
cron:/interval:) jobs into<org>-<repo>-<branch>__periodics.yamlso the generated jobs land in a separate-periodics.yamland the data is consumable by CI analytical tooling. Definingtls-pqc-readiness-periodicandtls13-adherence-periodicdirectly in the main config also means they get duplicated when this config is branched, doubling execution of identical code.As per coding guidelines: "For jobs producing test data that will be processed by CI analytical tooling, periodic jobs should be configured separately from the main config" and "
--skip-periodics" exists to avoid duplicate periodic execution across branches.Also applies to: 193-225
🤖 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/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main__5.0.yaml` around lines 128 - 159, Move the periodic job definitions out of the main config and into the matching __periodics.yaml so they generate into a separate -periodics.yaml and don’t get duplicated across branches. Update the configuration around tls-pqc-readiness-periodic and tls13-adherence-periodic to follow the repo’s periodic-job convention, keeping only non-periodic jobs in the main medik8s-fence-agents-remediation config.Source: Coding guidelines
🤖 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/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml`:
- Around line 109-111: The install-operator step still uses the unpinned
self-node-remediation bundle image, so update the operator-sdk run bundle
invocation in the install flow to replace
quay.io/medik8s/self-node-remediation-operator-bundle:latest with a versioned
bundle tag or digest. Keep the existing extra install and the subsequent
"$OO_BUNDLE" install plus the oc wait on node-healthcheck-controller-manager,
but make sure the self-node-remediation bundle reference is pinned in the same
step.
---
Nitpick comments:
In
`@ci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main__5.0.yaml`:
- Around line 128-159: Move the periodic job definitions out of the main config
and into the matching __periodics.yaml so they generate into a separate
-periodics.yaml and don’t get duplicated across branches. Update the
configuration around tls-pqc-readiness-periodic and tls13-adherence-periodic to
follow the repo’s periodic-job convention, keeping only non-periodic jobs in the
main medik8s-fence-agents-remediation config.
In
`@ci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8__5.0.yaml`:
- Around line 128-159: The release-branch config contains manually added cron
periodics that duplicate the main branch’s scheduled tests and will be
overwritten by config-brancher. Remove the periodic test block(s) from the
branch config and move them into the corresponding __periodics.yaml, keeping
only branch-derived config in the release file and preserving the existing
tls-pqc-readiness-periodic definition there if it belongs in the periodics-only
config.
🪄 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: 6a8678fd-0c19-4b69-974e-fd10fddc50ce
⛔ Files ignored due to path filters (20)
ci-operator/jobs/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-release-0.7-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-release-0.7-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-release-0.12-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-release-0.12-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-release-0.21-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-release-0.21-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/self-node-remediation/medik8s-self-node-remediation-main-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/self-node-remediation/medik8s-self-node-remediation-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/self-node-remediation/medik8s-self-node-remediation-release-0.13-periodics.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/medik8s/self-node-remediation/medik8s-self-node-remediation-release-0.13-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (10)
ci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-main__5.0.yamlci-operator/config/medik8s/fence-agents-remediation/medik8s-fence-agents-remediation-release-0.8__5.0.yamlci-operator/config/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-main__5.0.yamlci-operator/config/medik8s/machine-deletion-remediation/medik8s-machine-deletion-remediation-release-0.7__5.0.yamlci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yamlci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-release-0.12__5.0.yamlci-operator/config/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-main__5.0.yamlci-operator/config/medik8s/node-maintenance-operator/medik8s-node-maintenance-operator-release-0.21__5.0.yamlci-operator/config/medik8s/self-node-remediation/medik8s-self-node-remediation-main__5.0.yamlci-operator/config/medik8s/self-node-remediation/medik8s-self-node-remediation-release-0.13__5.0.yaml
| operator-sdk run bundle -n "$INSTALL_NAMESPACE" quay.io/medik8s/self-node-remediation-operator-bundle:latest | ||
| operator-sdk run bundle -n "$INSTALL_NAMESPACE" "$OO_BUNDLE" | ||
| oc wait --for condition=Available -n "$INSTALL_NAMESPACE" deployment node-healthcheck-controller-manager |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant region plus nearby context.
git ls-files 'ci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml'
wc -l ci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml
sed -n '1,260p' ci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml
# Search for the self-node-remediation bundle usage in related configs.
rg -n 'self-node-remediation-operator-bundle:latest|OO_BUNDLE|node-healthcheck-controller-manager|install-operator' ci-operator/config/medik8s -S
# Find sibling configs that may show intended pattern.
fd -a 'medik8s-.*__5\.0\.yaml' ci-operator/config/medik8sRepository: openshift/release
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the repository contains docs or comments explaining this operator pairing.
rg -n 'self-node-remediation|node-healthcheck|TLS scan|remediation provider|OO_BUNDLE' -S .Repository: openshift/release
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact install step and surrounding test names in the node-healthcheck config.
sed -n '95,125p' ci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml
sed -n '130,220p' ci-operator/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml
# Compare with the matching self-node-remediation config to see whether this is an intentional shared pattern.
sed -n '100,125p' ci-operator/config/medik8s/self-node-remediation/medik8s-self-node-remediation-main__5.0.yaml
sed -n '135,220p' ci-operator/config/medik8s/self-node-remediation/medik8s-self-node-remediation-main__5.0.yamlRepository: openshift/release
Length of output: 8388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the TLS scanner step or related docs explain why node-healthcheck needs self-node-remediation.
rg -n 'self-node-remediation|remediation provider|node-healthcheck-controller-manager|TLS scanner|tls-scanner-run|PQC_CHECK|TLS_13_ENABLE_TLS_ADHERENCE' \
ci-operator/step-registry/medik8s ci-operator/step-registry/tls ci-operator/config/medik8s -S
# Show the tls-scanner-run contract.
sed -n '1,220p' ci-operator/step-registry/tls/scanner/run/tls-scanner-run-ref.yaml
sed -n '1,340p' ci-operator/step-registry/tls/scanner/run/tls-scanner-run-commands.shRepository: openshift/release
Length of output: 50373
Pin the self-node-remediation bundle image The install-operator step in each TLS readiness job still pulls quay.io/medik8s/self-node-remediation-operator-bundle:latest before "$OO_BUNDLE" and waiting on node-healthcheck-controller-manager. Keep the extra install if it is required, but replace :latest with a versioned bundle or digest.
🤖 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/config/medik8s/node-healthcheck-operator/medik8s-node-healthcheck-operator-main__5.0.yaml`
around lines 109 - 111, The install-operator step still uses the unpinned
self-node-remediation bundle image, so update the operator-sdk run bundle
invocation in the install flow to replace
quay.io/medik8s/self-node-remediation-operator-bundle:latest with a versioned
bundle tag or digest. Keep the existing extra install and the subsequent
"$OO_BUNDLE" install plus the oc wait on node-healthcheck-controller-manager,
but make sure the self-node-remediation bundle reference is pinned in the same
step.
|
@weshayutin: all tests passed! 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. |
|
/pj-rehearse |
There was a problem hiding this comment.
- Stagger periodic cron schedules
This PR adds 40 new job definitions (5 operators x 2 branches x 2 scanner types x 2 job types), when 20 of them are periodic with the same cron schedule 0 6 * * 1 (Monday 6:00 UTC).
There is a resource contention risk and cascading failures if the pool cannot provision 20 clusters at once on the medik8s-aws cluster profile.
Consider staggering schedules with 15-minute offsets per operator, e.g.:
- FAR:
0 6 * * 1 - MDR:
15 6 * * 1 - NHC:
30 6 * * 1 - NMO:
45 6 * * 1 - SNR:
0 7 * * 1
- Consider consolidating periodic jobs
Looking at how other OCP teams handle TLS/PQC scanning:
- 9 repos currently use
tls-scanner-run. Most use just 1-2 jobs per repo. - Only OADP matches this PR's 4-job-per-operator pattern.
The SCAN_NAMESPACE parameter supports comma-separated namespaces, and all operators could even share a single namespace (they have distinct CRDs with no conflicts). So all 5 operators can be installed on one cluster and be scanned together.
Proposed consolidation
- Keep 20 per-operator optional presubmits - these need per-operator
$OO_BUNDLEto test PR-specific changes, and release branch coverage matters since that's what ships - Replace 20 periodic jobs with 2 consolidated periodics hosted under
medik8s/system-testsCI config - install all operators on one cluster, scan once (one for pqc-readiness, one for tls13-adherence). Main branch periodics are sufficient since release branches rarely change, and any changes trigger presubmits anyway.
That's 22 jobs instead of 40, and 2 weekly cluster provisions instead of 20.
- Missing operator: SBR
storage-based-remediation is not included in this PR. Should it be added to the per-operator presubmits and the consolidated periodic scan?
- OCP 4.23 coverage?
This PR only targets __5.0.yaml configs. PQC readiness was introduced in OCP 4.20 (hybrid ML-KEM key exchange) and TLS 1.3 Modern profile is relevant to 4.23 as well. Should 4.23 configs also get TLS/PQC scanner jobs?
|
The PR can be closed after #83399 was merged |
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
needs-rebase
|
/close |
|
@JonahSussman: You can't close an active issue/PR unless you authored it or you are a collaborator. 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. |
3 participants
Summary by CodeRabbit
This PR updates CI configs for several medik8s operator repositories to add TLS/PQC validation coverage. In practical terms, it introduces a shared
tls-scanner-toolbase image and wires in new optional jobs that run operator installs followed by TLS 1.3 and PQC scanner steps.Across the affected repos, the new jobs include both ad-hoc and weekly periodic variants for:
These jobs generally:
medik8s-aws/ipi-awstest flow,tls-13andtls-scanner-runreferences with the appropriate environment toggles.This expands CI coverage for RHWA-related operators to verify both TLS policy adherence and PQC readiness before release.