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

Implement GitHub Actions workflow for scanning Docker images (Ref: #301) #317

Merged
merged 1 commit into from
Nov 20, 2024

Conversation

Hawazyn
Copy link
Contributor

@Hawazyn Hawazyn commented Nov 20, 2024

This pull request enhances the GitHub Actions workflow to streamline Docker image scanning, addressing issue #301.

Key Updates

  • Matrix Strategy: Utilizes the matrix strategy to specify folders containing Docker images for scanning.
  • Artifact Export: Generates reports for each Dockerfile.
  • Execution: Runs only when triggered manually via workflow_dispatch.

Successfully tested on a forked repository.

Reference: #301

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Hayyaaf ! LGTM -- and I like the approach of being able to run this only manually: Nice to the environment, not running these scan-builds always/unnecessarily. Might it a good improvement to also run it on push-to-main as a safeguard (either in this job or as an additional step in the main build-and-push jobs we already have)?

Signed-off-by: Khalid <187553667+Hayyaaf@users.noreply.github.com>

Add push-to-main trigger
@Hawazyn
Copy link
Contributor Author

Hawazyn commented Nov 20, 2024

Thanks for the feedback, @baentsch. I have updated the workflow in the same file to include a trigger for push-to-main. Let me know if there’s anything else you’d like adjusted.

@baentsch baentsch merged commit b04c59e into open-quantum-safe:main Nov 20, 2024
2 checks passed
@baentsch
Copy link
Member

Thanks for the feedback, @baentsch. I have updated the workflow in the same file to include a trigger for push-to-main. Let me know if there’s anything else you’d like adjusted.

Nope -- that's OK for the time being. I'd like to see the new workflow become available and we can then improve in followup PRs, e.g. while doing #294, adding this scan logic at the end of the build(s) to avoid CI run duplication. Thanks for this contribution!

for FILE in $FILES; do
IMAGE_NAME="${{ matrix.folder }}-$(basename $FILE | tr '[:upper:]' '[:lower:]' | tr -cd '[:alnum:]-')"
echo "Building Docker image: $IMAGE_NAME using $FILE"
docker build -t $IMAGE_NAME -f $FILE ./${{ matrix.folder }}
Copy link
Member

Choose a reason for hiding this comment

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

This logic has a flaw @Hawazyn : If finding a Dockerfile in a subdirectory (such as nginx/fulltest), the subsequent docker buildcommand does not switch CWD to that subdirectory, leading to files not being found. IMO this is the reason for the persistent CI failure, e.g., here. Please let me know whether you'd provide a PR to fix for this or whether I should do to get CI status back to green.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @baentsch, I’ll look into this issue today and provide a fix PR.

Copy link
Member

Choose a reason for hiding this comment

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

On closer look, I'm wondering whether this can be improved in general: Am I right thinking that this job duplicates the tasks in the other CI workflows (building images)? Wouldn't it be better in general to only add a scan step to each separate image build as and when done anyway? This would be much less resource intensive. OK for you to change things this way? This would also do away with this separate file entirely (and thus automatically solve the problem above): OK for you @Hawazyn ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with combining the scan with image builds for efficiency. Should I proceed to apply the changes and remove the docker-scan file?

Copy link
Member

Choose a reason for hiding this comment

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

If you have time for that, that would be welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll work on this, but I kindly ask for some additional time as I am currently managing other priorities, in addition to work related to Docker files and their documentation. I truly appreciate your patience and understanding, and I will keep you updated as I make progress.

Copy link
Member

Choose a reason for hiding this comment

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

No urgency -- would you mind if I did it to save you the hassle and let you focus on what you're already doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for offering to help! I can see you already have a great deal to manage at the moment, and while I am perfectly happy to continue with this, I do not wish to add to your workload unnecessarily. My tasks are quite manageable, so please let me know how you would prefer to proceed. I am happy to adjust to whatever suits you best.

Copy link
Contributor Author

@Hawazyn Hawazyn Dec 27, 2024

Choose a reason for hiding this comment

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

I sincerely apologize for being unable to address the workflow scanning integration as planned due to unforeseen circumstances. Unfortunately, I will be unavailable for the next month and unable to take on new tasks or make additional changes during this time. However, I am committed to finalizing PR #338 to ensure the current work is completed before my absence.

If there are further issues or adjustments needed, I kindly request your assistance to ensure a smooth resolution. I deeply appreciate your understanding and support.

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