Skip to content
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

Add input plugin for OpenLDAP #2612

Closed
wants to merge 33 commits into from
Closed

Conversation

acobaugh
Copy link
Contributor

@acobaugh acobaugh commented Apr 3, 2017

This is a plugin to pull metrics out of cn=Monitor from a local or remote OpenLDAP server.

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@danielnelson
Copy link
Contributor

Can you add some unit tests?

@acobaugh
Copy link
Contributor Author

acobaugh commented Apr 7, 2017

Added unit tests. There is a very simple test function that mocks up a SearchRequest and feeds that in, and the integration tests connect to a super simple openldap container.

```
$ telegraf -config telegraf.conf -input-filter openldap -test --debug
* Plugin: inputs.openldap, Collection 1
> openldap,port=389,host=localhost,server=localhost abandon_operations_initiated=4,extended_operations_completed=125963,bytes_statistics=595939321,pdu_statistics=17028251,modify_operations_initiated=0,delete_operations_completed=0,compare_operations_completed=0,max_file_descriptors_connections=4096,unbind_operations_completed=7981688,extended_operations_initiated=125963,referrals_statistics=0,modify_operations_completed=0,delete_operations_initiated=0,bind_operations_completed=8115329,search_operations_completed=4385841,add_operations_completed=0,abandon_operations_completed=4,write_waiters=0,bind_operations_initiated=8115329,modrdn_operations_initiated=0,compare_operations_initiated=0,entries_statistics=4401128,read_waiters=1,current_connections=3,search_operations_initiated=4385842,modrdn_operations_completed=0,add_operations_initiated=0,total_connections=8147531,unbind_operations_initiated=7981688 1491189665000000000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all the values being added as floats? None of them look to be floatish (have a decimal in them).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the syntax for the monitorCounter attribute specifies a 32-bit integer. I'll go ahead and make that change.

TlsSkipverify bool
BindDn string
BindPassword string
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably support SSL as well (meaning LDAP over SSL on port 636).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what about providing a path to a custom CA cert, so that users running their own cert aren't forced to use tls_skipverify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't implement ldapS because it was never formally standardized, LDAPv3 doesn't specify anything regarding ldaps, only starttls, and go-ldap itself doesn't directly support ldaps.

I'll have to dig into the tls and x509 packages to see about adding a ca cert option.

return nil
}

func gatherSearchResult(sr *ldap.SearchResult, o *Openldap, acc telegraf.Accumulator) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has an error in the return signature, but it doesn't ever return an error, nor is the caller checking the error.

return nil
}

// Convert a DN to metric name, eg cn=Read,cn=Waiters,cn=Monitor to read_waiters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on reversing this? So that it's waiters_read? LDAP DNs are backwards, in that they go from most specific to least specific. The advantage of reversing this is that it allows grouping when sorted.
Meaning if you have cn=Write,cn=Waiters,..., cn=Read,cn=Waiters,..., cn=Write,cn=foo,..., and cn=Read,cn=foo,..., you end up with foo_read,foo_write,waiters_read,waiters_write instead of read_foo,read_waiters,write_foo,write_waiters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated having a map of DNs to field names instead. It does bother my OCD just a bit having read_waiters instead of waiters_read, but it also reads a little easier? If other plugins name their fields so they sort more naturally I can change this one to match that pattern.

@acobaugh
Copy link
Contributor Author

Added a tls_cacertificate option - not sure how best to do the integration test for that.
Field values are stored as ints now.
gatherSearchResults() returns nil.

I'm still a bit hesitant to support ldapS, but I will if there is consensus around it.

@danielnelson
Copy link
Contributor

@cobaugh Do you think the config can be made to look more like some the existing tls options? Here is an example:

// Path to CA file

@danielnelson
Copy link
Contributor

Is this a server setting?

@phemmer
Copy link
Contributor

phemmer commented May 31, 2017

Client and server. The documentation I copied was the server side. The client side documentation just says "see server side documentation".

@danielnelson
Copy link
Contributor

Sounds good, lets just do it like OpenLDAP.

@phemmer
Copy link
Contributor

phemmer commented May 31, 2017

Do we want to do LDAPS on the first release, or just make sure that we would be able to add support in the future without breaking compatibility?

@danielnelson
Copy link
Contributor

So long as we can add it in the future I think we are good.

@acobaugh
Copy link
Contributor Author

Again, sorry to drag this out, but I'm still confused what the consensus is on this. TLS_REQCERT has to do with verifying the server certificate in the client context, and nothing to do with whether we are doing LDAPS or LDAP+StartTLS. Normally LDAPS is requested with a URI of ldaps://, and StartTLS is requested with the -ZZ option. To me, that means having a boolean called 'starttls'.

If we want to take this conversation off of this issue to avoid cluttering it up I'm fine with that too.

Fwiw, grafana has two booleans for its ldap authentication, use_ssl and start_tls:
grafana/grafana#2441

use_ssl enables ssl, and if start_tls is True, then it does a StartTLS instead of DialTLS.

@danielnelson
Copy link
Contributor

What happens if the TLS_REQCERT option is set to demand, but the -ZZ option is not given? I think we will want to behave as closely as possible to the openldap client.

One common issue with using multiple booleans like grafana is that you could specify an invalid state combination, such as setting both start_tls and use_ssl to be true, or they could conflict with the url scheme which I think we will want to match openldap.

Don't worry having too much discussion here, this is the right place. Best to sort this out beforehand than get stuck with a config file we don't like.

@acobaugh
Copy link
Contributor Author

acobaugh commented Jun 26, 2017 via email

@danielnelson
Copy link
Contributor

The problem is that the options are mutually exclusive, since you would never do a tls connection and then issue a start_tls command. So Grafana must be giving more precedence to one of them, I assume use_ssl.

One issue with using ldap:// urls, if we do this I'm sure people will want to use all features of this url, and not understand that it is only meant to be an host + port. ldap://host:port/DN?attributes?scope?filter?extensions.

Also, what about unix sockets, are these commonly used?

How do you feel about a single option that holds either ssl/tls or start_tls?

@acobaugh
Copy link
Contributor Author

acobaugh commented Jun 27, 2017 via email

@danielnelson
Copy link
Contributor

So in Grafana, StartTLS doesn't do anything unless UseSSL is set, this seems a bit unexpected to me... the single option should resolve any confusion around this.

@acobaugh
Copy link
Contributor Author

Made ssl option a string, accepting "starttls" or "ldaps". Empty string disables encryption entirely. Any other value is an error. We also support ldapS now.

Also synced up with master and switched from HasIntField to HasInt64Field due to #2813

@danielnelson
Copy link
Contributor

Can you update the example output now that we are using integers?

@acobaugh
Copy link
Contributor Author

Example output and example config updated, as well as a couple of other minor issues fixed.

danielnelson pushed a commit that referenced this pull request Jul 21, 2017
@danielnelson
Copy link
Contributor

This has been merged for 1.4, github just doesn't realize it.

vlamug pushed a commit to vlamug/telegraf that referenced this pull request Jul 24, 2017
jeichorn pushed a commit to jeichorn/telegraf that referenced this pull request Jul 24, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants