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

fix(inputs.x509_cert): Fix Windows path handling #12629

Merged
merged 5 commits into from
Feb 13, 2023

Conversation

srebhan
Copy link
Contributor

@srebhan srebhan commented Feb 6, 2023

resolves #10580

This PR fixes the handling of Windows paths by directly creating the URL instead of using a fragile indirection parsing a hand-crafted string.

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 6, 2023
@srebhan 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 Feb 7, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you for your attention on this one. I am going to leave it to you to merge it, in case you want to wait for PR feedback. Otherwise, feel free to land it before the bug fix next week.

@powersj powersj assigned srebhan and unassigned powersj Feb 7, 2023
@jjh74
Copy link
Contributor

jjh74 commented Feb 8, 2023

@jjh74 so do you think we should not support "file://C:/Windows/Temp/test.pem" but "file:///C:/Windows/Temp/test.pem"? If so, please comment on the PR!

If "file://C:/Windows/Temp/test.pem" has worked before then it makes perfect sense to support it.(Probably best if both file://C:/ and file:///C:/ could work).

(AFAIK if you url.Parse file://C:/ with golang(https://go.dev/src/cmd/go/internal/web/url_windows.go) then go interprets file://C:/ as scheme=file, host=C, but I think your patch changes this from url.Parse to &url.URL (no parsing)).

@srebhan
Copy link
Contributor Author

srebhan commented Feb 8, 2023

Both should be supported now on Windows.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Feb 8, 2023

@srebhan srebhan requested a review from powersj February 9, 2023 10:08
@aarnaud
Copy link
Contributor

aarnaud commented Feb 11, 2023

So everything working, I tested these

sources = ["file:///Folder with Space/certs/github.crt"]
sources = ['file://C:\Folder with Space\certs\github.crt']
sources = ["file://C:\\Folder with Space\\certs\\github.crt"]
sources = ["/Folder with Space/certs/github.crt"]
sources = ["file://c:/Folder with Space/certs/github.crt"]
sources = ["file:///c:/Folder with Space/certs/git*.crt"]
sources = ["file://c:/Folder with Space/certs/git*.crt"]
sources = ["file:///Folder with Space/certs/git*.crt"]
sources = ["file:///C:/Folder with Space/certs/github.crt"]

@srebhan srebhan merged commit ff89b77 into influxdata:master Feb 13, 2023
@srebhan srebhan deleted the x509_issue_10580 branch February 13, 2023 08:22
powersj pushed a commit that referenced this pull request Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[[inputs.x509_cert]] not working with filepath on windows
4 participants