feat(inputs.http_listener_v2): Add unix socket mode #15764

bazko1 · 2024-08-21T13:18:31Z

Summary

Following changes add two new options to http_listener_v2 -> network default tcp can be unix which results in service_address interpreted as unix socket path that will be created and listened on by HTTP server.
socket_mode - for editing socket file permission settings, by default "" can be for example 644 only available for network: "unix"

Why:
I would like to have possibility of writing HTTP messages over unix socket to push messages to telegraf using libcurl based tools. This allows me to limit access to writing/reading messages with built in users/groups based mechanisms instead of trying to limit access via some firewall based options. More explanation can be found in linked issue.

Checklist

No AI generated code was used in this PR

Related issues

resolves #15730

Considerations

For now I added single unit test for unix network case I could instead extend some of the existing ones to be something like:

var networkCases = [2]string{"tcp", "unix"}
func TestSomething(){
for _, net := range networkCases {
               t.Run(net, func(
                       t *testing.T,
               ) {  ... test scenario for network ... }
}

I was thinking about directly using Socket from socket_listener but its listener field is made private and I decided to not change that thus some of the code is basically copied from its implementation for example the SocketMode part.

telegraf-tiger · 2024-08-21T13:18:38Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

bazko1 · 2024-08-21T13:30:03Z

!signed-cla

powersj · 2024-08-21T13:33:39Z

!signed-cla

srebhan

Thanks for your contribution @bazko1! I do have some smaller comments in the code, please take a look!

plugins/inputs/http_listener_v2/sample.conf

plugins/inputs/http_listener_v2/http_listener_v2_test.go

srebhan

Thanks @bazko1, the code looks really good. Just some more small comments...

plugins/inputs/http_listener_v2/README.md

srebhan · 2024-08-28T07:05:31Z

plugins/inputs/http_listener_v2/http_listener_v2.go

+	if runtime.GOOS == "windows" && strings.HasPrefix(h.ServiceAddress, "unix://") {
+		u = &url.URL{Scheme: "unix", Path: strings.TrimPrefix(h.ServiceAddress, "unix://")}
+	} else {


Is this really necessary? Parsing a unix URL should also work on Windows, shouldn't it?

Yes it was problematic as sample address will look like unix://c:/Users/bazko1/temp/telegraf.socket the url.Parse starts to interpret the part after c: as port and it results in an parse error.

You can check that in socket_listener tests for windows are skipped with comment that the unixgram is not supported even though the unix socket is.
See: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/socket_listener/socket_listener_test.go#L125-L128

I think the correct way is to use unix:///C:/Users/bazko1/... (i.e. three slashes) according to golang/go@844b625...

Yes great I tested it and this works I will remove the windows case then.

plugins/inputs/http_listener_v2/http_listener_v2.go

srebhan · 2024-08-28T07:12:55Z

plugins/inputs/http_listener_v2/http_listener_v2_test.go

+func TestServiceAddressURL(t *testing.T) {
+	unixSocket := filepath.FromSlash(os.TempDir() + "/test.sock")
+	cases := []struct {
+		serviceAddress, expectedAddress string
+		expectedPort                    int
+		shouldError                     bool
+	}{
+		{":8080", "[::]:8080", 8080, false},
+		{"localhost:4123", "127.0.0.1:4123", 4123, false},
+		{"tcp://localhost:4321", "127.0.0.1:4321", 4321, false},
+		{"127.0.0.1:9443", "127.0.0.1:9443", 9443, false},
+		{"tcp://127.0.0.1:8443", "127.0.0.1:8443", 8443, false},
+		{"tcp://:8443", "[::]:8443", 8443, false},
+		// port not provided
+		{"8.8.8.8", "", 0, true},
+		{"unix://" + unixSocket, unixSocket, 0, false},
+		// wrong protocol
+		{"notexistent:///tmp/test.sock", "", 0, true},
+	}
+	for _, c := range cases {
+		listener, err := newTestHTTPListenerV2()
+		require.NoError(t, err)
+		listener.ServiceAddress = c.serviceAddress
+		err = listener.Init()
+		require.Equal(t, c.shouldError, err != nil, "Init returned wrong error result error is: %q", err)
+		if c.shouldError {
+			continue
+		}
+		acc := &testutil.Accumulator{}
+		require.NoError(t, listener.Start(acc))
+		require.Equal(t, c.expectedAddress, listener.listener.Addr().String())
+		require.Equal(t, c.expectedPort, listener.Port)
+		listener.Stop()
+	}
+}
+


This test is problematic in CI environments. Imagine another test using the same port, if they by chance run at the same time one of them will fail and we get flaky tests. I would remove the test here or solely stick to the URL parsing part without actually "starting" the listener...

Yes I agree such test might be problematic when running in parallel with others, but afaik they are run synchronously and there are also other tests that actually, start listening on port.
Anyway my logic should be tested enough without Start as it is located in Init function so I will remove the Start method call.

Well actually the Init already calls the net.Listen so that the port gets busy and we need to stop the internal dial.Listener.

I think my changes are big enough especially with special windows case that having unit test for url parsing would be nice so please reconsider the idea of fully removing test, but if you insist I will remove.

Well starting the listener in Init() is also wrong, this should be done in Start()... Anyhow, as you didn't introduce this, I don't blame you. ;-)

Regarding the tests: Yes, the tests in this file/package are executes sequentially, but all packages are executed in parallel (in the best case)... So if e.g. outputs.http opens a server on this port we run into trouble... So it is acceptable to specify no port or port zero as this will allow the OS to choose a free port but the above will definitively bite us. Been there suffered that.

In any case, what you do is to test if url.Parse works I think and I guess we should assume this works at least as long as this test will open a fixed port... So either change the test/code to not starting listen or remove the fixed ports.

Okay lets make it right then I agree that the test can be actually removed especially when windows unix path works fine with url.Parse.
I will move the .Listen part to Start then.

Done in 45792b5

telegraf-tiger · 2024-08-28T14:31:50Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

👍 This pull request doesn't change the Telegraf binary size

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Thanks for the nice PR @bazko1!

feat(inputs.http_listener_v2): add unix socket mode

06bfdea

bazko1 force-pushed the addUnixSocketsListenerV2 branch from 11bf51e to 06bfdea Compare August 21, 2024 13:28

bazko1 changed the title ~~feat(inputs.http_listener_v2) Add unix socket mode~~ feat(inputs.http_listener_v2): add unix socket mode Aug 21, 2024

telegraf-tiger bot added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Aug 21, 2024

close temp file to avoid windows issue

7cf3f65

srebhan reviewed Aug 26, 2024

View reviewed changes

plugins/inputs/http_listener_v2/sample.conf Outdated Show resolved Hide resolved

plugins/inputs/http_listener_v2/http_listener_v2_test.go Outdated Show resolved Hide resolved

plugins/inputs/http_listener_v2/http_listener_v2_test.go Outdated Show resolved Hide resolved

srebhan self-assigned this Aug 26, 2024

srebhan changed the title ~~feat(inputs.http_listener_v2): add unix socket mode~~ feat(inputs.http_listener_v2): Add unix socket mode Aug 26, 2024

bazko1 and others added 8 commits August 26, 2024 18:20

revert style changes

20c253d

remove network parameter and handle protocol in address instead

17eb6f5

add more samples to readme

c2b9157

add more samples to sample config too

95083f6

fix windows unix path by adding special case

949e4a2

switch port to satisfy mac

b958236

looks like address has to exists for mac

1427d39

replace sprintf with concat

bbad579

bazko1 requested a review from srebhan August 27, 2024 12:24

srebhan reviewed Aug 28, 2024

View reviewed changes

bazko1 added 2 commits August 28, 2024 10:26

double has conf comment to follow convention

a239d9e

deprecate port option

dfbaf83

bazko1 requested a review from srebhan August 28, 2024 09:17

move listen and socket permission changing to start, rm redundant test

45792b5

srebhan approved these changes Aug 29, 2024

View reviewed changes

srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 29, 2024

srebhan assigned DStrand1 Aug 29, 2024

srebhan removed their assignment Aug 29, 2024

DStrand1 approved these changes Sep 4, 2024

View reviewed changes

DStrand1 merged commit 0b4f77d into influxdata:master Sep 4, 2024
27 checks passed

github-actions bot added this to the v1.32.0 milestone Sep 4, 2024

BrewTestBot mentioned this pull request Sep 9, 2024

telegraf 1.32.0 Homebrew/homebrew-core#184076

Merged

asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024

feat(inputs.http_listener_v2): Add unix socket mode (influxdata#15764)

d3f45e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(inputs.http_listener_v2): Add unix socket mode #15764

feat(inputs.http_listener_v2): Add unix socket mode #15764

bazko1 commented Aug 21, 2024

telegraf-tiger bot commented Aug 21, 2024

bazko1 commented Aug 21, 2024

powersj commented Aug 21, 2024

srebhan left a comment

srebhan left a comment

srebhan Aug 28, 2024

bazko1 Aug 28, 2024

srebhan Aug 28, 2024

bazko1 Aug 28, 2024

srebhan Aug 28, 2024

bazko1 Aug 28, 2024

bazko1 Aug 28, 2024

srebhan Aug 28, 2024

bazko1 Aug 28, 2024

bazko1 Aug 29, 2024

telegraf-tiger bot commented Aug 28, 2024

Artifact URLs

srebhan left a comment

feat(inputs.http_listener_v2): Add unix socket mode #15764

feat(inputs.http_listener_v2): Add unix socket mode #15764

Conversation

bazko1 commented Aug 21, 2024

Summary

Checklist

Related issues

Considerations

telegraf-tiger bot commented Aug 21, 2024

bazko1 commented Aug 21, 2024

powersj commented Aug 21, 2024

srebhan left a comment

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

telegraf-tiger bot commented Aug 28, 2024

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment