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
api/pre-1.44: Default ReadOnlyNonRecursive
to true
#47391
Conversation
cb8306a
to
a995be2
Compare
return errdefs.InvalidParameter(errors.New("BindOptions.ReadOnlyForceRecursive needs API v1.44 or newer")) | ||
} else { | ||
bo = &mount.BindOptions{} | ||
hostConfig.Mounts[idx].BindOptions = bo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is needed, because m.BindOptions = ...
would only overwrite the BindOptions
in the m
variable which is a local copy of the original mount and not a reference to the Mount
in hostConfig.Mounts
.
Failure related, test needs adjustment:
EDIT: Oh, actually it's not the test, |
a995be2
to
16bafea
Compare
16bafea
to
c247ce3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
c247ce3
to
c12c8b0
Compare
Looks like some failures are related:
Parsing added by this PR doesn't return-early in case of error, so not sure yet why that happens.. |
6694464
to
8057434
Compare
} | ||
} | ||
newBinds = append(newBinds, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this clear the Binds
field after they've been migrated?
Also wondering if this migration should always happen, so also on current API versions 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't migrate all Binds
(yet), only the read-only ones to reduce potential blast area.
Was planning to make a separate PR that would also deprecate the Binds
field and transform all of them to Mounts
.
f4db0f9
to
1f7ccf9
Compare
This looks green now, failures are unrelated and are caused by Docker Hub search API being down. |
1f7ccf9
to
020b64e
Compare
8539542
to
6e21f36
Compare
@@ -426,6 +426,69 @@ func TestContainerCopyLeaksMounts(t *testing.T) { | |||
assert.Equal(t, mountsBefore, mountsAfter) | |||
} | |||
|
|||
func TestContainerBindMountReadOnlyDefault(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a to check if rro is supported on the kernel for the test to function properly.
Alternatively, we could just check the registered mountpoints in the API to see if the option is set as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also sorry, one more check, test should be skipped if api version is < 1.44.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It also tests that the behavior doesn't change when <1.44 API is requested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes technically the 1.43 test should pass on an older daemon.
But the 1.44 test would fail since it is not a supported version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.44 test will be skipped: 0f8270b#diff-0ac47068153ec59b77b2044bd459fb6395f028e6e112dc03645f7ba72f324bf2R456
But yeah the version: ""
would fail, updated it to also be skipped.
Also, added the test for api.MinSupportedAPIVersion
client version.
Don't error out when mount source doesn't exist and mounts has `CreateMountpoint` option enabled. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
6e21f36
to
0f8270b
Compare
Don't change the behavior for older clients and keep the same behavior. Otherwise client can't opt-out (because `ReadOnlyNonRecursive` is unsupported before 1.44). Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
0f8270b
to
4323903
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Don't change the behavior for older clients and keep the same behavior.
- How to verify it
TestContainerBindMountReadOnlyDefault
- Description for the changelog
- To preserve backwards compatibility, read-only mounts are not recursive by default when using older clients (API version < v1.44).
- A picture of a cute animal (not mandatory but encouraged)