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.

Sign up for GitHub

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

Jump to bottom

feat: add fips-operator-check task #1681

Merged
merged 1 commit into from
Dec 17, 2024
Merged

feat: add fips-operator-check task #1681

merged 1 commit into from
Dec 17, 2024

Conversation

yashvardhannanavati
Copy link
Contributor

Refers to CVP-4333. This task uses the check-payload tool to verify if an operator bundle image is FIPS compliant.It utilizes Tekton stepAction because the code will be reused for checking FBC fragments in the fbc-validation check.

@openshift-ci openshift-ci bot requested review from tisutisu and tkdchen December 3, 2024 06:58
@yashvardhannanavati
Copy link
Contributor Author

@arewm @dirgim Could you please help review this?

@yashvardhannanavati yashvardhannanavati force-pushed the fips_check branch 13 times, most recently from c350bbf to 1be8abc Compare December 5, 2024 15:29
arewm
arewm reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

Nit: There are some long jq lines in here. Is there any way that we can improve those?

I'll have to review the stepAction later.

task/fips-operator-bundle-check/0.1/fips-operator-bundle-check.yaml
capabilities:
add:
- SETFCAP
script: |
Copy link
Member

Choose a reason for hiding this comment

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

This script is doing a lot of work that would be great to unit test. Would you be opposed to putting it in an image (or elsewhere) where we can add tests? Even if it is something like https://github.com/konflux-ci/oras-container/blob/main/.tekton/test.yaml . This would also enable better reusability of functionality like getting all related images.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would moving parts of the script as functions to the konflux-test image utils be acceptable?
The bash functions there can be unit tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 to Kruno's idea. We already have a few functions there which will be used in the new fbc task we will introduce. I'll work on creating those functions

Copy link
Member

@arewm arewm Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I think that would work. I understand that this is a balance between technical debt and doing more of what we have been doing.

I think that there is likely some technical debt of the current validate-fbc task where some of that functionality can be moved to the utils as well. :)

@yashvardhannanavati yashvardhannanavati requested a review from a team as a code owner December 10, 2024 09:13
chmeliik
chmeliik previously approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

CODEOWNERS changes LGTM, for the rest I'll defer to @konflux-ci/integration-service-maintainers

@chmeliik
Copy link
Contributor

/ok-to-test

@yashvardhannanavati yashvardhannanavati mentioned this pull request Dec 10, 2024
Merged
@yashvardhannanavati yashvardhannanavati force-pushed the fips_check branch 3 times, most recently from 5760539 to 82b0df6 Compare December 16, 2024 07:51
@yashvardhannanavati
feat: add fips-operator-check task
c94a207 
Refers to CVP-4333. This task uses the check-payload tool to verify if
an operator bundle image is FIPS compliant.It utilizes Tekton stepAction
because the code will be reused for checking FBC fragments in the
fbc-validation check.

Signed-off-by: Yashvardhan Nanavati <[email protected]>
@yashvardhannanavati
Copy link
Contributor Author

@dirgim @arewm @hongweiliu17 The task image have now been updated and all suggested changes have been made. Can I please request a final review?

dirgim
dirgim approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

LGTM

arewm
arewm approved these changes Dec 17, 2024
View reviewed changes
Copy link
Member

@arewm arewm left a comment

Choose a reason for hiding this comment

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

I didn't fully re-review, but I resolved the lingering thread.

Thanks!

@dirgim
Copy link
Contributor

dirgim commented Dec 17, 2024

/ok-to-test

chmeliik
chmeliik approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM for the CODEOWNERS change

@dirgim dirgim added this pull request to the merge queue Dec 17, 2024
Merged via the queue into konflux-ci:main with commit 90bff99 Dec 17, 2024
16 checks passed
@davdhacs davdhacs mentioned this pull request Jan 21, 2025
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arewm arewm arewm approved these changes

@dirgim dirgim dirgim approved these changes

@chmeliik chmeliik chmeliik approved these changes

@tisutisu tisutisu Awaiting requested review from tisutisu

@tkdchen tkdchen Awaiting requested review from tkdchen

@14rcole 14rcole Awaiting requested review from 14rcole

@kasemAlem kasemAlem Awaiting requested review from kasemAlem

@dheerajodha dheerajodha Awaiting requested review from dheerajodha

@sonam1412 sonam1412 Awaiting requested review from sonam1412

@jsztuka jsztuka Awaiting requested review from jsztuka

@jencull jencull Awaiting requested review from jencull

@chipspeak chipspeak Awaiting requested review from chipspeak

@hongweiliu17 hongweiliu17 Awaiting requested review from hongweiliu17

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yashvardhannanavati @chmeliik @dirgim @arewm @hongweiliu17