Add Dockerfile instructions to reset properties inherited from parent image

[dqminh]

This is an attemp to fix #3465. I've been working on this yesterday and figure that the design can be reviewed at this stage.

New added instructions are:

• UNSETENV ENV1 ENV2 ... unset environment variables inherited from parent image
• UNEXPOSE PORT ... remove exposed ports inherited from parent image
• NOVOLUME PATH remove volume mountpoints from parent image

[erikh]

fwiw, I think this is a good feature, but would rather have a 'RM' command that takes subcommands. Other than that new verbs are really solomon's call.

[erikh]

Please adjust your tests to additionally address this scenario:

1. user adds a volume/expose/etc of value "one"
2. user rms in the next image value "one"
3. user recreates next image value "one"

I don't see this and this will likely be a very common use case.

[thaJeztah]

but would rather have a 'RM' command that takes subcommands.

+1 to that suggestion.

Additional suggestion; I'd prefer UNSET <something> over RM <something> (just the wording); RM (remove) could possibly confuse (noob) users, for example, will RM VOLUME remove the data in the volume?

[erikh]

UNSET communicates the intent better too, I agree.

On Sep 23, 2014, at 1:22 PM, Sebastiaan van Stijn notifications@github.com wrote:

but would rather have a 'RM' command that takes subcommands.

+1 to that suggestion.

Additional suggestion; I'd prefer UNSET over RM (just the wording); RM (remove) could possibly confuse (noob) users, for example, will RM VOLUME remove the data in the volume?

—
Reply to this email directly or view it on GitHub.

[thaJeztah]

I guess the wait is now for solomon to return from holidays 😄

Thanks for the PR @dqminh nice (and welcome) feature!

[dqminh]

@erikh , @thaJeztah i agree that UNSET <something> <args> is better too. Let me see if i can make that works.

[dqminh]

@erikh i changed RM* to UNSET CMD in dqminh@810847a. Right now it's a special case too (like onbuild), hence I also put some comments in dispatch() method.

Regarding the addtional test:

user adds a volume/expose/etc of value "one"
user rms in the next image value "one"
user recreates next image value "one"

I wrote a new test in dqminh@a2aa2c0. Is this what you want ?

[erikh]

@dqminh I don't see how the test tests the functionality. I have additional comments in the line notes.

Next steps:

1. Get approval from solomon
2. Get reviews from other maintainers

Just so you know what the process is.

[SvenDowideit]

and a step in between 1 and 2 - write the documentation for it.

[dqminh]

ping @shykes @erikh . What do you think about the new instructions ?

[crosbymichael]

@shykes Right now this is a pure UI change and adding dockerfile instructions. Please review at the next design session.

[shykes]

Roger that.

On Thursday, November 13, 2014, Michael Crosby notifications@github.com
wrote:

@shykes https://github.com/shykes Right now this is a pure UI change
and adding dockerfile instructions. Please review at the next design
session.

—
Reply to this email directly or view it on GitHub
#8177 (comment).

[shykes]

Hi, sorry for the late review.

As discussed in #3465, I agree with the idea. However may I request small changes in the syntax:

• UNSETENV instead of UNSET ENV
• UNEXPOSE instead of UNSET EXPOSE
• NOVOLUME instead of UNSET VOLUME

I know it's arbitrary (as UI decisions always are), but I feel that the extra keyword adds a cognitive burden without clear benefit. I'd rather have more total keywords, but less words on any given dockerfile.

[icecrime]

Small precision: only the design was reviewed, not the implementation yet.

[erikh]

I can review the impl in a bit.

On Dec 23, 2014, at 10:16 AM, Arnaud Porterie notifications@github.com wrote:

Small precision: only the design was reviewed, not the implementation yet.

—
Reply to this email directly or view it on GitHub #8177 (comment).

[erikh]

I would like to see these runconfig elements as separate functions. This function is extremely long and anything that goes towards making it simpler, I am all for.

[erikh]

Tests look good, but this needs a serious refactor because of the syntax changes.

Please ping me if/when this arrives.

Thanks!

[icecrime]

@dqminh Would you be available to rebase/update this PR based on the last feedback? Now that we have a go on design, we should be able to move forward with it. Thank you!

[erikh]

It’s going to need a full redesign, the methods used there will not work for the new syntax.

-Erik

On Dec 23, 2014, at 6:36 PM, Arnaud Porterie notifications@github.com wrote:

@dqminh https://github.com/dqminh Would you be available to rebase/update this PR based on the last feedback? Now that we have a go on design, we should be able to move forward with it. Thank you!

—
Reply to this email directly or view it on GitHub #8177 (comment).

[icecrime]

@erikh Oh crap... :-/

@dqminh Let us know if you're available to submit a new PR. Until then I'm afraid we're gonna have to close that one.

[erikh]

Don’t close this one, he can just force push on top of it.

-Erik

On Dec 23, 2014, at 7:20 PM, Arnaud Porterie notifications@github.com wrote:

@erikh https://github.com/erikh Oh crap... :-/

@dqminh https://github.com/dqminh Let us know if you're available to submit a new PR. Until then I'm afraid we're gonna have to close that one.

—
Reply to this email directly or view it on GitHub #8177 (comment).

[dqminh]

Thanks so much for the review guys ❤️

@erikh @icecrime afair there should not be too much changes ( it's essentially a revert of dqminh@63b3daf ). I can just force-push new changes to current branch. I should be able to work on this soonish.

[erikh]

Thanks Daniel!

-Erik

On Dec 23, 2014, at 7:32 PM, Daniel, Dao Quang Minh notifications@github.com wrote:

Thanks so much for the review guys

@erikh https://github.com/erikh @icecrime https://github.com/icecrime afair there should not be too much changes ( it's essentially a revert of dqminh/moby@63b3daf dqminh@63b3daf ). I can just force-push new changes to current branch. I should be able to work on this soonish.

—
Reply to this email directly or view it on GitHub #8177 (comment).

[cmehay]

I have an usecase for UNEXPOSE

Linking a container with exposed port to another one results some environment variables to be created on the container with information about exposed ports and environment.

I'm using these variables with a tool to manage auto-configuration in containers, if there is an exposed port which is not actually used, my configuration will be corrupted 😞

There is no way to reset exposed port from a container, you can only add some but nothing to close one.

So, I think that UNEXPOSE is an important feature.

[juliangamble]

Use case for UNEXPOSE

Suppose I have four docker hosts and I want to run 16 maven tomcat containers which all default to internal port 8080.

Now imagine I'm using registrator with rancher CNI - which locks me down to the internal port.
gliderlabs/registrator#541 (comment)
This means I can only run one 8080 internal port per host. (Since I have to do 8080:8080 port mappings)

In this situation - docker's internal->external port mapping isn't enough to solve my problem. I actually need to override the internal port mapping, preferably without rebuilding the original container.

[maxgorovenko]

You guys spent almost 4 years for talking about UI and command name? Really?

[edevil]

Just today I needed to run a wordpress site and I faced the same problem, it declares a /var/www/html volume: https://github.com/docker-library/wordpress/blob/master/Dockerfile-debian.template#L28

I wanted to include the content via a child image but could not...

[ibc]

Excuse me, is this implemented or not?

[thaJeztah]

@ibc no it wasn't; see #3465 (comment)

[juliangamble]

Thanks @thaJeztah - for us to do a pull request - could you please point to the line of code (in your point of view) for us to start reading.

[gdraheim]

As in the dicussion of #3465 please note that there is a workaround available (using docker-copyedit).

[kinglion811]

the UNSETENV or UNSET ENV can work now,how can I delete some base image's env when I build image

[chpock]

+UNVOLUME

[Athaphian]

Why was this closed due to no real world cases? This happens pretty often right?

We build a bunch of docker images that use each other as base image. In one of the base images we set ENV CYPRESS_BINARY_VERSION because all our projects require this. But now we want to upgrade Cypress to a new version for one project at the time and this version requires CYPRESS_BINARY_VERSION to not exist... so we need to override this in one of the project docker images but not change the base image.. only UNSET does not exist! This was started in 2014, it could have been fixed in 2017, but still almost 2019 and not fixed?

[gdraheim]

@Athaphian .... #3465 ist still open.

(and you can use the workaround with docker-copyedit )

[Athaphian]

@gdraheim Im sure that will work, and we might eventually go that route too. But it is more tedious than just adding unsetenv in the dockerfile.

[akramfares]

+UNVOLUME

[syedhassaanahmed]

Really need an UNVOLUME option. docker-copyedit is a clunky option.

[ebuildy]

Sorry to "exhume" this wonderful PR, but clearly remove env / volume / port from base image make sense in order to avoid bad surprise (such as a ton of abandoned volumes, unwanted exposed ports ...), the PR seem well documented and tested, why this is not merged yet?

Or we should strongly discourage to use such properties in base image? (I am in trouble with the official image https://github.com/docker-library/docker/blob/65fab2cd767c10f22ee66afa919eda80dbdc8872/18.09/dind/Dockerfile#L40 that create a volume where Docker images are stored, in CI env, this is really unwanted).

Thanks you

[seraasch]

Also have a use case for UNEXPOSE.

Why is this marked closed?

[markmsmith]

@icecrime Sorry to @ you, but I was just curious if there were any plans to revisit this PR, now that several real world use cases have been collected above. It looks like #3465 has been reopened, so does that mean that this is back on the table?

I ran in to this problem today using the official Postgres image with the TestContainers project (https://github.com/testcontainers/testcontainers-node), where my integration tests started generating volumes from the containers. It appears that the docker run --rm command has been enhanced to remove unnamed volumes (#19568), but this doesn't help when working with the container programatically.
I'm hoping that there will be a solution in the future to allow extending a base image and unset a volume from the parent.

[zengqingfu1442]

@dqminh Thanks, are these commands available now in Dockerfile?

New added instructions are:
UNSETENV ENV1 ENV2 ... unset environment variables inherited from parent image
UNEXPOSE PORT ... remove exposed ports inherited from parent image
NOVOLUME PATH remove volume mountpoints from parent image

[yywing]

+UNVOLUME

[simaotwx]

I'm all for UNVOLUME which would be really helpful for a number of scenarios where all someone wants is to remove a volume from a base image and nothing else.

[alexeyp0708]

@icecrime

Collective review with @LK4D4 @jfrazelle @crosbymichael @calavera

I'm sorry, we don't understand valid use cases for this. Could you please bring us more use cases, or a link to a real world Dockerfile people use but have trouble inheriting from it because of the lack of such feature?

We're closing this as we mostly disagree, but please feel free to prove us wrong in the comments and we can reconsider!

Arguments in favor for adding UNVOLUME.
The problem exists and people suffer from it and ask for something to be done.

1. The child image does not always need the volumes of the parent image.

2. There is such a problem. COPY can add files to an existing VOLUME but RUN can't #33842 (comment) Taken together, this behavior with the default parent volume makes the situation unpredictable.

3. At the same time, your documentation describes the situation ambiguously.
   https://docs.docker.com/reference/dockerfile/#volume

Changing the volume from within the Dockerfile: If any build steps change the data within the volume after it has been declared, those changes will be discarded.

Therefore, in theory, the files of a child image will under no circumstances end up in parent default volume.
Buildkit is not affected by this problem since for it volumes (in VOLUME instruction) are metadata, and default volumes are mounted when the container is launched.

4. When changing daemon settings, developers sometimes prefer to specify their own volumes for worker processes (For example, specify a directory in a shared volume for containers.) .
   As a result, parent volumes hang like extra garbage by default and take up disk space (often data from the container is copied to volumes by default).

5. The developer would prefer to have access to the folder in the container that the volume overlaps.
   But the default volume always overlaps this folder in the container and the user does not have access to it.

Please study the topic #3465 and you will understand that there are problems. It’s just that not everyone can or wants to describe the situation that the developers encountered.

I think there are enough legal grounds to consider this problem seriously and include any option for canceling the parent default volume.

[simaotwx]

Collective review with @LK4D4 https://github.com/jfrazelle @crosbymichael @calavera

I'm sorry, we don't understand valid use cases for this. Could you please bring us more use cases, or a link to a real world Dockerfile people use but have trouble inheriting from it because of the lack of such feature?

We're closing this as we mostly disagree, but please feel free to prove us wrong in the comments and we can reconsider!

My use case is https://github.com/docker-library/wordpress/blob/master/latest/php8.1/fpm/Dockerfile#L151 where I want to add a customized wordpress install directly to a downstream image but I have to resort to copying the entire file, just to remove that VOLUME.

I saw that you can use COPY --from base / / but I'm not sure if that would introduce edge cases and weird behavior with symlinks, xattrs, ACLs, permissions, ownership, creation/modification time and special files.

[thaJeztah]

Sorry, but I'm locking the conversation on this PR; this PR is 10 years old, including decisions made 10 Years ago.

The Dockerfile syntax and Builder is no longer maintainer in this repository, and now maintained by the BuildKit project (https://github.com/moby/buildkit), which is where features like this can be implemented.

While this original PR was rejected, the ticked related to this PR was reopened, but the overall design may need discussion. Also note that some of the use-cases (UNVOLUME or UNSET VOLUME to prevent volumes from being applied during build) changed with BuildKit, which does not apply volumes during build.

This comment #3465 (comment) is relevant here;

@veqryn since reopening this issue, nobody started working on a pull-request; the existing pull request did no longer apply cleanly on the code-base so a new one has to be opened; if anyone is interested in working on this, then things can get going again.

See my earlier comment(s); #3465 (comment)

We discussed this issue in the maintainers meeting, and generally, we're OK to start working on this again.
@duglin are you perhaps interested in working on this?

And #3465 (comment)

As to why it isn't there yet; simply because nobody has had time to start working on it, but if someone is interested, it most likely will be accepted.

These things are best discussed in the BuildKit issue tracker, in which a design can be proposed.