Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ROX-20750: Onboard scanner-db-slim to Konflux #1436

Conversation

tommartensen
Copy link
Contributor

@tommartensen tommartensen commented Mar 7, 2024

How are scanner-db-slim pipelines different from scanner-db?

-> scanner-db includes SQL definitions, scanner-db-slim ships without
-> TARGET_STAGE points to the respective stage in the Dockerfile

Multi-stage Docker build:

We have a scanner-db-common stage.
The final scanner-db-slim is just this plus updated labels and ROX_SLIM_MODE env var.
The full scanner-db, in addition to scanner-common stage, copies the SQL definitions to the container image (and has slightly different labels).

Validation

Looking at the images and confirming that slim does not have SQL definitions.

  • docker run -it --entrypoint /bin/sh quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db-slim:on-pr-679696d44285c6dc3d20c72ebdd79ddabffacf99
  • docker run -it --entrypoint /bin/sh quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db:on-pr-679696d44285c6dc3d20c72ebdd79ddabffacf99

Do the new pipelines match?

$ diff .tekton/scanner-db-slim-push.yaml .tekton/scanner-db-slim-pull-request.yaml 
7a8
>     build.appstudio.redhat.com/pull_request_number: '{{pull_request_number}}'
10c11,12
<     pipelinesascode.tekton.dev/on-cel-expression: event == "push" && target_branch == "master"
---
>     # TODO(ROX-21073): re-enable for all PR branches
>     pipelinesascode.tekton.dev/on-cel-expression: event == "pull_request" && (source_branch.contains("konflux") || source_branch.contains("rhtap"))
16c18
<   name: scanner-db-slim-on-push
---
>   name: scanner-db-slim-on-pull-request
29c31
<       value: quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db-slim:{{revision}}
---
>       value: quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db-slim:on-pr-{{revision}}

Do the slim and full pipelines match?

$ diff .tekton/scanner-db-slim-push.yaml .tekton/scanner-db-push.yaml
14c14
<     appstudio.openshift.io/component: scanner-db-slim
---
>     appstudio.openshift.io/component: scanner-db
16c16
<   name: scanner-db-slim-on-push
---
>   name: scanner-db-on-push
29c29
<       value: quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db-slim:{{revision}}
---
>       value: quay.io/redhat-user-workloads/rh-acs-tenant/acs/scanner-db:{{revision}}
39c39
<     # No language dependencies are required for scanner-db-slim image.
---
>     # No language dependencies are required for scanner-db image.
195c195
<           # A shallow repo clone is sufficient for scanner-db-slim build.
---
>           # A shallow repo clone is sufficient for scanner-db build.
239a240,255
>       - name: fetch-sql-definitions
>         runAfter:
>           - clone-repository
>         taskSpec:
>           steps:
>             - name: fetch-sql-definitions
>               image: registry.access.redhat.com/ubi8/ubi-minimal:latest
>               script: |
>                 "$(workspaces.source.path)/source/scripts/konflux/fetch-scanner-data.sh" \
>                     "$(workspaces.source.path)/source" \
>                     pg-definitions.sql.gz
>               timeout: '10m'
>         workspaces:
>           - name: source
>             workspace: workspace
> 
257c273
<             value: scanner-db-slim
---
>             value: scanner-db
259a276
>           - fetch-sql-definitions

TODO:

Copy link

openshift-ci bot commented Mar 7, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

io.k8s.display-name="scanner-db" \
name="rhacs-scanner-db-rhel8"

COPY blob-pg-definitions.sql.gz \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that /docker-entrypoint-initdb.d/definitions.sql.gz is now owned by 70:70 (postgres:postgres), the user that executes the container.
It was owned root before, read allowed for all users.

@tommartensen tommartensen marked this pull request as ready for review March 7, 2024 13:28
@tommartensen tommartensen requested a review from a team as a code owner March 7, 2024 13:28
@tommartensen
Copy link
Contributor Author

Konflux EC fails because of scanner-v4, unrelated to the changes in this PR.

Copy link
Contributor

@msugakov msugakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a few cosmetic notes

image/db/rhel/konflux.Dockerfile Outdated Show resolved Hide resolved
image/db/rhel/konflux.Dockerfile Outdated Show resolved Hide resolved
image/db/rhel/konflux.Dockerfile Outdated Show resolved Hide resolved
Comment on lines +64 to +65
COPY blob-pg-definitions.sql.gz \
/docker-entrypoint-initdb.d/definitions.sql.gz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you can keep root ownership by doing this:

Suggested change
COPY blob-pg-definitions.sql.gz \
/docker-entrypoint-initdb.d/definitions.sql.gz
USER 0:0
COPY blob-pg-definitions.sql.gz \
/docker-entrypoint-initdb.d/definitions.sql.gz
USER 70:70

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary though or was root ownership just an artifact of the midstream Dockerfile?

Comment on lines +274 to +275
- name: TARGET_STAGE
value: scanner-db
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this isn't necessary since Dockerfile will be built until the final stage by default. Although I'm happy if you keep this for explicitness.

Comment on lines 257 to 258
- name: TARGET_STAGE
value: scanner-db-slim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After diffing normal and slim pipelines, a thought crossed my mind. It feels a bit wrong that this essential setting which actually differentiates slim and non-slim builds is hidden somewhere in a middle of the file.

How would you feel about introducing a top-level param (under spec.params) for this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tommartensen and others added 2 commits March 12, 2024 08:45
Co-authored-by: Misha Sugakov <537715+msugakov@users.noreply.github.com>
@tommartensen tommartensen merged commit 84ee52c into tm/scanner-slim-konflux-onboarding Mar 12, 2024
19 of 20 checks passed
@tommartensen tommartensen deleted the tm/tm/scanner-db-slim-konflux-onboarding branch March 12, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants