Wildcard support for x509_cert files #6952

jaroug · 2020-01-29T17:07:49Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests. (as much as there is no existing ones)

Closes #6951

jaroug · 2020-01-29T17:28:46Z

Fixes #6951

plugins/inputs/x509_cert/x509_cert.go

ikkemaniac · 2020-07-25T15:37:54Z

Would this also allow to add a complete dir?

["/etc/certs/*"]

jaroug · 2020-07-25T17:51:55Z

Would this also allow to add a complete dir?
["/etc/certs/*"]

Yes :)

ikkemaniac · 2020-07-25T20:33:09Z

Would this also allow to add a complete dir?
["/etc/certs/*"]
Yes :)

AWEsome, let's hope this get's merged quickly.

I should be able to build it from 9309072 right?

jaroug · 2020-07-25T21:35:59Z

Would this also allow to add a complete dir?
["/etc/certs/*"]
Yes :)
AWEsome, let's hope this get's merged quickly.

I should be able to build it from 9309072 right?

I just rebased master, resolved the merge conflict, it works :)

ikkemaniac · 2020-07-26T14:01:25Z

Took a little bit to x-compile but i got r running. No strange things yet so thumbs up from me!

Thx bud!

MatthewGow · 2020-11-03T21:16:29Z

Wow, this is exactly what we were looking to do - great work @jaroug !

@danielnelson - is there anything holding up the review of this other than time?

VorobevPavel-dev · 2021-02-11T05:23:49Z

It’s sad that it’s been all year since PR was created. Maybe there is something new or you just forgot about it. Very useful functions and features cannot be added for a whole year. Please, take a look, @danielnelson, @jaroug

srebhan · 2021-02-12T08:36:36Z

plugins/inputs/x509_cert/x509_cert.go

+	var allFiles []string
+
+	for _, source := range c.Sources {
+		if strings.HasPrefix(source, "/") {


I think you should swap the order of processing and first call sourcesToURLs() and afterwards this function. This way you can be sure that file always have file:// prepended. Otherwise you might miss files where the user added file:// already for you.

Suggested change

if strings.HasPrefix(source, "/") {

if strings.HasPrefix(source, "file://") {

the globing tools don't work with url styled path, so I'll have to strip the leading file://

srebhan · 2021-02-12T08:38:44Z

plugins/inputs/x509_cert/x509_cert.go

+			if len(files) <= 0 {
+				return fmt.Errorf("could not find file: %v", source)
+			}
+			allFiles = append(allFiles, files...)


Please remember to prepend file:// again here after swapping functions.

srebhan · 2021-02-12T08:40:37Z

plugins/inputs/x509_cert/x509_cert.go

+	err := c.refreshFilePaths()
+	if err != nil {
+		return err
+	}
+
+	err = c.sourcesToURLs()
+	if err != nil {
+		return err
+	}
+


I think it's better to swap these functions so refreshFilePaths() (which might be called expandFilePaths()) can work on normalized inputs as otherwise you miss entries where the user specified files in the form file:///etc/certs/*.

srebhan

I suggest swapping the two functions in Init() and work on normalized URLs instead of relying on the user not specifying file:// prefixed with wildcards. See my comments in the code.

ssoroka

looks like it needs some merge conflicts resolved. Any additional test coverage would be great, but not a blocker.

sjwang90 · 2021-02-16T23:49:11Z

@jaroug If you can address the comments from @srebhan and @ssoroka we can do another review and look to get this into our next feature release. Thanks!

jaroug · 2021-02-17T08:58:38Z

@sjwang90 gonna have a look asap

srebhan

Looks good so far. Only one more question: You now moved the globbing to Init() which means you won't catch new certs and maybe will poke old (already deleted) certs. Is this by intention? Maybe it would make sense to take the extra cycles and update the cert-list in every gather-cycle?!?

jaroug · 2021-02-22T17:29:38Z

@srebhan it was requested by the previous person holding the PR: #6952 (comment)

srebhan · 2021-02-23T08:44:14Z

@jaroug almost, if you read the comment you cited carefully the idea was to prepare the glob-statement in Init() but call the Match() method in each gather cycle. Do you think this is doable to prepare the glob statement in Init() then call the Match() for the glob entries in Gather() and join the result with the static entries? This would mean to split the refreshFilePaths() function into two parts...

jaroug · 2021-02-23T09:08:25Z

@srebhan oh yeah ok, I totally misunderstood, my bad, I'll have a look asap

sjwang90 · 2021-03-02T18:01:02Z

Hi @jaroug, just checking on the status. We're hoping for a Telegraf 1.18 release candidate this week and would love to get this in.

jaroug · 2021-03-03T12:07:05Z

👋 I made those changes a bit in a hurry so it might be far from perfect but it seems to work. I haven't considered searching for globpath on "file://" inputs since I'm not sure it's supposed to be supported by the URL package, should I?

ikkemaniac · 2021-03-05T12:03:36Z

*sees 'ready to merge' tag

is this really happening ? 🤞

jaroug · 2021-03-08T17:32:09Z

*sees 'ready to merge' tag

is this really happening ?

Well hopefully, poke @sjwang90

srebhan · 2021-03-09T07:44:17Z

@jaroug and @ikkemaniac please stay calm and let the Influx guys do their job. It might take some more time due to the amount of PRs they have to handle and the way PRs are merged during the release cycle. So poking at this every second day takes away review resources (e.g. mine ;-))...

ssoroka · 2021-03-10T20:31:31Z

Looks good, but i'd like to see some tests. also looks like the existing tests are failing in windows.

ivorybilled · 2021-03-16T19:44:11Z

plugins/inputs/x509_cert/x509_cert.go

+		}
+		for _, file := range files {
+			file = "file://" + file
+			u, err := url.Parse(file)


Looks like this causes windows tests to fail, as it assumes the path is a linux-style path. On windows the path will start with C: or something and it will recognize the colon as a port. A workaround for windows is adding an extra slash to the file:// part but it looks like there might be other issues (see: golang/go#13276). I'm thinking maybe we'd just need to manually handle the windows case here, not 100% sure but we will need it to work on windows.

reimda · 2021-03-18T19:09:57Z

Hi @jaroug, are you able/willing to fix this windows path problem? If not, let us know and we'll try to find someone who can. This PR is useful and I'd hate to see it stagnate again after getting so close to being merged! Thanks!

* rename refreshFilePaths to expandFilePaths * expandFilePaths handles '/path/to/*.pem' and 'files:///path/to/*.pem' * update sample config

* add var globFilePathsToUrls to stack files path * add var globpaths to stack compiled globpath * rework sourcesToURLs to compile files path and stack them * rename expandFilePaths to expandFilePathsToUrls * rework expandFilePathsToUrls to only match compiled globpath * rework the `Gather` ticker to match globpath at each call

* add specifics regarding relative paths in sample config * add logger and use it in expandFilePathsToUrls() * precompile glob for `files://`, `/` and `://`

* rename expandFilePathsToUrls() to collectCertURLs() * collectCertURLs() now returns []*url.URL to avoid extra field globFilePathsToUrls in structure * update the Gather() ticker accordingly

…not supposed to be supported by the OS

telegraf-tiger

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

telegraf-tiger

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

* Accept standard unix glob matching rules * comply with indentation * update readme * move globing expand and url parsing into Init() * chore: rebase branch on upstream master * rename refreshFilePaths to expandFilePaths * expandFilePaths handles '/path/to/*.pem' and 'files:///path/to/*.pem' * update sample config * fix: recompile files globing pattern at every gather tic * add var globFilePathsToUrls to stack files path * add var globpaths to stack compiled globpath * rework sourcesToURLs to compile files path and stack them * rename expandFilePaths to expandFilePathsToUrls * rework expandFilePathsToUrls to only match compiled globpath * rework the `Gather` ticker to match globpath at each call * fix: comply with requested changes * add specifics regarding relative paths in sample config * add logger and use it in expandFilePathsToUrls() * precompile glob for `files://`, `/` and `://` * fix: update README to match last changes * fix: comply with last requested changes * rename expandFilePathsToUrls() to collectCertURLs() * collectCertURLs() now returns []*url.URL to avoid extra field globFilePathsToUrls in structure * update the Gather() ticker accordingly * fix(windows): do not try to compile glopath for windows path as it's not supposed to be supported by the OS * fix(ci): apply go fmt * fix(ci): empty-lines/import-shadowing Co-authored-by: Anthony LE BERRE <[email protected]>

aslgithub · 2021-06-16T17:05:32Z

This Pull seems to be the only change I can obviously find between v1.18.1 and v1.19.0 for x509_cert and I'm concerned it may have broken URL testing on Windows ?
#9384

jaroug mentioned this pull request Jan 29, 2020

wildcard (glob) support for files sources in plugins/inputs/x509_cert (PR included) #6951

Closed

danielnelson suggested changes Feb 6, 2020

View reviewed changes

plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved

plugins/inputs/x509_cert/x509_cert.go Outdated Show resolved Hide resolved

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Feb 6, 2020

danielnelson changed the title ~~Inputs/x509 cert~~ Wildcard support for x509_cert files Apr 21, 2020

jaroug requested a review from danielnelson May 13, 2020 10:12

ssoroka requested review from ssoroka and removed request for danielnelson February 11, 2021 20:21

srebhan reviewed Feb 12, 2021

View reviewed changes

srebhan requested changes Feb 12, 2021

View reviewed changes

srebhan self-assigned this Feb 12, 2021

ssoroka approved these changes Feb 12, 2021

View reviewed changes

jaroug force-pushed the inputs/x509_cert branch from 9309072 to ee52ee5 Compare February 20, 2021 17:18

srebhan reviewed Feb 22, 2021

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 Mar 4, 2021

ivorybilled reviewed Mar 16, 2021

View reviewed changes

srebhan removed 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 Mar 19, 2021

Anthony LE BERRE added 11 commits March 21, 2021 14:16

Accept standard unix glob matching rules

6b76e33

comply with indentation

3517dd3

update readme

f91d5ff

move globing expand and url parsing into Init()

d4614e6

chore: rebase branch on upstream master

528a3f2

* rename refreshFilePaths to expandFilePaths * expandFilePaths handles '/path/to/*.pem' and 'files:///path/to/*.pem' * update sample config

fix: comply with requested changes

4cce113

* add specifics regarding relative paths in sample config * add logger and use it in expandFilePathsToUrls() * precompile glob for `files://`, `/` and `://`

fix: update README to match last changes

0f1e7f1

fix: comply with last requested changes

60f6454

* rename expandFilePathsToUrls() to collectCertURLs() * collectCertURLs() now returns []*url.URL to avoid extra field globFilePathsToUrls in structure * update the Gather() ticker accordingly

fix(windows): do not try to compile glopath for windows path as it's …

4e23299

…not supposed to be supported by the OS

fix(ci): apply go fmt

73e4b2b

jaroug force-pushed the inputs/x509_cert branch from 2c66fc5 to 73e4b2b Compare March 21, 2021 13:16

telegraf-tiger bot reviewed Mar 21, 2021

View reviewed changes

fix(ci): empty-lines/import-shadowing

6730766

telegraf-tiger bot reviewed Mar 21, 2021

View reviewed changes

jessingrass merged commit b2b3613 into influxdata:master Mar 23, 2021

jjh74 mentioned this pull request May 21, 2021

inputs.x509_cert unexpected behavior with SNI #8914

Closed

This was referenced Jun 16, 2021

Issue 8914 x509sni #9289

Merged

x509 Certificate Input Plugin v1.19.0 does not work with URL #9384

Closed

srebhan mentioned this pull request Jun 18, 2021

Fix schema mess-up for x509 with https #9400

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wildcard support for x509_cert files #6952

Wildcard support for x509_cert files #6952

jaroug commented Jan 29, 2020 •

edited by sjwang90

Loading

jaroug commented Jan 29, 2020 •

edited by sjwang90

Loading

ikkemaniac commented Jul 25, 2020

jaroug commented Jul 25, 2020

ikkemaniac commented Jul 25, 2020

jaroug commented Jul 25, 2020

ikkemaniac commented Jul 26, 2020

MatthewGow commented Nov 3, 2020

VorobevPavel-dev commented Feb 11, 2021 •

edited

Loading

srebhan Feb 12, 2021

jaroug Feb 20, 2021

srebhan Feb 12, 2021

srebhan Feb 12, 2021

srebhan left a comment

ssoroka left a comment

sjwang90 commented Feb 16, 2021

jaroug commented Feb 17, 2021

srebhan left a comment

jaroug commented Feb 22, 2021

srebhan commented Feb 23, 2021

jaroug commented Feb 23, 2021 •

edited

Loading

sjwang90 commented Mar 2, 2021

jaroug commented Mar 3, 2021

ikkemaniac commented Mar 5, 2021

jaroug commented Mar 8, 2021

srebhan commented Mar 9, 2021

ssoroka commented Mar 10, 2021

ivorybilled Mar 16, 2021

reimda commented Mar 18, 2021

telegraf-tiger bot left a comment

telegraf-tiger bot left a comment

aslgithub commented Jun 16, 2021

	if strings.HasPrefix(source, "/") {
	if strings.HasPrefix(source, "file://") {

Wildcard support for x509_cert files #6952

Wildcard support for x509_cert files #6952

Conversation

jaroug commented Jan 29, 2020 • edited by sjwang90 Loading

Required for all PRs:

jaroug commented Jan 29, 2020 • edited by sjwang90 Loading

ikkemaniac commented Jul 25, 2020

jaroug commented Jul 25, 2020

ikkemaniac commented Jul 25, 2020

jaroug commented Jul 25, 2020

ikkemaniac commented Jul 26, 2020

MatthewGow commented Nov 3, 2020

VorobevPavel-dev commented Feb 11, 2021 • edited Loading

srebhan Feb 12, 2021

Choose a reason for hiding this comment

jaroug Feb 20, 2021

Choose a reason for hiding this comment

srebhan Feb 12, 2021

Choose a reason for hiding this comment

srebhan Feb 12, 2021

Choose a reason for hiding this comment

srebhan left a comment

Choose a reason for hiding this comment

ssoroka left a comment

Choose a reason for hiding this comment

sjwang90 commented Feb 16, 2021

jaroug commented Feb 17, 2021

srebhan left a comment

Choose a reason for hiding this comment

jaroug commented Feb 22, 2021

srebhan commented Feb 23, 2021

jaroug commented Feb 23, 2021 • edited Loading

sjwang90 commented Mar 2, 2021

jaroug commented Mar 3, 2021

ikkemaniac commented Mar 5, 2021

jaroug commented Mar 8, 2021

srebhan commented Mar 9, 2021

ssoroka commented Mar 10, 2021

ivorybilled Mar 16, 2021

Choose a reason for hiding this comment

reimda commented Mar 18, 2021

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

Artifact URLs

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

Artifact URLs

aslgithub commented Jun 16, 2021

jaroug commented Jan 29, 2020 •

edited by sjwang90

Loading

jaroug commented Jan 29, 2020 •

edited by sjwang90

Loading

VorobevPavel-dev commented Feb 11, 2021 •

edited

Loading

jaroug commented Feb 23, 2021 •

edited

Loading