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

validated bad container image URL #4283

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

msivasubramaniaan
Copy link
Collaborator

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 43.88%. Comparing base (da60441) to head (e2c3bdb).
Report is 360 commits behind head on main.

Files Patch % Lines
src/deployment.ts 29.41% 12 Missing ⚠️
src/oc/ocWrapper.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4283       +/-   ##
===========================================
+ Coverage   32.37%   43.88%   +11.50%     
===========================================
  Files          85       95       +10     
  Lines        6505     7716     +1211     
  Branches     1349     1634      +285     
===========================================
+ Hits         2106     3386     +1280     
+ Misses       4399     4330       -69     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

We should change the way we validate it. See the in line comment

src/deployment.ts Outdated Show resolved Hide resolved
@msivasubramaniaan msivasubramaniaan added this to the 1.16.0 milestone Jul 15, 2024
@msivasubramaniaan msivasubramaniaan removed this from the 1.16.0 milestone Jul 15, 2024
vrubezhny
vrubezhny previously approved these changes Jul 15, 2024
Copy link
Contributor

@vrubezhny vrubezhny left a comment

Choose a reason for hiding this comment

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

Looks good to me.
After moving the image info validation to onDidAccept it works really well!
Also it doesn't make any excessive 'oc' binary invocations, which is also good, imho.

@@ -675,6 +675,13 @@ export class Oc {
.catch((error) => namespaces);
}

public async hasImageInfo(url: string): Promise<boolean> {
const result = await CliChannel.getInstance().executeTool(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try this with docker.io/library/mongo. For me at least, it fails, so I can't deploy mongodb

Copy link
Contributor

@vrubezhny vrubezhny Jul 15, 2024

Choose a reason for hiding this comment

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

image

I could successfully deploy mongodb using URL docker.io/library/mongo. Some temporary/network problem?

PS: The only problem is that it has gone into CrashLoopBackOff immediately

Copy link
Collaborator

Choose a reason for hiding this comment

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

What version of the mongo DB image do you have? I have a31b196b207d which should be the latest image, since I recently deleted all my container images to free some space. Maybe this affects it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll double check on windows

Copy link
Contributor

@vrubezhny vrubezhny Jul 15, 2024

Choose a reason for hiding this comment

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

Can I check it with OS Tools somehow?

Docker says the mongo:latest is a31b196b207d...

$ docker images --no-trunc
REPOSITORY                           TAG       IMAGE ID                                                                  CREATED         SIZE
mongo                                latest    sha256:a31b196b207d768e78f2af331869e91d13443f691080d3b93e8009a53391eeaa   2 weeks ago     796MB

However, the Pod's config says:

...
"containerStatuses": [
            {
                "containerID": "cri-o://4080a87a588b22b75135f996350ffe331dc13b6a4423d07f550450bb9d929f43",
                "image": "docker.io/library/mongo:latest",
                "imageID": "docker.io/library/mongo@sha256:1cd3951000020c1cb1757868e6cfd82667f57d80bb31fed8b585e26a8a1d960f",
...

which actually doesn't look like the same image... Does it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was able to reproduce the same error on Windows. I guess you can check the status of running the command that's listed here. I don't really know what's going on

Copy link
Collaborator

Choose a reason for hiding this comment

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

The command result is an error for me, since it detects windows and linux versions of the docker image

Copy link
Contributor

@vrubezhny vrubezhny Jul 15, 2024

Choose a reason for hiding this comment

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

The command result is an error for me, since it detects windows and linux versions of the docker image

I didn't get this... So, you have the same error result while deploying the same image on Windows and *nix?
And what does it give to us?

... And yes, I think if you hit the docker pull rate limit, you'll probably won't be able to deploy any image till the counter is dropped. Not 100% sure though.

@datho7561
Copy link
Collaborator

It would be nice to have some sort of indication that a verification is going on; I don't know yet how this would be implemented though.

@msivasubramaniaan
Copy link
Collaborator Author

It would be nice to have some sort of indication that a verification is going on; I don't know yet how this would be implemented though.

@datho7561 added progress to show the info
image-url-validation

@datho7561
Copy link
Collaborator

The progress indicator on the image check looks great! I'd like to figure out what's going on with the mongodb image before merging.

@datho7561
Copy link
Collaborator

I just hit my pull rate limit for docker. Maybe this is related?

@vrubezhny
Copy link
Contributor

I just hit my pull rate limit for docker. Maybe this is related?

If I execute oc image info docker.io/library/mongo I get quite a long list of mongo images with different versions, and the command output ends with:

error: the image is a manifest list and contains multiple images - use --filter-by-os to select from:

  OS             DIGEST
  linux/amd64    sha256:19c11a8f1064fd2bb713ef1270f79a742a184cd57d9bb922efdd2a8eca514af8
  linux/arm64/v8 sha256:f9b01e368fd95a783a68471930a15c2494d525318ff65a1b408ff3e6bc83e3d4
  windows/amd64  sha256:bbb44a208559a1fad8f164e0bd97db2b9f4b699d6adfa01ba4f20b9ec60eb832
  windows/amd64  sha256:bc25ab206f79c4618ca23b8fb5f8b2710783ed6a5058f30e8f196a85f3f5c3fb

None of these images has neither a31b196b207d (mongo:latest) nor 1cd395100002 sha256 and actually might not be full.

So, I'm not actually sure if we can really check what's getting installed with oc create deployment ... --image=docker.io/library/mongo.

@datho7561
Copy link
Collaborator

@vrubezhny @msivasubramaniaan I think this will work, however I have exceeded the rate limit and need to wait before I can try it again:

oc image info docker.io/library/mongo:latest --show-multiarch=true

changes I made:

:latest --> This means info is only collected for the newest version of the container image

--show-multiarch=true --> from the argument description, this should prevent an error code from being returned when the container supports multiple architectures

@vrubezhny
Copy link
Contributor

vrubezhny commented Jul 15, 2024

oc image info docker.io/library/mongo:latest --show-multiarch=true

Do you suggest adding :latest and --show-multiarch=true to the command arguments?

If so, we should take care if user has NOT already added any tags to the URL (like :latest)

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.

Workflow for entering a bad container image URL isn't good
4 participants