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: image checker extension API #4662

Merged
merged 15 commits into from Nov 17, 2023

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Nov 6, 2023

What does this PR do?

This PR defines and implements the API for Image Checker providers.

An extension registers as an Image checker, by calling the following function, passing a Provider object and optional metadata:

  registerImageCheckerProvider(
      imageCheckerProvider: ImageCheckerProvider,
      metadata?: ImageCheckerProviderMetadata,
    )

The provider object needs to define a check method, which will be called by the UI:

  export interface ImageCheckerProvider {
    check(image: ImageInfo, token?: CancellationToken): ProviderResult<ImageChecks>;
  }

The check method returns a list of image checks results.

  export interface ImageCheck {
    name: string;
    status: 'success' | 'failed';
    severity?: 'low' | 'medium' | 'high' | 'critical';
    markdownDescription?: string;
  }

  export interface ImageChecks {
    checks: ImageCheck[];
  }

What issues does this PR fix or reference?

Fixes #4697

@feloy feloy requested review from benoitf and a team as code owners November 6, 2023 15:48
@feloy feloy requested review from dgolovin and cdrage and removed request for a team November 6, 2023 15:48
@feloy feloy marked this pull request as draft November 6, 2023 15:48
@feloy feloy mentioned this pull request Nov 6, 2023
2 tasks
Comment on lines 2503 to 2505
readonly name: string;
readonly categories: string[];
checkImage(image: string): Promise<ImageCheckResult>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK the provider goal is to provide only functions so I don't see name and categories there ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think checkImage is missing a cancellationToken: CancellationToken parameter to cancel the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name is what the UI can display for this provider, for example "Image OpenShift Analyzer". The categories indicates to the UI which are the different categories of tests the checker will run, so the results can be classified in these categories (for example USER directive, RUN Directive, Expose directive)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks that from design perspective, the static fields are passed on the registration method

example of contract:

registerCodeActionsProvider(selector: DocumentSelector, provider: CodeActionProvider, metadata?: CodeActionProviderMetadata): Disposable;

From end user perspective, I don't really understand at all the goals of USER, RUN or Expose directive

I mean, I launch a check on the image so if the provider is there it'll do the check, else it won't.

then, if the checker is not able to analyze some images it's more a selector/filter parameter

About the name, it's part of the Result no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

design-checker

In this example, the name would be OpenShift Checker, the UI needs to know it before to start the tests, to be able to display it in the tab.

For the categories, they would be Container User and Exposed Ports, so the UI can display them without any failed / passed mark, the time the checks are executed, then they are updated with passed / failed. But I perhaps misunderstood the design doc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I read again draft of the spec

For categories, something like 'Container User' or 'Exposed Ports' are only known by the checker and to me it's part of the result of the scan, not part of the registration.

Until we didn't launch a scan, nothing is displayed except that we're processing.

In this example, the name would be OpenShift Checker, the UI needs to know it before to start the tests, to be able to display it in the tab.

what we can do for that is to give an extra argument when registering the provider with metadata

registerImageCheckerProvider(imageCheckerProvider: ImageCheckerProvider, metadata?: ImageCheckerProviderMetadata): Disposable;

and in the metadata you can specify a readonly name

without the name, we can assume the provider will take the name of the extension

so if your extension is named 'OpenShift Checker' then it'll use that name as well

Now if I look at the signature of the provider, it should return a ProviderResult result (or ImageCheck as the type we want is ImageCheck, result is just from what we call)

then if I look at Trivy extension result
image

I got them classified/sorted by severity

but if I look at

  export interface ImageCheckPartialResult {
    category: string;
    description: string;
  }

  export interface ImageCheckResult {
    status: string;
    partialResults: ImageCheckPartialResult[];
  }

(a partial result indicates a failing check on the image).

I don't get the why a partial result would be a failed result. I assume partial result is that I don't have all the result, not that it failed

so looking back at trivy results and openshift checker, it looks like I need a severity (like using a wrong EXPOSE stuff might be less critical than using an image with root user)

I would probably need some markdown result like if it passes the test

image

and then probably a score or grade if it fails.

my image is Grade A, grade B, grade C etc depending on the check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @benoitf for your feedback. I'll make changes based on this

@benoitf benoitf changed the title feat: image checker extenstion API feat: image checker extension API Nov 7, 2023
@feloy feloy force-pushed the feat-4454/image-checker-extension branch from f946e5a to 0a81db1 Compare November 8, 2023 12:20
@feloy
Copy link
Contributor Author

feloy commented Nov 8, 2023

@benoitf how do we want to proceed for integrating this API into main? Do I continue to work on the implementation of the API and the UI on this same PR?

@benoitf
Copy link
Collaborator

benoitf commented Nov 8, 2023

@feloy you can use a branch in your forked repository but to move forward, we should have one PR about the API d.ts and its implementation and other PR for UI side.
Also other PRs for the extension itself but it'll be in a separate repository

feloy and others added 14 commits November 14, 2023 09:31
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Philippe Martin <feloy1@gmail.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the feat-4454/image-checker-extension branch from 4ea5a5e to d0eb98d Compare November 14, 2023 08:32
Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy requested a review from benoitf November 14, 2023 10:36
@feloy feloy marked this pull request as ready for review November 14, 2023 10:48
@feloy feloy requested review from jeffmaury and deboer-tim and removed request for cdrage November 15, 2023 14:30
@feloy feloy merged commit bdcaff1 into containers:main Nov 17, 2023
9 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.6.0 milestone Nov 17, 2023
@@ -1015,6 +1017,17 @@ export class ExtensionLoader {
},
};

const imageCheckerProvider = this.imageCheckerProvider;
console.log('imageCheckerProvider', this.imageCheckerProvider);
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be removed

@feloy feloy mentioned this pull request Nov 17, 2023
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.

Image Checker extension API specification
3 participants