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

Add atomic scanner for pip, npm and gem package managers #130

Merged
merged 1 commit into from Jan 12, 2017

Conversation

dharmit
Copy link
Collaborator

@dharmit dharmit commented Dec 22, 2016

No description provided.

@dharmit dharmit force-pushed the misc-package-updates branch 4 times, most recently from 64eb0c3 to 3f9127e Compare December 23, 2016 11:20
@dharmit
Copy link
Collaborator Author

dharmit commented Dec 23, 2016

#dotests

@dharmit dharmit force-pushed the misc-package-updates branch 20 times, most recently from 181bd19 to 6147a21 Compare December 28, 2016 14:09
@dharmit dharmit changed the title [WIP] Add atomic scanner for pip, npm and gem package managers Add atomic scanner for pip, npm and gem package managers Dec 28, 2016
@dharmit
Copy link
Collaborator Author

dharmit commented Dec 28, 2016

In this PR, I've added a new beanstalk_worker/scanner.py which defines a class Scanner. It acts as a base class and can be used by the classes for individual scanners inside beanstalk_worker/worker_start_scan.py. An example of this is the MiscPackageUpdates class which uses the methods of base class to perform atomic scan.

@navidshaikh We can modify ScannerRPMVerify class to use the same as base class and remove duplicate code.

@bamachrn @mohammedzee1000 @navidshaikh need PR review.

Copy link
Collaborator

@bamachrn bamachrn left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,20 @@
type: scanner
scanner_name: misc-package-updates
image_name: registry.centos.org/pipeline-images/misc-package-updates
Copy link
Collaborator

Choose a reason for hiding this comment

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

we are meaning it would be built with :latest tag right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bamachrn yes, it'll be built with latest tag.

@@ -0,0 +1,95 @@
#!/usr/bin/env python
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file is the central scanner right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bamachrn yep, it's the base class that would be inherited by other scanners.

--------------------------------

This is a container image scanner based on `atomic scan`. The goal of the
scanner is to scan CentOS based Docker images in the CentOS Community Container
Copy link
Collaborator

Choose a reason for hiding this comment

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

*CentOS based container images


Steps to use:

- Pull Docker image from **registry.centos.org**:
Copy link
Collaborator

Choose a reason for hiding this comment

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

*Pull container image from registry.centos.org

return response


def list_of_outdated_packages(response):
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstrings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Not sure why GitHub doesn't say "Show outdated".


# remove the container
client.remove_container(container=container.get("Id"), force=True)
except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

log the exception /error message and we should put another error message about failure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Not sure why GitHub doesn't say "Show outdated".

- name: Set SELinux to permissive
selinux:
state: permissive

Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@navidshaikh because we're mounting Docker socket /var/run/docker.sock inside the container that's going to scan the image. Using which a new container is spunn off to check for respective package manager updates.

elif cli_arg == "npm":
exe = client.exec_create(
container=container.get("Id"),
cmd="npm outdated"
Copy link

Choose a reason for hiding this comment

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

I am not sure if this command can do the trick. There can be modules installed globally (npm list -g) and also in npm_modules directories located basically anywhere on the filesystem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@msrb agreed. This one takes care of globally installed npm modules. However, I doubt we can go through all the directories on the filesystem.

Also, what you mention about npm holds true for pip as well. If a dev prefers to use virtualenv inside the container, we are going to face similar issue.

It boils down to whether a dev wants to use npm/pip inside container in such a way that they can have multiple environments or a global environment. Our scanner should handle the latter scenario. Not sure if we can handle the former effectively.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although ideally, putting stuff into containers should eliminate the need of again using a virtualenv as the container itself is a box in its own right.

The same should apply to npm.

Copy link

Choose a reason for hiding this comment

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

@dharmit I am no NPM expert, so don't quote me on this :) I think if you want to check globally installed modules, you need to add -g, i.e.: npm outdated -g. Otherwise you're checking modules installed in ./npm_modules/ directory.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@msrb gotcha. I'm not an npm expert either. @bamachrn any thoughts?

Copy link

Choose a reason for hiding this comment

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

@mohammedzee1000 whatever path you guys will take, just make sure that known limitations are documented. And you'll be fine 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@msrb I hate to admit it but I hadn't looked at the npm_modules dir that gets created whenever you do a npm install <package>@<version>. The -g tip is more than helpful. It's savior because that's what I actually wanted to accomplish. Having worked with pip, I didn't realize npm would create a dir at whatever location you execute npm install if it doesn't have a -g switch.

Thank you!

@dharmit dharmit force-pushed the misc-package-updates branch 9 times, most recently from e6fdb36 to b9979de Compare January 6, 2017 08:34
@navidshaikh
Copy link
Collaborator

#dotests

Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

LGTM

@dharmit
Copy link
Collaborator Author

dharmit commented Jan 9, 2017

#dotests

@dharmit
Copy link
Collaborator Author

dharmit commented Jan 11, 2017

#dotests

@bamachrn bamachrn merged commit f5e4526 into CentOS:master Jan 12, 2017
@dharmit dharmit deleted the misc-package-updates branch January 13, 2017 14:59
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.

None yet

5 participants