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: Clear error message when provided config is not a text file #11787

Merged
merged 6 commits into from
Sep 13, 2022

Conversation

sspaink
Copy link
Contributor

@sspaink sspaink commented Sep 12, 2022

resolve: #8753

The issue was providing a non text file to Telegraf as a config would cause the error Error parsing data: line 1: invalid TOML syntax which isn't that helpful. I couldn't find a good way to detect the type of a file without reading it, you could detect it by extension: https://pkg.go.dev/mime?utm_source=godoc#TypeByExtension but testing this on windows I wouldn't get a result for .toml. So I chose to use: https://pkg.go.dev/golang.org/x/tools/godoc/util?utm_source=godoc#IsText which will look at the contents of the file to determine if it is a text file.

This does mean that depending on the binary size it could be slow to print this error message, but hopefully the error message will help regardless. Because when providing a URL the request header is updated with req.Header.Add("Accept", "application/toml"), I thought this might suffice and the extra check if it is a text file shouldn't be necessary and its the servers responsibility. But open to consider this differently.

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Sep 12, 2022
Copy link
Contributor

@Hipska Hipska left a comment

Choose a reason for hiding this comment

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

when providing a URL the request header is updated with req.Header.Add("Accept", "application/toml"), I thought this might suffice and the extra check if it is a text file shouldn't be necessary and its the servers responsibility.

I agree with this, they will still receive the message that it isn't valid TOML.

@Hipska Hipska 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 Sep 13, 2022
@sspaink
Copy link
Contributor Author

sspaink commented Sep 13, 2022

Weird, the tests in Windows are failing in the CI because of this change but I can't reproduce it locally on a Windows machine (https://app.circleci.com/pipelines/github/influxdata/telegraf/12585/workflows/9895e703-3964-4ddf-ae2a-91aef8ecaab1/jobs/203622). I thought maybe it had to do with the encoding because util.IsTest requires utf8 but before failing it prints the mimetype correctly as text/plain; charset=utf-8.

Updated the logic to use the mimetype returned by http.DetectContentType as a potential alternative.

@sspaink sspaink merged commit b5f7ca4 into master Sep 13, 2022
@sspaink sspaink deleted the validconfig branch September 13, 2022 16:43
reimda pushed a commit that referenced this pull request Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration fix pr to fix corresponding bug 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.

Telegraf does not check if configuration file is valid
3 participants