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

feat(byon): Create task and pipeline resource for image validation #24

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

mimotej
Copy link
Contributor

@mimotej mimotej commented Feb 1, 2022

Resolves: #21

Tekton task, which runs script inside an image given as parameter. Script checks if there is a installed Python and checks if there are any additional pip packages. If either of these are invalid script prints message and exits with exit code 1. Otherwise Python version and pip packages can be found in results in JSON format.

@sesheta sesheta added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 1, 2022
@sesheta sesheta added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 1, 2022
@sesheta
Copy link
Member

sesheta commented Feb 1, 2022

Hi @mimotej. Thanks for your PR.

I'm waiting for a thoth-station member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 1, 2022
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

As a first pass this is great!

Can you please change it so it outputs to stdout only and with following format:

Validating image:
  Python:      <Not available | 3.9.4>
  Packages: <Not available | list of packages...>
  ...

Comment on lines 43 to 45
PACKAGES=$(pip list --disable-pip-version-check --format json)

if [[ $? != 0 ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Same as above - let's do command -v pip first, instead of checking exit status.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, let's rather ask than reinvent stuff....

@fridex, @codificat, @pacospace do you know about any simple and native way to fetch the explicitly installed in Python packages only? Something that would skip all the nested dependencies so we can get a clean list. E.g. do not show Werkzeug if flask is installed.

I would like to avoid installing additional dependencies here if possible, so we don't compromise/modify the analyzed stack (pip-chill comes to mind). Also we need to be sure it would work with any possible python version since we should be able to analyze any imaginable stack.

Copy link
Contributor Author

@mimotej mimotej Feb 2, 2022

Choose a reason for hiding this comment

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

I was thinking that --not-required option in pip list might do the trick. According to documentation https://pip.pypa.io/en/stable/cli/pip_list/ it is used to list packages that are not dependencies of installed packages. And from my testing (picture below) it seems to do exactly what we want unless there is some catch that I didn't notice. Also I am not sure when was this option added and if it is available in older versions of pip. WDYT @tumido
image

Choose a reason for hiding this comment

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

The problem with this solution is that we cannot identify the index from where the package has been installed (just something to consider, even though we are not worrying about index yet for this first iteration). Moreover it seems you basically want to discard the transitive dependencies right? Should we ask today? maybe the UI can hide the transitive ones unless the admin wants to see them? wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

@pacospace We should definitely discuss this. I don't think you wan't to display transitive deps at all. The same for indexes. Mind this list is used by users on the spawner. I think they are interested in explicit deps only. The argument is - do you want to see this:
image

Or full pip list from the same package:
image

It's a very different story if you want to show user list of 159 packages compared to 2 important ones currently displayed for the same image in the UI.

The --not-required flag looks like a great find @mimotej 👍 That way the list of 159 packages shrinks to 10. Which I think is a good starting point:
image

Choose a reason for hiding this comment

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

@mimotej --not-required is in pip since v9.0.0 (released Nov 2016) https://pip.pypa.io/en/stable/news/#v9-0-0, I think this should be safe to use slightly_smiling_face

we can do a diff between base command and this command maybe? so we don't use other packages?

Copy link
Member

Choose a reason for hiding this comment

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

@pacospace not sure if pipenv or thamos is the right answer. We can't assume either is installed and our validation should not fail due to them being unavailable. 🙁 Is there a binary/python version independent version of either libs that we can simply mount to the image or something?

Choose a reason for hiding this comment

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

@pacospace not sure if pipenv or thamos is the right answer. We can't assume either is installed and our validation should not fail due to them being unavailable. slightly_frowning_face Is there a binary/python version independent version of either libs that we can simply mount to the image or something?

we will need to think about indexes also as pip list won't tell you from where the package came from.

cc @fridex for more info

Copy link

Choose a reason for hiding this comment

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

We have this thread https://discuss.python.org/t/pip-installation-reports/12316 and thoth-station/micropipenv#206 I think this will be sooner-or-later implemented upstream. I hadn't time to push any implementation, but it looks like a feature that is desperately lacking in the Python community.

Copy link

Choose a reason for hiding this comment

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

E.g. do not show Werkzeug if flask is installed.

Are these images built by AICoE-CI? AICoE-CI puts information about direct dependencies to /opt, you can eventually read that file. When it comes to pip, I don't know about any option that could be helpful in this case (if I understand your case correctly).


# Save existing Python packages
echo "Checking for existing Python packages"
PACKAGES=$(pip list --disable-pip-version-check --format json)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we want to list all the packages here.. is there a way to recognize which packages are explicitly installed and which are dependency packages?

@mimotej mimotej force-pushed the task-image-validation branch from 7d34af5 to 15d7e30 Compare February 2, 2022 13:34
@mimotej mimotej requested a review from tumido February 2, 2022 13:37
@mimotej mimotej force-pushed the task-image-validation branch 2 times, most recently from ab8f8d3 to c22d65c Compare February 2, 2022 13:49
@mimotej mimotej marked this pull request as ready for review February 7, 2022 06:59
@sesheta sesheta removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2022
@codificat
Copy link
Member

/ok-to-test

@sesheta sesheta added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 7, 2022
@mimotej mimotej force-pushed the task-image-validation branch from c22d65c to 624e025 Compare February 7, 2022 12:28
Copy link
Member

@codificat codificat left a comment

Choose a reason for hiding this comment

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

The way this repo works for now, you will have to increase the chart version in Chart.yaml for any change that touches charts/*.

Besides that, I only have a question about privileged

Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I think nested echo $(echo) is not required.

Also recommendation messages should be provided in format:

{"severity":"X","message":"Y"}
{"severity":"X","message":"Y"}

Instead of the current (in PR):

[{"severity":"X","message":"Y"}]
[{"severity":"X","message":"Y"}]

@mimotej mimotej force-pushed the task-image-validation branch from 624e025 to 3c7f095 Compare February 7, 2022 14:39
@mimotej mimotej requested review from tumido and codificat February 7, 2022 14:45
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2022
@sesheta
Copy link
Member

sesheta commented Feb 7, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tumido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BYON] Create a Tekton task for image validation
6 participants