daemon/logger: remove uses of pkg/urlutil, fix fluentd validation and parsing by thaJeztah · Pull Request #43476 · moby/moby

thaJeztah · 2022-04-10T13:23:21Z

daemon/logger/gelf: remove uses of pkg/urlutil.IsTransportURL()

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts.

This patch:

removes a redundant use of urlutil.IsTransportURL(); code further below already checked if the given scheme (protocol) was supported.
renames some variables that collided with imported packages.

daemon/logger/splunk: remove uses of pkg/urlutil.IsURL()

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts.

This patch removes the use of urlutil.IsURL(), in favor of just checking if the provided scheme (protocol) is supported.

daemon/logger/syslog: remove uses of pkg/urlutil.IsTransportURL()

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts.

This patch:

removes a redundant use of urlutil.IsTransportURL(); instead adding some code to check if the given scheme (protocol) is supported.
define a defaultPort const for the default port.
use net.JoinHostPort() instead of string concatenating, to account for possible issues with IPv6 addresses.
renames a variable that collided with an imported package.

daemon/logger/fluentd: add coverage for ValidateLogOpt(), parseAddress()

This exposed a bug where host is ignored on some valid cases (to be fixed).

daemon/logger/fluentd: rename var that collided with import

daemon/logger/fluentd: make error-handling less DRY

daemon/logger/fluentd: validate path element

fix some missing validation: the driver was silently ignoring path elements in some cases, and expecting a host (not an URL), and for unix sockets did not validate if a path was specified.

For the latter case, we should have a fix in the upstream driver, as it uses an empty path as default path for the socket (defaultSocketPath), and performs no validation.

daemon/logger/fluentd: fix missing host, remove urlutil.IsTransportURL()

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts.

This patch:

fix some cases where the host was ignored for valid addresses.
removes a redundant use of urlutil.IsTransportURL(); instead adding code to check if the given scheme (protocol) is supported.
improve port validation for out of range ports.
fix some missing validation: the driver was silently ignoring path elements, but expected a host (not an URL)

daemon/logger/fluentd: remove udp, tcp+tls, unixgram, add tls scheme

unix and unixgram were added in cb176c8 (#26088), but at the time, the driver only supported "tcp" and "unix": see vendor/src/github.com/fluent/fluent-logger-golang/fluent/fluent.go#L243-L2610

support for tls was added in github.com/fluent/fluent-logger-golang v1.8.0, which was vendored in e24d61b (#42979).

the list of currently supported schemes by the driver is: tcp, tls and unix: see vendor/github.com/fluent/fluent-logger-golang/fluent/fluent.go#L435-L463

samuelkarp

A few nits/suggestions, but LGTM otherwise.

daemon/logger/gelf/gelf.go

daemon/logger/fluentd/fluentd_test.go

daemon/logger/fluentd/fluentd.go

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. This patch: - removes a redundant use of urlutil.IsTransportURL(); code further below already checked if the given scheme (protocol) was supported. - renames some variables that collided with imported packages. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. This patch removes the use of urlutil.IsURL(), in favor of just checking if the provided scheme (protocol) is supported. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. This patch: - removes a redundant use of urlutil.IsTransportURL(); instead adding some code to check if the given scheme (protocol) is supported. - define a `defaultPort` const for the default port. - use `net.JoinHostPort()` instead of string concatenating, to account for possible issues with IPv6 addresses. - renames a variable that collided with an imported package. - improves test coverage, and moves an integration test. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This exposed a bug where host is ignored on some valid cases (to be fixed). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

fix some missing validation: the driver was silently ignoring path elements in some cases, and expecting a host (not an URL), and for unix sockets did not validate if a path was specified. For the latter case, we should have a fix in the upstream driver, as it uses an empty path as default path for the socket (`defaultSocketPath`), and performs no validation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

pkg/urlutil (despite its poorly chosen name) is not really intended as a generic utility to handle URLs, and should only be used by the builder to handle (remote) build contexts. This patch: - fix some cases where the host was ignored for valid addresses. - removes a redundant use of urlutil.IsTransportURL(); instead adding code to check if the given scheme (protocol) is supported. - improve port validation for out of range ports. - fix some missing validation: the driver was silently ignoring path elements, but expected a host (not an URL) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

unix and unixgram were added in cb176c8, but at the time, the driver only supported "tcp" and "unix": https://github.com/moby/moby/blob/cb176c848e0731f77fa48b4e1a90ae74d1f2deae/vendor/src/github.com/fluent/fluent-logger-golang/fluent/fluent.go#L243-L261 support for tls was added in github.com/fluent/fluent-logger-golang v1.8.0, which was vendored in e24d61b. the list of currently supported schemes by the driver is: tcp, tls and unix: https://github.com/moby/moby/blob/5179299b98a37ac2ba6eb8aefe8bf211f33f7dcc/vendor/github.com/fluent/fluent-logger-golang/fluent/fluent.go#L435-L463 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

tianon

👍

tianon · 2022-04-11T18:34:45Z

daemon/logger/gelf/gelf.go

-		return nil, fmt.Errorf("gelf-address should be in form proto://address, got %v", address)
-	}
-	url, err := url.Parse(address)
+	addr, err := url.Parse(address)


Hmm, are there cases where url.Parse might infer or assign a scheme that we didn't intend? The check in urlutil.IsTransportURL is prefix-based, so should be pretty performant (and feels safer than trusting url.Parse, IMO).

I guess I'd be more worried about it inferring file:// than udp:// or tcp://, so this is probably fine. 🙈

I think we should be fine; couple of lines down we check if the scheme is supported;

if addr.Scheme != "udp" && addr.Scheme != "tcp" {

I can't come up with cases where it could invalidate a valid address (at most one that's specified without scheme, but that should produce an error message, helping the user to find the problem)

thaJeztah · 2022-04-11T19:51:20Z

Thanks for review! I'll bring this one in and rebase #43477 (moving it out of draft)

thaJeztah added area/logging status/2-code-review area/daemon Core Engine kind/bugfix PR's that fix bugs kind/refactor PR's that refactor, or clean-up code labels Apr 10, 2022

thaJeztah added this to the 22.04.0 milestone Apr 10, 2022

thaJeztah force-pushed the split_urlutil branch 2 times, most recently from 743a528 to 305c087 Compare April 10, 2022 15:09

This comment was marked as outdated.

Sign in to view

thaJeztah force-pushed the split_urlutil branch from 305c087 to e16baba Compare April 10, 2022 18:16

thaJeztah mentioned this pull request Apr 10, 2022

pkg/urlutil: deprecate, and move to builder/remotecontext/urlutil #43477

Merged

thaJeztah force-pushed the split_urlutil branch 2 times, most recently from 59cf6b3 to 228c2c0 Compare April 10, 2022 21:34

samuelkarp approved these changes Apr 10, 2022

View reviewed changes

daemon/logger/gelf/gelf.go Show resolved Hide resolved

daemon/logger/fluentd/fluentd_test.go Show resolved Hide resolved

daemon/logger/fluentd/fluentd.go Show resolved Hide resolved

daemon/logger/fluentd/fluentd.go Show resolved Hide resolved

thaJeztah added 9 commits April 11, 2022 17:42

daemon/logger/fluentd: add coverage for ValidateLogOpt(), parseAddress()

4018295

This exposed a bug where host is ignored on some valid cases (to be fixed). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/logger/fluentd: rename var that collided with import

0dd2b4d

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/logger/fluentd: make error-handling less DRY

b161616

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the split_urlutil branch from 228c2c0 to 3e47a75 Compare April 11, 2022 16:03

tianon approved these changes Apr 11, 2022

View reviewed changes

thaJeztah merged commit 6f8bc7e into moby:master Apr 11, 2022

thaJeztah deleted the split_urlutil branch April 11, 2022 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon/logger: remove uses of pkg/urlutil, fix fluentd validation and parsing#43476

daemon/logger: remove uses of pkg/urlutil, fix fluentd validation and parsing#43476
thaJeztah merged 9 commits intomoby:masterfrom
thaJeztah:split_urlutil

thaJeztah commented Apr 10, 2022 •

edited

Loading

Uh oh!

This comment was marked as outdated.

samuelkarp left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianon left a comment

Uh oh!

tianon Apr 11, 2022

Uh oh!

thaJeztah Apr 11, 2022

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thaJeztah commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

daemon/logger/gelf: remove uses of pkg/urlutil.IsTransportURL()

daemon/logger/splunk: remove uses of pkg/urlutil.IsURL()

daemon/logger/syslog: remove uses of pkg/urlutil.IsTransportURL()

daemon/logger/fluentd: add coverage for ValidateLogOpt(), parseAddress()

daemon/logger/fluentd: rename var that collided with import

daemon/logger/fluentd: make error-handling less DRY

daemon/logger/fluentd: validate path element

daemon/logger/fluentd: fix missing host, remove urlutil.IsTransportURL()

daemon/logger/fluentd: remove udp, tcp+tls, unixgram, add tls scheme

Uh oh!

This comment was marked as outdated.

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

tianon Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Apr 10, 2022 •

edited

Loading