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

Set User-Agent header on Consul API calls #434

Merged
merged 1 commit into from
Feb 8, 2021
Merged

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Feb 4, 2021

  • Upgrade to latest consul api package
  • Create common consul package that implements NewClient(). This
    function returns a Consul client that will set the header
    User-Agent: consul-k8s/<version> on all calls to Consul.
  • Swap to using this new constructor everywhere.
  • Add a linter that ensures we use our NewClient() function everywhere
    in the future. Cribbed from https://github.com/hashicorp/lint-consul-retry.

How I've tested this PR:

  • unit test

  • ran a request printer: docker run -p 8501:8443 --rm -t mendhak/http-https-echo:17

    then ran CONSUL_HTTP_SSL_VERIFY=false consul-k8s get-consul-client-ca -output-file /tmp/out -server-addr localhost -server-port 8501

    "GET /v1/agent/connect/ca/roots HTTP/1.1" 200 458 "-" "consul-k8s/0.23.0-dev"
    -----------------
    {
        "path": "/v1/agent/connect/ca/roots",
        "headers": {
            "host": "localhost:8501",
            "user-agent": "consul-k8s/0.23.0-dev",
            "accept-encoding": "gzip"
        },
        "method": "GET",
        "body": "",
        "fresh": false,
        "hostname": "localhost",
    

How I expect reviewers to test this PR:

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@lkysow lkysow force-pushed the version-headers branch 2 times, most recently from 516256f to 529e291 Compare February 4, 2021 18:11
* Upgrade to latest consul api package
* Create common consul package that implements NewClient(). This
  function returns a Consul client that will set the header
  "Consul-K8s-Version" to the current consul-k8s version on all calls to
  Consul.
* Swap to using this new constructor everywhere.
* Add a linter that ensures we use our NewClient() function everywhere
  in the future.
@lkysow lkysow marked this pull request as ready for review February 4, 2021 18:22
@lkysow lkysow requested review from a team, ndhanushkodi and kschoche and removed request for a team February 4, 2021 18:23
consul/consul.go Show resolved Hide resolved
@lkysow lkysow changed the title Set Consul-K8s-Version header on Consul API calls Set User-Agent header on Consul API calls Feb 4, 2021
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

This is great stuff, and I like the linter too!
Tested it out and looks good.
I left one comment which isn't a blocker but I'd like to get other opinions on whether or not we should keep using DefaultConfig() from API or duplicate it inside the new lib from a code readability standpoint.
So cool

consul-k8s % go run hack/lint-api-new-client/main.go
Found code using github.com/hashicorp/consul/api.NewClient()
instead of github.com/hashicorp/consul-k8s/consul.NewClient()
in the following files:
-  github.com/hashicorp/consul-k8s/connect-inject/health_check_resource.go
exit status 1

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Looks great!!

connect-inject/health_check_resource.go Show resolved Hide resolved
@lkysow lkysow merged commit 431bc8b into master Feb 8, 2021
@lkysow lkysow deleted the version-headers branch February 8, 2021 18:40
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
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.

None yet

5 participants