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

Vendored Docker SDK for Python updates #434

Merged
merged 10 commits into from
Jul 31, 2022

Conversation

felixfontein
Copy link
Collaborator

SUMMARY

Incorporates new commits that apply from https://github.com/docker/docker-py/commits/main.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

vendored Docker SDK for Python components

@felixfontein
Copy link
Collaborator Author

felixfontein commented Jul 29, 2022

Comment on lines 1 to 11
bugfixes:
- "modules and plugins communicating directly with the Docker daemon - fix parsing of IPv6 addresses with a port in ``docker_host``. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "modules and plugins communicating directly with the Docker daemon - fix ``ProxyCommand`` handling for SSH connections when not using ``use_ssh_client=true``. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "modules and plugins communicating directly with the Docker daemon - do not create a subshell for SSH connections when using ``use_ssh_client=true``. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
- "docker_image - when composing the build context, trim trailing whitespace from ``.dockerignore`` entries. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
minor_changes:
- "modules and plugins communicating directly with the Docker daemon - improve default TLS version selection for Python 3.6 and newer. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
security_fixes:
- "modules and plugins communicating directly with the Docker daemon - when connecting by SSH and not using ``use_ssh_client=true``, reject unknown host keys instead of accepting them. This is only a change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
breaking_changes:
- "modules and plugins communicating directly with the Docker daemon - when connecting by SSH and not using ``use_ssh_client=true``, reject unknown host keys instead of accepting them. This is only a breaking change relatively to older community.docker 3.0.0 pre-releases, or with respect to Docker SDK for Python < 6.0.0. Docker SDK for Python 6.0.0 will also include this change (https://github.com/ansible-collections/community.docker/pull/434)."
Copy link
Contributor

@gotmax23 gotmax23 Jul 31, 2022

Choose a reason for hiding this comment

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

Grammar nitpick: s/relatively/relative/. I like adverbs (my name is Maxwell, after all), but this is not the correct place to use them. Perhaps it also makes sense to shorten modules and plugins communicating directly with the Docker daemon to just plugins communicating directly with the Docker daemon, as modules are a type of plugin, but meh.

Also you don't need a comma in , or with respect to Docker SDK for Python < 6.0.0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed everything except the "modules and plugins ...". That because a) that formulation has been used in earlier changelog fragments, and b) many users do not know that modules are a special kind of plugin. Since the changelog is mainly for users, I think it's important to also mention modules here.

Copy link
Contributor

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

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

I meant to submit that other comment as part of the review, but instead, I clicked the wrong button and got side tracked. All of the Python code looks good to me, and if we wanted to change it more, it would be nice to submit it upstream first. I would suggest preserving the entire commit messages from upstream in the future, instead of just the subjects, but I won't hold up the review on that.

felixfontein and others added 10 commits July 31, 2022 16:41
This was using a deprecated function (`urllib.splitnport`),
ostensibly to work around issues with brackets on IPv6 addresses.

Ironically, its usage was broken, and would result in mangled IPv6
addresses if they had a port specified in some instances.

Usage of the deprecated function has been eliminated and extra test
cases added where missing. All existing cases pass as-is. (The only
other change to the test was to improve assertion messages.)

Cherry-picked from
docker/docker-py@f16c4e1

Co-authored-by: Milas Bowman <[email protected]>
We want "The above exception was the direct cause of the following exception:" instead of "During handling of the above exception, another exception occurred:"

Cherry-picked from
docker/docker-py@bb11197

Co-authored-by: Maor Kleinberger <[email protected]>
Specific TLS versions are deprecated in latest Python, which
causes test failures due to treating deprecation errors as
warnings.

Luckily, the fix here is straightforward: we can eliminate some
custom version selection logic by using `PROTOCOL_TLS_CLIENT`,
which is the recommended method and will select the highest TLS
version supported by both client and server.

Cherry-picked from
docker/docker-py@56dd6de

Co-authored-by: Milas Bowman <[email protected]>
In the Secure Shell (SSH) protocol, host keys are used to verify the identity of remote hosts. Accepting unknown host keys may leave the connection open to man-in-the-middle attacks.

Do not accept unknown host keys. In particular, do not set the default missing host key policy for the Paramiko library to either AutoAddPolicy or WarningPolicy. Both of these policies continue even when the host key is unknown. The default setting of RejectPolicy is secure because it throws an exception when it encounters an unknown host key.

Reference: https://cwe.mitre.org/data/definitions/295.html

NOTE: This only affects SSH connections using the native Python SSH implementation (Paramiko), when `use_ssh_client=False` (default). If using the system SSH client (`use_ssh_client=True`), the host configuration
(e.g. `~/.ssh/config`) will apply.

Cherry-picked from
docker/docker-py@d929864

Co-authored-by: Audun Nes <[email protected]>
Set `daemon` attribute instead of using `setDaemon` method that
was deprecated in Python 3.10.

Cherry-picked from
docker/docker-py@adf5a97

Co-authored-by: Karthikeyan Singaravelan <[email protected]>
Use `from e` to ensure that the error context is propagated
correctly.

Cherry-picked from
docker/docker-py@05e1434

Co-authored-by: Milas Bowman <[email protected]>
@felixfontein
Copy link
Collaborator Author

All of the Python code looks good to me, and if we wanted to change it more, it would be nice to submit it upstream first.

Upstream is Python 3.6+, while we also support Python 3.5 and Python 2.7. So I cannot copy the upstream changes literally, and they will not want the things we have to change for compatibility :-)

I would suggest preserving the entire commit messages from upstream in the future, instead of just the subjects, but I won't hold up the review on that.

Good idea, I've included the entire messages for all commits (except the signed-off part, which I converted to Co-authored-by: so that GH shows correct attribution). The message is sometimes misleading (like mentioning that the code now uses from e, while we actually have to use six's raise_from()), but in general I think it's more helpful to have the full message included.

@felixfontein felixfontein merged commit ae708a7 into ansible-collections:main Jul 31, 2022
@felixfontein felixfontein deleted the docker-py-updates branch July 31, 2022 15:09
@felixfontein
Copy link
Collaborator Author

@gotmax23 thanks for reviewing this!

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.

2 participants