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

Fixes bugs in scanners #626

Merged
merged 5 commits into from Sep 14, 2018

Conversation

Projects
None yet
4 participants
@navidshaikh
Collaborator

navidshaikh commented Sep 11, 2018

This changeset fixes bugs in scanners.

  1. Uses yum python API client library to list yum updates

    Stops using system commands and parsing stdout, now uses
    yum python API client library to list and print the updates
    in cleaner way.

    Uses a Suppressor for not printing the yum repo updates stdout
    before printing results.

    Adds ability to list installed version of package, for which
    updates are available.
    Adds ability to list the source yum repo.

  2. Adds error checking in rpmverify scanner

    Adds missing sys import
    Adds formating indexes to be compatible for python version in
    centos6 images.

  3. Fixes string formatting in misc and capabilities scanner

    Fixes string formatting
    Uses explicit indexes in string formatting for python version
    in centos6 image

  4. Uses /usr/bin/python as entrypoint to stay compatible with centos6 images

    Since python is installed at /usr/bin/python in CentOS6 image,
    and earlier we were using /bin/python , which is unavailable in
    CentOS6 image.
    CentOS7 image has needed symlinks to stay compatible with either
    /usr/bin/python or /usr/bin/python.

@@ -46,7 +46,7 @@ def check_args(cmd):
if not found_sec_arg:
print("\nThis container uses privileged "
"security switches:")
print("\nINFO: {}\n\t{}".format(sec_arg, security_args[sec_arg]))
print("\nINFO: {0}\n\t{1}".format(sec_arg, security_args[sec_arg]))

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

The python version available in CentOS6 image does not understanding {} {}, one needs to provide explicit ordering.

When I fixed a scanner, I tested the fixes and all other scanners with CentOS6 and CentOS7 images to be sure it works as expected, I found this issue. We didn't hit this issue in testing because the execution failed before code could reach until here, the execution failed at the very start for python's absolute path.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

The python version available in CentOS6 image does not understanding {} {}, one needs to provide explicit ordering.

When I fixed a scanner, I tested the fixes and all other scanners with CentOS6 and CentOS7 images to be sure it works as expected, I found this issue. We didn't hit this issue in testing because the execution failed before code could reach until here, the execution failed at the very start for python's absolute path.

def yum_updates():
class SysStdoutSuppressor(object):

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

When we were doing yum updates checking we're providing -q option to be quiet for yum repo updates download stdout. This is same implementation in python. We don't want to print anything other than the updates available, helps the logs fetching to do less processing.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

When we were doing yum updates checking we're providing -q option to be quiet for yum repo updates download stdout. This is same implementation in python. We don't want to print anything other than the updates available, helps the logs fetching to do less processing.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

If we don't do this, the python processing for finding updates will print on stdout.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

If we don't do this, the python processing for finding updates will print on stdout.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

Basically, without this, its yum check-update

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

Basically, without this, its yum check-update

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

https://www.python.org/dev/peps/pep-0343/ this should help review this.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

https://www.python.org/dev/peps/pep-0343/ this should help review this.

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Contributor

👍 Thanks for the extensive explanation. I think adding these comments as comment in the code will help existing and new engineers understand what this piece does. At least the pep link would be very helpful. To me this part on the pep link helped clear any doubt I had:

In this PEP, context managers provide __enter__() and __exit__() methods that are invoked on entry to and exit from the body of the with statement.

@dharmit

dharmit Sep 12, 2018

Contributor

👍 Thanks for the extensive explanation. I think adding these comments as comment in the code will help existing and new engineers understand what this piece does. At least the pep link would be very helpful. To me this part on the pep link helped clear any doubt I had:

In this PEP, context managers provide __enter__() and __exit__() methods that are invoked on entry to and exit from the body of the with statement.

This comment has been minimized.

@navidshaikh

navidshaikh Sep 12, 2018

Collaborator

ok.

@navidshaikh

navidshaikh Sep 12, 2018

Collaborator

ok.

y = YumUpdates()
# prevent repo update downloads stdout
with SysStdoutSuppressor():
updates = y.find_updates()

This comment has been minimized.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

The suppressor class is used here, we dont want to print extra output while fetching available updates.

@navidshaikh

navidshaikh Sep 11, 2018

Collaborator

The suppressor class is used here, we dont want to print extra output while fetching available updates.

navidshaikh added some commits Sep 11, 2018

Uses yum python API client library to list yum updates
  Stops using system commands and parsing stdout, now uses
  yum python API client library to list and print the updates
  in cleaner way.

 Uses a Suppressor for not printing the yum repo updates stdout
 before printing results.

 Adds ability to list installed version of package, for which
 updates are available.
 Adds ability to list the source yum repo.
Adds error checking in rpmverify scanner
 Adds missing sys import
 Adds formating indexes to be compatible for python version in
 centos6 images.
Fixes string formatting in misc and capabilities scanner
  Fixes string formatting
  Uses explicit indexes in string formatting for python version
  in centos6 image
Uses /usr/bin/python as entrypoint to stay compatible with centos6 im…
…ages

  Since python is installed at /usr/bin/python in CentOS6 image,
  and earlier we were using /bin/python , which is unavailable in
  CentOS6 image.
  CentOS7 image has needed symlinks to stay compatible with either
  /usr/bin/python or /usr/bin/python.
@dharmit

Minor changes requested. Looks good otherwise. 👍

Show outdated Hide outdated Dockerfiles/ccp-openshift-scan/scanning/yumupdates.py
def yum_updates():
class SysStdoutSuppressor(object):

This comment has been minimized.

@dharmit

dharmit Sep 12, 2018

Contributor

👍 Thanks for the extensive explanation. I think adding these comments as comment in the code will help existing and new engineers understand what this piece does. At least the pep link would be very helpful. To me this part on the pep link helped clear any doubt I had:

In this PEP, context managers provide __enter__() and __exit__() methods that are invoked on entry to and exit from the body of the with statement.

@dharmit

dharmit Sep 12, 2018

Contributor

👍 Thanks for the extensive explanation. I think adding these comments as comment in the code will help existing and new engineers understand what this piece does. At least the pep link would be very helpful. To me this part on the pep link helped clear any doubt I had:

In this PEP, context managers provide __enter__() and __exit__() methods that are invoked on entry to and exit from the body of the with statement.

Show outdated Hide outdated Dockerfiles/ccp-openshift-scan/scanning/yumupdates.py
Show outdated Hide outdated Dockerfiles/ccp-openshift-scan/scanning/yumupdates.py
Updates stdout messages and docstrings of methods
 - Also removes an unnecessary variable
@dharmit

This comment has been minimized.

Show comment
Hide comment
@dharmit

dharmit Sep 12, 2018

Contributor

LGTM, waiting for CI.

Contributor

dharmit commented Sep 12, 2018

LGTM, waiting for CI.

@mohammedzee1000

LGTM, merging

@mohammedzee1000 mohammedzee1000 merged commit ba5a56c into CentOS:openshift Sep 14, 2018

2 checks passed

centos-ci functional tests centos-ci functional tests succeeded
Details
centos-ci unit tests centos-ci unit tests succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment