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

WIP: Check for inherited optional labels #213

Closed
wants to merge 2 commits into from
Closed

WIP: Check for inherited optional labels #213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6633c99
Create new image+dockerfile target and check type
twaugh Nov 30, 2018
7c337cf
WIP: Inherited optional labels check
twaugh Dec 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions colin/checks/labels.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
#

from colin.core.checks.abstract_check import ImageDockerfileAbstractCheck
from colin.core.checks.labels import LabelAbstractCheck
from colin.core.checks.fmf_check import FMFAbstractCheck

Expand Down Expand Up @@ -106,3 +107,10 @@ class VendorLabelCheck(FMFAbstractCheck, LabelAbstractCheck):

class VersionLabelCheck(FMFAbstractCheck, LabelAbstractCheck):
name = "version_label"


class InheritedOptionalLabelCheck(ImageDockerfileAbstractCheck):
name = "inherited_labels"
# TODO: Compare image labels and labels declared in Dockerfile
# to compute inherited labels; filter by labels which are optional;
# the remaining set should be empty.
12 changes: 9 additions & 3 deletions colin/cli/colin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ def cli():
@click.command(name="check",
context_settings=CONTEXT_SETTINGS)
@click.argument('target', type=click.STRING)
@click.option('--supplementary-target', type=click.STRING,
Copy link
Member

Choose a reason for hiding this comment

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

Internally the input for colin is an image. I wonder how do we discover the specific dockerfile.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, is colin not shown the Dockerfile used to build the image?

I wonder if this check is a good fit for colin at all. I saw the 'dockerfile' target and assumed it was something it was also told.

Copy link
Member

Choose a reason for hiding this comment

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

Internally, colin testing is initiated when an osbs build completes successfully. The input is only the name of the image (within the registry), colin then fetches it to ostree repo and starts the analysis.

I think it's feasiable to also link to the specific dockerfile which was used to produce the image -- we can still get the dockerfile from inside of the image, right?

Copy link
Author

Choose a reason for hiding this comment

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

Only for OSBS-built images. Is that OK?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, unless @lachmanfrantisek objects. We can easily implement that check that it would only apply to OSBS' built images, otherwise it would fail with some reasonable error message.

Copy link
Author

Choose a reason for hiding this comment

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

The absolutely ideal input would be the output of skopeo inspect $fq_name_parent_image. Then I can just look at .Labels and compare directly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with either way and from unit test POV it might be simpler to have output of skopeo inspect But on the other hand could hide network operations.

Choose whatever you/Tomas like more. And I'll provide it in CVP :-)

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 also fine with either way; I'm just slightly concerned with passing json via CLI or env vars, that's it. But since openshift does the same thing and apparently it works fine, let's pass labels directly.

Copy link
Contributor

@rcerven rcerven May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, I am not sure how general we want this new check to be.
So we can either pass directly inspect json from parent OR

we can basically pass secondary_target (optional), maybe even secondary_target_type (or we can use same target_type for secondary_target)

that way we can use Target class also for parent target

and then we will just simply use those 2 targets for that inherited optional labels check

what do you think ? @TomasTomecek @lachmanfrantisek @lslebodn @twaugh

Copy link
Contributor

@rcerven rcerven May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

just realized that, when we use target_type dockerfile, this check doesn't make sense, so in that case we would either ignore that secondary_target or forbid it with dockerfile type

help="Additional target. For image+dockerfile target type "
"this is the Dockerfile.")
@click.option('--ruleset', '-r', type=click.STRING, envvar='COLIN_RULESET',
help="Select a predefined ruleset (e.g. fedora).")
@click.option('--ruleset-file', '-f', type=click.File(mode='r'),
Expand All @@ -75,13 +78,15 @@ def cli():
help="Pull the image from registry.")
@click.option('--target-type', type=click.STRING, default="image",
help="Type of selected target (one of image, dockerfile, "
"ostree). For ostree, please specify image name and path like this: image@path")
"image+dockerflie, ostree). For ostree, please specify "
"image name and path like this: image@path")
@click.option('--timeout', type=click.INT,
help="Timeout for each check in seconds. (default=600)")
@click.option('--insecure', is_flag=True, default=False,
help="Pull from an insecure registry (HTTP or invalid TLS).")
def check(target, ruleset, ruleset_file, debug, json, stat, skip, tag, verbose,
checks_paths, target_type, timeout, pull, insecure):
def check(target, supplementary_target, ruleset, ruleset_file, debug, json,
stat, skip, tag, verbose, checks_paths, target_type, timeout, pull,
insecure):
"""
Check the image/dockerfile (default).
"""
Expand All @@ -101,6 +106,7 @@ def check(target, ruleset, ruleset_file, debug, json, stat, skip, tag, verbose,
verbose=verbose)
results = run(
target=target,
supplementary_target=supplementary_target,
ruleset_name=ruleset,
ruleset_file=ruleset_file,
logging_level=log_level,
Expand Down
5 changes: 5 additions & 0 deletions colin/core/checks/abstract_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,10 @@ class ImageAbstractCheck(AbstractCheck):
check_type = "image"


class ImageDockerfileAbstractCheck(DockerfileAbstractCheck,
ImageAbstractCheck):
check_type = "image+dockerfile"


class FilesystemAbstractCheck(AbstractCheck):
pass
3 changes: 3 additions & 0 deletions colin/core/colin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ def run(
insecure=False,
skips=None,
timeout=None,
supplementary_target=None,
):
"""
Runs the sanity checks for the target.
Expand All @@ -54,12 +55,14 @@ def run(
:param checks_paths: list of str, directories where the checks are present
:param pull: bool, pull the image from registry
:param insecure: bool, pull from an insecure registry (HTTP/invalid TLS)
:param supplementary_target: str, additional target (for image+dockerfile)
:return: Results instance
"""
_set_logging(level=logging_level)
logger.debug("Checking started.")
target = Target.get_instance(
target=target,
supplementary_target=supplementary_target,
logging_level=logging_level,
pull=pull,
target_type=target_type,
Expand Down
23 changes: 22 additions & 1 deletion colin/core/target.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@

from dockerfile_parse import DockerfileParser

from .checks.abstract_check import ImageAbstractCheck, DockerfileAbstractCheck
from .checks.abstract_check import (ImageAbstractCheck,
DockerfileAbstractCheck,
ImageDockerfileAbstractCheck)
from ..core.exceptions import ColinException
from ..utils.cont import ImageName

Expand Down Expand Up @@ -313,6 +315,24 @@ def get_output(self, cmd):
raise NotImplementedError("Unsupported right now.")


class ImageWithDockerfileTarget(ImageTarget, DockerfileTarget):
"""
Represents the podman image, along with the Dockerfile it was
built from, as a target.
"""
target_type = "image+dockerfile"

def __init__(self, target, pull, supplementary_target, insecure=False,
**kwargs):
super(ImageTarget, self).__init__(target, pull, insecure=insecure,
**kwargs)
super(DockerfileTarget, self).__init__(supplementary_target)

@classmethod
def get_compatible_check_class(cls):
return ImageDockerfileAbstractCheck


class OstreeTarget(AbstractImageTarget):
"""
Represents the ostree repository as an image target.
Expand Down Expand Up @@ -430,5 +450,6 @@ def get_output(self, cmd):
TARGET_TYPES = {
"image": ImageTarget,
"dockerfile": DockerfileTarget,
"image+dockerfile": ImageWithDockerfileTarget,
"ostree": OstreeTarget
}