MGMT-22814: day 2 sync of clair vulnerability data (import execution in tekton)#28
Merged
MGMT-22814: day 2 sync of clair vulnerability data (import execution in tekton)#28
Conversation
|
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:
WalkthroughAdds a Tekton-based Clair import workflow (ServiceAccount, Task, Pipeline), switches Quay operator disconnected flow to RBAC (Role/RoleBinding) instead of in-pod exec, registers the pipeline via an Ansible task, and invokes the new play from sync.sh. Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as sync.sh
participant Ansible as Ansible Playbook
participant RBAC as Kubernetes RBAC
participant Tekton as Tekton API
participant Clair as Clair Pod
Sync->>Ansible: run playbook --tags clair-disconnected
Ansible->>RBAC: create Role & RoleBinding (clair-import)
Ansible->>Tekton: apply ServiceAccount, Task, Pipeline
Tekton->>Clair: schedule Task / exec clairctl (via pods/exec)
Clair-->>Tekton: command output & exit code
Tekton-->>Ansible: task/pipeline results
Ansible-->>Sync: playbook completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
operators/openshift-pipelines-operator-rh/tasks.yaml (1)
18-20: Consider gating Clair pipeline creation behind a feature/disconnected flag.This include is unconditional; adding a
when:guard (for the Clair-disconnected use case) would avoid provisioning unused pipeline assets in clusters that don’t need this flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/openshift-pipelines-operator-rh/tasks.yaml` around lines 18 - 20, The "Create Clair Import Pipeline" task currently includes clair_import_pipeline.yaml unconditionally; add a conditional guard so the include only runs when the pipeline should be provisioned, e.g. add a when: clause to that task using variables like clair_pipeline_enabled and clair_disconnected (example logic: when: (clair_pipeline_enabled | default(false)) and not (clair_disconnected | default(false))), referencing the task name "Create Clair Import Pipeline" and the include_tasks file "clair_import_pipeline.yaml" so callers can toggle provisioning via those vars.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operators/openshift-pipelines-operator-rh/clair_import_pipeline.yaml`:
- Around line 44-50: The step captures the `oc exec` exit code to
$(results.exit-code.path) but the shell then exits with 0, masking failures;
update the inline shell block that runs `oc exec` (where `clair_pod_name` is
computed and the commands download and run `/usr/bin/clairctl`) to capture the
command exit status (e.g., rc=$?) immediately after `oc exec`, write rc to
$(results.exit-code.path) and then `exit $rc` so the step process exits with the
same status; ensure the write to $(results.status-report.path) still occurs
before exiting and that the `exit $rc` happens inside the same quoted shell
fragment so Tekton receives the proper non-zero exit code when the import fails.
- Around line 62-70: The Pipeline defines task-level "results" under the
clair-import PipelineTask, which is invalid; move those result declarations to
the Pipeline's top-level spec.results and keep the references to the task
outputs where needed (use the task output syntax
$(tasks.clair-import.results.exit-code) and
$(tasks.clair-import.results.status-report)); specifically remove the results
block from the tasks entry for name "clair-import" and add corresponding entries
under spec.results with names "exit-code" and "status-report", then update any
downstream references to use $(tasks.clair-import.results.<name>) as needed.
In `@operators/quay-operator/clair_disconnected.yaml`:
- Around line 62-74: The Role definition for clair-import grants an invalid verb
"exec" on pods; update the Role named "clair-import" so the rule targets the
pods/exec subresource (resource "pods/exec") and uses the "create" verb instead
of "exec" so oc exec is authorized (leave other fields like apiVersion/namespace
unchanged).
---
Nitpick comments:
In `@operators/openshift-pipelines-operator-rh/tasks.yaml`:
- Around line 18-20: The "Create Clair Import Pipeline" task currently includes
clair_import_pipeline.yaml unconditionally; add a conditional guard so the
include only runs when the pipeline should be provisioned, e.g. add a when:
clause to that task using variables like clair_pipeline_enabled and
clair_disconnected (example logic: when: (clair_pipeline_enabled |
default(false)) and not (clair_disconnected | default(false))), referencing the
task name "Create Clair Import Pipeline" and the include_tasks file
"clair_import_pipeline.yaml" so callers can toggle provisioning via those vars.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a459e5bf-a008-419a-a0c3-a437e4f0cb96
📒 Files selected for processing (4)
operators/openshift-pipelines-operator-rh/clair_import_pipeline.yamloperators/openshift-pipelines-operator-rh/tasks.yamloperators/quay-operator/clair_disconnected.yamlsync.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operators/quay-operator/clair_disconnected.yaml`:
- Around line 89-91: The Pipeline and Task in clair_import_pipeline.yaml are
missing serviceAccountName, causing RBAC failures with the clair-import
ServiceAccount bound in the RoleBinding; update the Pipeline spec and the Task
spec to include serviceAccountName: clair-import (or alternatively ensure every
PipelineRun invoking this Pipeline sets serviceAccountName: clair-import) so the
pipeline steps run with the clair-import ServiceAccount rather than the
namespace default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35be5344-0af3-4cf0-a894-17c911f5dedd
📒 Files selected for processing (2)
operators/openshift-pipelines-operator-rh/clair_import_pipeline.yamloperators/quay-operator/clair_disconnected.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- operators/openshift-pipelines-operator-rh/clair_import_pipeline.yaml
maorfr
force-pushed
the
sync-clair
branch
2 times, most recently
from
0ecc679 to
5a30d30
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
operators/quay-operator/clair_disconnected.yaml (1)
79-95: Treat this binding as privileged.This gives
openshift-pipelines/clair-importexec access intoquay-enterprise. Make sure only the day-2 automation can create or modify PipelineRuns/TaskRuns that use this service account, and audit that path if you don’t already; otherwise this becomes a broader cross-namespace shell path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/quay-operator/clair_disconnected.yaml` around lines 79 - 95, The RoleBinding granting openshift-pipelines/clair-import access to quay-enterprise (RoleBinding name clair-import, Role clair-import, ServiceAccount openshift-pipelines/clair-import, target namespace quay-enterprise) must be treated as privileged: mark the resource (e.g., add a metadata annotation like security.kubernetes.io/privileged="true"), and lock down who can create/modify PipelineRuns/TaskRuns that reference this ServiceAccount by restricting RBAC in the openshift-pipelines namespace to only the day-2 automation principals (or a dedicated Role/RoleBinding), and ensure audit logging/alerts are enabled for creation/modification of PipelineRun/TaskRun objects that reference this SA.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@operators/quay-operator/clair_disconnected.yaml`:
- Around line 79-95: The RoleBinding granting openshift-pipelines/clair-import
access to quay-enterprise (RoleBinding name clair-import, Role clair-import,
ServiceAccount openshift-pipelines/clair-import, target namespace
quay-enterprise) must be treated as privileged: mark the resource (e.g., add a
metadata annotation like security.kubernetes.io/privileged="true"), and lock
down who can create/modify PipelineRuns/TaskRuns that reference this
ServiceAccount by restricting RBAC in the openshift-pipelines namespace to only
the day-2 automation principals (or a dedicated Role/RoleBinding), and ensure
audit logging/alerts are enabled for creation/modification of
PipelineRun/TaskRun objects that reference this SA.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98de471b-622e-4480-a06b-7c93e98d0887
📒 Files selected for processing (4)
operators/openshift-pipelines-operator-rh/clair_import_pipeline.yamloperators/openshift-pipelines-operator-rh/tasks.yamloperators/quay-operator/clair_disconnected.yamlsync.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- sync.sh
- operators/openshift-pipelines-operator-rh/clair_import_pipeline.yaml
maorfr
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
part of https://issues.redhat.com/browse/MGMT-22814
related to https://github.com/gori-project/GoRI/issues/799
keep vulnerability export in LZ as part of bootstrap. in this PR we also add this to sync, which is an activity performed prior to content update, such as mirroring.
we are also adding a tekton pipeline to perform the vulnerability data import within the management cluster instead of as part of bootstrap. this is to enable multiple executions of this operations in case of failure.
the import operation is also more fragile, and removing it from the e2e bootstrap makes the initial bootstrap more resilient.
Summary by CodeRabbit
New Features
Chores