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

Parse security_opts before sending them to docker daemon #7554

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

benbuzbee
Copy link
Contributor

Fixes #6720

Copy the parsing function from the docker CLI. Docker daemon expects to see JSON for seccomp file not a path.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 31, 2020

CLA assistant check
All committers have signed the CLA.

@benbuzbee benbuzbee force-pushed the benbuz/fix-seccomp-file branch from 6a37b45 to d689a75 Compare March 31, 2020 01:39
Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding a test too!

I made a couple of test suggestions that you can follow up. I intend to merge by Tuesday end of my day either way. Thanks again.

drivers/docker/driver_test.go Outdated Show resolved Hide resolved
drivers/docker/driver_test.go Outdated Show resolved Hide resolved
func TestDockerDriver_SecurityOptFromFile(t *testing.T) {
seccompPath := "./test-resources/docker/seccomp.json"

if runtime.GOOS == "windows" {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also move the test to driver_unix_test.go (or driver_linux_test.go) instead if it doesn't apply to Windows. Does it apply to macOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Honestly not sure. I copied the boiler plate from the existing seccomp=unconfined test. I would have thought even windows would run the container in a VM where seccomp applies but I have no good way to test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense - I'll test behavior!

Fixes hashicorp#6720

Copy the parsing function from the docker CLI. Docker daemon expects to see JSON for seccomp file not a path.
@benbuzbee benbuzbee force-pushed the benbuz/fix-seccomp-file branch from d689a75 to a3c3f7e Compare March 31, 2020 15:35
@notnoop
Copy link
Contributor

notnoop commented Mar 31, 2020

Thank you so much @benbuzbee - this fix should be out in 0.11.0!

@notnoop notnoop merged commit b931961 into hashicorp:master Mar 31, 2020
@benbuzbee benbuzbee deleted the benbuz/fix-seccomp-file branch April 8, 2020 17:22
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker driver security_opts field doesn't match flag behavior
3 participants