feat(inputs.ldap): Support ldapi protocol - #17478
Conversation
telegraf-tiger
Bot
added
feat
0b91794 to
fb3f465
Compare
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Thanks @mistotebe for your contribution. I think we can simplify the handling of LDAPI and avoid touching the code that much. Furthermore, I think the new bind_mechanism feature should be in its own PR as we want only one feature in a PR...
|
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
|
Sorry @mistotebe for closing the PR due to inactivity. I added a comment above. However, this PR does many different things and IMO changes the code structure without an actual need. Please try to be as less intrusive as possible. If you need to change the code structure, please do this in a first PR so that each PR only does one thing! |
telegraf-tiger
Bot
removed
the
waiting for response
|
So my approach would be diff --git a/plugins/inputs/ldap/ldap.go b/plugins/inputs/ldap/ldap.go
index 7a69871ca..164ac98b7 100644
--- a/plugins/inputs/ldap/ldap.go
+++ b/plugins/inputs/ldap/ldap.go
@@ -7,6 +7,7 @@ import (
_ "embed"
"fmt"
"net/url"
+ "strings"
"time"
"github.com/go-ldap/ldap/v3"
@@ -49,6 +50,20 @@ func (l *LDAP) Init() error {
l.Server = "ldap://localhost:389"
}
+ // Handle LDAPI which contains the unix sockets in percent encoded form, so we need to
+ // unescape the path to allow the LDAP library to handle the server using the url package
+ if addr, isLdapi := strings.CutPrefix(l.Server, "ldapi://"); isLdapi {
+ path, rest, found := strings.Cut(addr, "/")
+ if found {
+ return fmt.Errorf("additional elements %q are not supported by this plugin", rest)
+ }
+ path, err := url.PathUnescape(path)
+ if err != nil {
+ return fmt.Errorf("parsing escaped path %q failed: %w", path, err)
+ }
+ l.Server = "ldapi://" + path
+ }
+
u, err := url.Parse(l.Server)
if err != nil {
return fmt.Errorf("parsing server failed: %w", err)
@@ -72,6 +87,8 @@ func (l *LDAP) Init() error {
u.Host = u.Host + ":636"
}
tlsEnable = true
+ case "ldapi":
+ u.Host = u.Path
default:
return fmt.Errorf("invalid scheme: %q", u.Scheme)
}
@@ -131,9 +148,9 @@ func (l *LDAP) Gather(acc telegraf.Accumulator) error {
func (l *LDAP) connect() (*ldap.Conn, error) {
var conn *ldap.Conn
switch l.mode {
- case "ldap":
+ case "ldap", "ldapi":
var err error
- conn, err = ldap.DialURL("ldap://" + l.Server)
+ conn, err = ldap.DialURL(l.mode + "://" + l.Server)
if err != nil {
return nil, err
}with the unit-test func TestLdapiURLHandling(t *testing.T) {
// Setup the plugin
plugin := &LDAP{
Server: "ldapi://%2ftmp%2fsocket",
}
require.NoError(t, plugin.Init())
// Test the resulting setting
require.Equal(t, "/tmp/socket", plugin.Server)
require.Equal(t, "/tmp/socket", plugin.host)
require.Empty(t, plugin.port)
}As you can see this is much less-intrusive... |
f7caa34 to
14e61a9
Compare
mistotebe
changed the title
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Just two small comments left @mistotebe...
| } | ||
| tlsEnable = false | ||
| case "ldapi": | ||
| tlsEnable = false |
There was a problem hiding this comment.
Doesn't need to be set to false as this is the default in Golang.
There was a problem hiding this comment.
I kept the structure identical with the original switch (e.g. "ldap" case also sets tlsEnable = false) is that not desired?
There was a problem hiding this comment.
I'm OK with keeping it as it is also done for ldap but we really shouldn't. Anyway, that's probably for another PR to remove both.
| - uri -- configured server URI | ||
| - server -- Server name or IP | ||
| - port -- Port used for connecting | ||
|
|
||
| Or with the ldapi scheme: | ||
|
|
||
| - uri -- configured server URI | ||
| - path -- Path used to connect |
There was a problem hiding this comment.
How about
| - uri -- configured server URI | |
| - server -- Server name or IP | |
| - port -- Port used for connecting | |
| Or with the ldapi scheme: | |
| - uri -- configured server URI | |
| - path -- Path used to connect | |
| - uri -- configured server URI | |
| - server -- Server name or IP (for non Unix socket) | |
| - port -- Port used for connecting (for non Unix socket) | |
| - path -- Path used to connect (for Unix socket) |
There was a problem hiding this comment.
How do we get hold of the host name if I wanted to put it in (unless I just put "localhost" in?). Also there is still no port in UNIX sockets, I could make it an empty string if that's what you prefer to it being absent.
There was a problem hiding this comment.
But if you prefer it has a value, then shouldn't "path" now have a value in the TCP case? Don't have any strong feelings either way, just trying to gauge what to put where.
There was a problem hiding this comment.
Did you read what's in the brackets after the text? ;-)
I'm just putting the "condition" in brackets after the tag instead of splitting everything into sections as you did.
There was a problem hiding this comment.
I did but the reds and greens confused me and so I misread the intent, sorry, I'll update the doc.
29e8c65 to
26afe3c
Compare
|
@mistotebe please revert 448d556 as this again is intrusive and has nothing to do with the PR title. If you want to cleanup the stuff, do it in a separate PR! ONE PR ONE THING! |
The current structure is spaghetti and adding LDAPI support without addressing that would make the code even harder to understand, I tried and you can too if you don't believe me. If it really is project policy not to include required cleanup in feature PRs, let me know and I will submit it as a separate one. |
|
There are 3 patches currently:
The alternative is to squash the last two commits together but that goes against every practice of drafting patch series for review/inclusion I've seen so far. Again you're the maintainer, so your rules, however since I'm starting to feel I'm not following them at all yet we seem to agree on the rough shape of the final code(?), please outline a path for me to where you're willing to accept a patch(set) that gets there and I'll follow it or state that no such path exists. |
|
@mistotebe don't get me wrong I'm not saying you cannot change the code, but you cannot modify code in a PR that is unrelated to that PR. If you need to cleanup code before adding a new feature then this should go to a separate PR! Let me reply in detail
This modifies existing metrics and is a no-go.
Exactly, you should add a Furthermore, your current patch is very intrusive, e.g. by changing the
I really don't see this! At #17478 (review) we were in a state where you only would need to change the documentation and remove the
My suggestion for a path forward is to revert to commit 14e61a9, remove the |
Now I'm confused, so let me talk about one thing at a time, starting with the one that really does it for me: To the best of my understanding, the above state you're referencing is the PR at commit 14e61a9 (2 hours before your review) which contains all the changes you also say are unacceptable? Am I missing something or are you referring to another message but put the wrong link? |
|
Part two:
Ok, that no metrics can even have new tags added to them is new to me and not clear from the documentation at all. I guess I'm OK to drop this completely, we are able to synthesise this tag in our own deployments from the data already exposed (except the actual protocol but that's a decision that can be baked into the processing at deployment time).
Hmm, you previously rejected me doing the very same thing in this conversation... I mean it makes more sense to set them up in
The point is it does nothing of the sort. It makes the intention clear (first connect over the right protocol, then set up TLS if requested and not already a layer that's set up as part of the protocol) without undue duplication and escaping things properly where needed (i.e. not just doing text manipulation to get a URL)? Since adding a new 'URI' tag is a no-go, I'm preparing a minimal patch that never has to deal with an actual URI, so it touch as little of the existing code as possible. Once go-ldap/ldap#564 is resolved and the module accepts the correct form, to finally support all possible ldapi:// URLs (relative paths are not connectable at the moment) you will probably do exactly as I did until now. |
|
I've done the minimal possible implementation now, removing everything else that I could, aiming for the smallest (textual) diff. Since it depends on (and includes a commit from) #17989, switching state to Draft to avoid conflicts/accidental merges. |
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
@mistotebe this looks very good to me. I maybe would have set the l.tags in the switch statement but I'm fine doing it this way.
Can you please move the PR out of draft so I can approve it!?
And sorry for the confusion around the tags! It came from my impression to use the server tag also for the ldapi part but having this in an extra tag makes a lot of sense!
This one is draft because #17989 needs to happen first right? |
|
Yes we should get #17989 in first... |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Thank you very much @mistotebe for keeping this going!
srebhan
added
the
ready for final review
skartikey
left a comment
skartikey
left a comment
There was a problem hiding this comment.
Nice work on this PR, @mistotebe!
This is a clean and well-thought-out implementation.
4 participants
Summary
Implements #17477 (enabling Unix socket support and EXTERNAL SASL binds)
Checklist
Related issues
based on #17989
resolves #17477