fix: TCP+TLS docker daemon connection #1629

matejvasek · 2023-03-15T21:37:17Z

Changes

🐛 Fix TCP+TLS docker daemon connection. If the DOCKER_TLS_VERIFY environment variable is set then TLS should be applied to the TCP connection to the docker daemon.

fix: TCP+TLS docker daemon connection

resolves #1627

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-03-15T21:37:38Z

/cc @riverar

knative-prow · 2023-03-15T21:37:40Z

@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:

/cc @riverar

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.

matejvasek · 2023-03-15T21:37:51Z

@riverar please try this out

codecov · 2023-03-15T21:43:56Z

Codecov Report

Patch coverage: 16.36% and project coverage change: -0.10 ⚠️

Comparison is base (37fa5c8) 63.61% compared to head (4b385c8) 63.51%.

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

Flag	Coverage Δ
e2e-test	`38.95% <3.63%> (-0.18%)`	⬇️
e2e-test-oncluster	`34.67% <3.63%> (-0.16%)`	⬇️
e2e-test-oncluster-runtime	`29.52% <0.00%> (?)`
e2e-test-runtime-go	`30.22% <3.63%> (?)`
e2e-test-runtime-python	`30.18% <3.63%> (?)`
e2e-test-runtime-quarkus	`30.33% <3.63%> (?)`
e2e-test-runtime-springboot	`30.38% <3.63%> (?)`
e2e-test-runtime-typescript	`30.33% <3.63%> (?)`
integration-tests	`49.81% <16.36%> (-0.19%)`	⬇️
unit-tests-macos-latest	`48.60% <16.36%> (-0.15%)`	⬇️
unit-tests-ubuntu-latest	`49.51% <16.36%> (-0.21%)`	⬇️
unit-tests-windows-latest	`48.48% <16.36%> (-0.18%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/docker/docker_client.go	`62.79% <16.36%> (-22.09%)`	⬇️

... 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.
📢 Do you have feedback about the report comment? Let us know in this issue.

riverar · 2023-03-16T06:01:40Z

pkg/docker/docker_client.go

+	tlsVerify, _ := strconv.ParseBool(tlsVerifyStr)
+	if !tlsVerify {
+		tlsOpts = append(tlsOpts, func(t *tls.Config) {
+			t.InsecureSkipVerify = true


Would delete this.

But docker CLI does this too.

Without this DOCKER_TLS_VERIFY=0 wouldn't have desired effect, right?

@lance @evankanderson wdyt?

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?

@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?

Agreed. (I continue to be extremely disappointed with Docker, esp. its impl. on Windows.)

@riverar Just try out docker --tlsverify=false image ls it will use https but will skip cert verification.

@matejvasek Ah ha! It does indeed. (That's a miserable mismatch in behavior, imo.)

riverar · 2023-03-15T23:04:29Z

@matejvasek Will check it out, thanks!

riverar

Verified func now works in (my) TLS scenarios 🎉

riverar · 2023-03-16T06:01:40Z

pkg/docker/docker_client.go

+	tlsVerify, _ := strconv.ParseBool(tlsVerifyStr)
+	if !tlsVerify {
+		tlsOpts = append(tlsOpts, func(t *tls.Config) {
+			t.InsecureSkipVerify = true


Would delete this.

pkg/docker/docker_client.go

knative-prow · 2023-03-16T06:06:20Z

@riverar: changing LGTM is restricted to collaborators

In response to this:

Verified func now works in (my) TLS scenarios 🎉

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.

matejvasek · 2023-03-16T13:54:52Z

@zroubalik @lance PTAL

Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-03-16T14:04:37Z

I should add some tests in case of future regression.

lkingland

/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 :)

knative-prow · 2023-03-16T16:01:51Z

[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:

~~OWNERS~~ [lkingland,matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fix: TCP+TLS docker daemon connection

eb0853d

Signed-off-by: Matej Vasek <mvasek@redhat.com>

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2023

knative-prow bot requested review from maximilien and rhuss March 15, 2023 21:37

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 15, 2023

matejvasek requested review from lance, lkingland and zroubalik and removed request for maximilien and rhuss March 15, 2023 21:38

github-advanced-security bot found potential problems Mar 15, 2023

View reviewed changes

riverar approved these changes Mar 16, 2023

View reviewed changes

fixup: skip tls verify if DOCKER_TLS_VERIFY is falsy

4b385c8

Signed-off-by: Matej Vasek <mvasek@redhat.com>

lkingland approved these changes Mar 16, 2023

View reviewed changes

knative-prow bot assigned lkingland Mar 16, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 16, 2023

knative-prow bot merged commit db1b624 into knative:main Mar 16, 2023

matejvasek mentioned this pull request Mar 16, 2023

Does func support TLS-enabled docker hosts? #1627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: TCP+TLS docker daemon connection #1629

fix: TCP+TLS docker daemon connection #1629

matejvasek commented Mar 15, 2023 •

edited

Loading

matejvasek commented Mar 15, 2023

knative-prow bot commented Mar 15, 2023

matejvasek commented Mar 15, 2023

codecov bot commented Mar 15, 2023 •

edited

Loading

riverar Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023

matejvasek Mar 16, 2023

matejvasek Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023 •

edited

Loading

riverar Mar 16, 2023

matejvasek Mar 16, 2023

riverar Mar 16, 2023 •

edited

Loading

riverar commented Mar 15, 2023

riverar left a comment

riverar Mar 16, 2023 •

edited

Loading

knative-prow bot commented Mar 16, 2023

matejvasek commented Mar 16, 2023

matejvasek commented Mar 16, 2023

lkingland left a comment •

edited

Loading

knative-prow bot commented Mar 16, 2023

fix: TCP+TLS docker daemon connection #1629

fix: TCP+TLS docker daemon connection #1629

Conversation

matejvasek commented Mar 15, 2023 • edited Loading

Changes

matejvasek commented Mar 15, 2023

knative-prow bot commented Mar 15, 2023

matejvasek commented Mar 15, 2023

codecov bot commented Mar 15, 2023 • edited Loading

Codecov Report

riverar Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

matejvasek Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

matejvasek Mar 16, 2023

Choose a reason for hiding this comment

matejvasek Mar 16, 2023

Choose a reason for hiding this comment

matejvasek Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

matejvasek Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

riverar Mar 16, 2023

Choose a reason for hiding this comment

matejvasek Mar 16, 2023

Choose a reason for hiding this comment

riverar Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

riverar commented Mar 15, 2023

riverar left a comment

Choose a reason for hiding this comment

riverar Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

knative-prow bot commented Mar 16, 2023

matejvasek commented Mar 16, 2023

matejvasek commented Mar 16, 2023

lkingland left a comment • edited Loading

Choose a reason for hiding this comment

knative-prow bot commented Mar 16, 2023

matejvasek commented Mar 15, 2023 •

edited

Loading

codecov bot commented Mar 15, 2023 •

edited

Loading

riverar Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023 •

edited

Loading

matejvasek Mar 16, 2023 •

edited

Loading

riverar Mar 16, 2023 •

edited

Loading

riverar Mar 16, 2023 •

edited

Loading

lkingland left a comment •

edited

Loading