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

Oauth2 support in HTTP output plugin #4536

Merged
merged 21 commits into from
Sep 6, 2018
Merged

Conversation

vikrant6
Copy link
Contributor

@vikrant6 vikrant6 commented Aug 9, 2018

Support for OAuth2 Authentication in http output plugin.
The golang client credentials librarty supports token reuse untill its expiry.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Support for OAuth2 Authentication in http output plugin.
The golang client credentials librarty supports token reuse untill its expiry.
@vikrant6 vikrant6 closed this Aug 9, 2018
@vikrant6 vikrant6 reopened this Aug 9, 2018
@vikrant6
Copy link
Contributor Author

fmt check done

@vikrant6
Copy link
Contributor Author

dep ensure -vendor-only fixed

Copy link
Contributor Author

@vikrant6 vikrant6 left a comment

Choose a reason for hiding this comment

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

fixed test errors

@vikrant6 vikrant6 closed this Aug 23, 2018
@vikrant6 vikrant6 reopened this Aug 23, 2018
@vikrant6 vikrant6 closed this Aug 23, 2018
@vikrant6 vikrant6 reopened this Aug 23, 2018
@vikrant6
Copy link
Contributor Author

test

@danielnelson danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 23, 2018
@danielnelson
Copy link
Contributor

@vikrant6 Thanks for the PR. I'll try to take a look soon, ping me when you have the tests passing on circleci or if you need help.

@vikrant6
Copy link
Contributor Author

@danielnelson I very new to go and might be missing something basic here. Need your help

go vet $(go list ./... | grep -v ./plugins/parsers/influx)
plugins/outputs/http/http.go:17:2: cannot find package "golang.org/x/oauth2" in any of:
/go/src/github.com/influxdata/telegraf/vendor/golang.org/x/oauth2 (vendor tree)
/usr/local/go/src/golang.org/x/oauth2 (from $GOROOT)
/go/src/golang.org/x/oauth2 (from $GOPATH)
plugins/outputs/http/http.go:18:2: cannot find package "golang.org/x/oauth2/clientcredentials" in any of:
/go/src/github.com/influxdata/telegraf/vendor/golang.org/x/oauth2/clientcredentials (vendor tree)
/usr/local/go/src/golang.org/x/oauth2/clientcredentials (from $GOROOT)
/go/src/golang.org/x/oauth2/clientcredentials (from $GOPATH)

@danielnelson
Copy link
Contributor

This error is due to the new dependency not being added to the lock file. You don't want to edit it by hand though, undo your changes to Gopkg.toml and run:

dep ensure -add golang.org/x/oauth2@master

Then make sure there are no errors when you run:

dep check

@vikrant6
Copy link
Contributor Author

@danielnelson Thanks. vet is failing for some packages (existing) Any idea how to proceed?

@vikrant6
Copy link
Contributor Author

@danielnelson - Any pointer on how to proceed from here?

@danielnelson
Copy link
Contributor

@vikrant6 Is it alright if I fix it up and push to your fork?

@vikrant6
Copy link
Contributor Author

@danielnelson - Please go ahead. Thanks.

@danielnelson
Copy link
Contributor

I fixed up the dep issue, cleaned up some whitespace changes, and switch the client over to using the oauth2.Client and added some tests. I made some minor tweaks to the config file variable names as well.

Here is the rpm from CircleCI telegraf-1.8.0~f6efa1a9-0.x86_64.rpm

@vikrant6
Copy link
Contributor Author

@danielnelson Thank you !

@vikrant6 vikrant6 closed this Aug 29, 2018
@vikrant6 vikrant6 reopened this Aug 29, 2018
@vikrant6
Copy link
Contributor Author

@danielnelson What's next for this to merge ? Is there a milestone identified?

@danielnelson danielnelson added this to the 1.8.0 milestone Aug 29, 2018
@danielnelson
Copy link
Contributor

We should be able to add this to 1.8. Since I don't have a real system to do a final testing, can you let me know if everything is working?

@vikrant6
Copy link
Contributor Author

@danielnelson Found that it no longer honors the configuration "insecure_skip_verify=true"
For self signed certs it errors out as "x509: certificate signed by unknown authority"
I can test the windows and linux version extensively here.

@danielnelson
Copy link
Contributor

I think this will fix it: c1dc2d0

Updated package: telegraf-1.8.0~c1dc2d06-0.x86_64.rpm

@danielnelson danielnelson merged commit 091af7e into influxdata:master Sep 6, 2018
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants