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

Update docker run --security-opt command format #2757

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

stmcginnis
Copy link
Contributor

Issue number:

N/A

Description of changes:

During our build process we have several places where we use the Bottlerocket SDK to perform the build. In these docker run commands we pass --security-opt label:disable to avoid labeling a large number of files.

It appears this syntax may have changed since we started using it, and the correct syntax is now label=disable. Based on reports in the Docker repo, the way we had it of label:disable may not actually work.

While there are other issues to be addressed, the older format generates an error when trying to build with finch, while the newer format - though not recognized - only generates a warning message.

Testing done:

Performed full build, made sure everything completed and no errors.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

During our build process we have several places where we use the
Bottlerocket SDK to perform the build. In these `docker run` commands we
pass `--security-opt label:disable` to avoid labeling a large number of
files.

It appears this syntax may have changed since we started using it, and
the correct syntax is no `label=disable`. Based on reports in the Docker
repo, the way we had it of `label:disable` may not actually work.

While there are other issues to be addressed, the older format generates
an error when trying to build with finch, while the newer format -
though not recognized - only generates a warning message.

Signed-off-by: Sean McGinnis <stmcg@amazon.com>
@webern
Copy link
Contributor

webern commented Jan 30, 2023

Do you think there's any chance it is related to this? bottlerocket-os/twoliter#173

@stmcginnis
Copy link
Contributor Author

Do you think there's any chance it is related to this? bottlerocket-os/twoliter#173

Tangentially related, only in that if you are not using Docker Desktop and are instead trying to use finch, having that argument the way it is now will completely blow up.

I'm not really sure about the issue with Docker not really enforcing it if it is not formatted correctly. It is possible that could be related to the failure, but I have a feeling we're going to find some other sharp edges if we try to support building on macOS.

@cbgbt
Copy link
Contributor

cbgbt commented Feb 2, 2023

Is there significant overlap in Docker support for the new semantics vs the old? Do we need an "MSDV" (Minimum Supported Docker Version) advertised in the build instructions for Bottlerocket? Seems like a possible footgun for folks using the old docker client.

@stmcginnis
Copy link
Contributor Author

Is there significant overlap in Docker support for the new semantics vs the old? Do we need an "MSDV" (Minimum Supported Docker Version) advertised in the build instructions for Bottlerocket? Seems like a possible footgun for folks using the old docker client.

Good question. From what I could find, the "new" syntax I am using here has been the correct syntax since at least 2017.

docker-archive/docker-ce@5a52b7e#diff-817dee6c24c28e3521d4bcd856594f7af2dce8e1712f45451fbe9e90e39f2ea8R649

So my assumption would be that this PR is correcting the format to what it was always supposed to be. It used to handle parsing the non-official format we have been using, but that has never been the documented format for using this arg. Or if it was, it was so long ago that the risk of someone still using a client old enough where this could be an issue should be close to 0.

@jpculp
Copy link
Member

jpculp commented Feb 2, 2023

We should verify if this is still true, but in our BUILDING.md we recommend "Docker 20.10.10 or later".

As far as if label:disable still works, I believe that it does, but Docker-like tools such as podman absolutely require the new syntax (at least last time I tinkered with it).

@stmcginnis stmcginnis merged commit 8c4df72 into bottlerocket-os:develop Aug 18, 2023
@stmcginnis stmcginnis deleted the sec-label branch August 18, 2023 12:13
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.

None yet

4 participants