-
Notifications
You must be signed in to change notification settings - Fork 136
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
fix: TCP+TLS docker daemon connection #1629
fix: TCP+TLS docker daemon connection #1629
Conversation
Signed-off-by: Matej Vasek <mvasek@redhat.com>
/cc @riverar |
@matejvasek: GitHub didn't allow me to request PR reviews from the following users: riverar. Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@riverar please try this out |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1629 +/- ##
==========================================
- Coverage 63.61% 63.51% -0.10%
==========================================
Files 92 92
Lines 11682 11735 +53
==========================================
+ Hits 7431 7453 +22
- Misses 3575 3608 +33
+ Partials 676 674 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
tlsVerify, _ := strconv.ParseBool(tlsVerifyStr) | ||
if !tlsVerify { | ||
tlsOpts = append(tlsOpts, func(t *tls.Config) { | ||
t.InsecureSkipVerify = true |
Check failure
Code scanning / CodeQL
Disabled TLS certificate check
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.
Would delete this.
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.
But docker
CLI does this too.
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.
Without this DOCKER_TLS_VERIFY=0
wouldn't have desired effect, right?
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.
@lance @evankanderson wdyt?
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:
https://github.com/docker/cli/blob/82427d1a078e89d9bd10dd397e5ee7736a8086f9/cli/context/docker/load.go#L82-L86
Depends what sets c.SkipTLSVerify
maybe CLI only set is via flag?
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.
@riverar you are right about behaviour of the envvar -- it only matter if its set/empty.
The effect that I described (encryption+non_verify_cert) can be achieved by --tlsverify=false
flag.
So I immodestly dare to say that my implementation here is little bit better.
The thing is that we do not have --tlsverify
flag, so to achieve similar effect we can use the envvar (even if that deviates from what docker
CLI does).
Do you agree?
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.
Agreed. (I continue to be extremely disappointed with Docker, esp. its impl. on Windows.)
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.
@riverar Just try out docker --tlsverify=false image ls
it will use https but will skip cert verification.
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.
@matejvasek Ah ha! It does indeed. (That's a miserable mismatch in behavior, imo.)
@matejvasek Will check it out, thanks! |
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.
Verified func
now works in (my) TLS scenarios 🎉
tlsVerify, _ := strconv.ParseBool(tlsVerifyStr) | ||
if !tlsVerify { | ||
tlsOpts = append(tlsOpts, func(t *tls.Config) { | ||
t.InsecureSkipVerify = true |
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.
Would delete this.
@riverar: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@zroubalik @lance PTAL |
Signed-off-by: Matej Vasek <mvasek@redhat.com>
I should add some tests in case of future regression. |
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
Just a gentle reminder: accessing environment variables from anywhere outside of main
is fraught with danger. When possible (and I know this isn't practical in many situations, perhaps now being one of them), it seems to me often worth the trouble to plumb these settings through the API rather than access them directly from within the depths. Cue the obligatory diatribe about pursuing idempotency :)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland, matejvasek, riverar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
DOCKER_TLS_VERIFY
environment variable is set then TLS should be applied to the TCP connection to the docker daemon.resolves #1627