Use only official Dockerfile parser

[rhcarvalho]

This is a followup on @Kargakis' comment: #4562 (comment).

• Remove custom parser from pkg/generate/dockerfile, in favor of the official Dockerfile parser and AST traversal.
• Fix bug: in new-app, only the first EXPOSE instruction was used to get exposed ports.
• Fix bug: in new-app, we were taking the base image from the first FROM instruction, instead of the last.

[rhcarvalho]

@bparees @mfojtik PTAL

[test]

[rhcarvalho]

We should stop right away if the Dockerfile is unparseable: no chance Docker will be able to build it, since we rely on Docker's Dockerfile parser.

[rhcarvalho]

@bparees educational question / potential limitation:
new-app tries to mark ports to be exposed based on EXPOSE instruction(s) in a Dockerfile. Theoretically, that will not work with layered images where EXPOSE instructions are in the upper layers.

E.g.:

# rhcarvalho/httphostname
FROM gliderlabs/alpine
RUN apk-install curl
COPY httphostname /
EXPOSE 8080
ENTRYPOINT ["/httphostname"]

# app/Dockerfile 
FROM rhcarvalho/httphostname

In practice, oc new-app of the latter image works fine and sets up a service with correct port mappings (even if I change from 8080 to something else).

So, two questions:

• How do we eventually get the right ports? (I suspect we inspect the base image at some point)
• Given that eventually we figure out the right ports, do we really need to look for EXPOSE instructions in the Dockerfile?

[bparees]

@rhcarvalho i don't know the answer to those questions, maybe @csrwng does.

[csrwng]

@rhcarvalho @bparees For new-app we have two inputs... the image that you want to base your build on (builder image for s2i builds, or FROM image in the case of Docker builds) and source. In the case of a Docker build, we can inspect the FROM image because it exists, but the image that results from building your Dockerfile doesn't exist yet, all we have is the source. Therefore we need both, the ports exposed by the parent image if any, and the ports exposed in your source Dockerfile.

[rhcarvalho]

thanks @csrwng! Could you give me a pointer to where we Docker inspect the parent?

[rhcarvalho]

Maybe answering my own question, this is the only place I found in new-app code with a call to InspectImage: pkg/generate/app/dockerimagelookup.go#L84

[rhcarvalho]

@bparees comment addressed: consider regular files or symlinks named "Dockerfile" PTAL

[csrwng]

@rhcarvalho the image metadata can come from one of 3 places (in order of priority):

1. metadata from an ImageStreamImage stored in OpenShift (imagestreamlookup.go)
2. metadata from the registry where the image is stored (dockerimagelookup.go - DockerRegistrySearcher)
3. metadata we find in the local docker client (dockerimagelookup.go - DockerClientSearcher)

[bparees]

lgtm, needs squash.

[rhcarvalho]

@bparees the changes to pkg/generate/dockerfile/finder.go (first commit) are unrelated to "Use only official Dockerfile parser". But I put that commit in the same PR as I was reviewing the whole package. Should I submit it as a separate PR?

[rhcarvalho]

@bparees ok, single commit replacing the custom Dockerfile parser. Fix to Dockerfile finder goes in a separate PR: #4849

[bparees]

[test][extended:core]

[0xmichalis]

Shouldn't we depart from the graph terminology and use something more understandable like Arguments? I would prefer this to check if the provided node is a Dockerfile command, too.

[rhcarvalho]

This is a helper for manipulating the AST. Calling it Arguments is promising something we cannot fulfill, at least not generically.
For most commands, collecting the values of the linked list formed by the node.Next pointers gives the "arguments" of the given command, therefore the usefulness of this helper. It's not true for ONBUILD, for instance. It's also less useful for instructions that take pairs of arguments like ENV. You have to know what you're feeding the function with.

IMHO, what we can do is to have helpers to work with specific commands. E.g., given a Dockerfile's root node, return:

1. a list of all exposed ports;
2. the image of every from instruction.

WDYT?

[0xmichalis]

After the discussion we had, yes, i think specialized exported funcs is the way to go. This has to be unexported. 👍

[rhcarvalho]

@csrwng @bparees could you please have a closer look here?
Existing code will proceed to call b.AddComponents in case you have a Dockerfile like this:

# missing image argument below
FROM
RUN yum install -y apache
...

What should we be doing here? Maybe we should remove the continue and call b.AddComponents passing []string{""}?!

[mfojtik]

We replacing the FROM anyway, right? So it does not matter what you have in FROM

[rhcarvalho]

@mfojtik we use it, I think to look up existing ImageStreams, but @csrwng will know better.

[rhcarvalho]

BTW, unless we manipulate the Dockerfile to define a base image, building a Dockerfile without FROM will fail. AFAIK there's no way to define the base image in the new-app flow.

[csrwng]

Yes we do use it to look up the parent image and get its metadata. I didn't know we could have an empty FROM statement, is that equivalent to FROM scratch ?

[csrwng]

lgtm

[bparees]

works for me. @csrwng i don't think my proposed changes would impact this flow anyway. you could still have a case where you specify a repo with a dockerfile and do not specify an explicit docker image to use with it, in which case new-app would need to process the Dockerfile From to learn about the image being FROM'd.

[csrwng]

@bparees true, though you could imagine a Dockerfile that has an empty FROM that will only work with new-app where you specify which FROM you want :)

[bparees]

also true :)

[rhcarvalho]

Completing the list of crazy possibilities, I think today we don't handle "replacing" the FROM of images that have no FROM instruction. We could certainly insert one at the very beginning of the Dockerfile in that case.
Anyway, I wouldn't recommend anyone that pattern, as you'd be creating invalid Dockerfiles that happen to work in OpenShift, sometimes. Also, if one day Docker will have code to validate the parsed file independent of executing a build, we could use that validation ourselves before manipulating the AST, which would make those crazy Dockerfiles broken.

[rhcarvalho]

The test that is failing is the integration test TestAuthorizationResourceAccessReview, can Jenkins [test] again?

[openshift-bot]

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5717/)

[rhcarvalho]

Trying to sort out the failing tests....

According to @deads2k TestBootstrapPolicyOverwritePolicyCommand is a flake.

TestSimpleImageChangeBuildTriggerFromImageStreamTagDocker failed with:

--- FAIL: TestSimpleImageChangeBuildTriggerFromImageStreamTagDocker (13.34s)
    imagechange_buildtrigger_test.go:206: unexpected error: namespaces "integration" already exists

I think that's also a flake?

[openshift-bot]

Evaluated for origin test up to 8a9cb99

[mfojtik]

@rhcarvalho can you report the flake as separate issue?>

[mfojtik]

LGTM (are there any open issues about this? of so, lets convert them to follow ups and move on :)

[rhcarvalho]

@mfojtik all the comments were addressed. Just the (seemingly unrelated) extended tests keep falling...

[mfojtik]

[merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3613/) (Image: devenv-fedora_2464)

[openshift-bot]

Evaluated for origin merge up to 8a9cb99

[rhcarvalho]

@mfojtik issues for the last flakes: #5113 and #5075