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

Change init of consul client to respect TLS #348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mxab
Copy link

@mxab mxab commented Aug 4, 2023

This PR changes the initialisation of the consul client.

As during the creation of a new consul client a lot of environment variables are taken as fallback default configs, passing in a own HttpClient prevents on picking the consul specific TLS environment variables.

Instead of passing in the complete new HttpClient it only passes the transport and sets the timeout on the HttpClient afterwards

Fixes #346

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

@bboreham
Copy link
Contributor

bboreham commented Aug 8, 2023

Can you be a little more specific about "a lot of environment variables"?

After this PR the only comment is pointing to a justification for something rather different; can you add one on why the timeout is post-initialization?

@mxab
Copy link
Author

mxab commented Aug 9, 2023

Sorry for the confusing PR comment and I can also understand that the timeout parameters post-init looks strange :)

I'm trying to use loki with our consul which uses mTLS with self signed certificates.

I saw that you are using the official consul client to work with the KV store.

When using consul's NewClient(...

client, err := consul.NewClient(&consul.Config{
  Address: cfg.Host,
  Token:   cfg.ACLToken.String(),
  Scheme:  "http",
  HttpClient: &http.Client{
	  Transport: cleanhttp.DefaultPooledTransport(),
	  // See https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
	  Timeout: cfg.HTTPClientTimeout,
  },
})

the init method sets already a lot of defaults it detects from environment variables if not explicitly provided as init parameters.

Some environment variables that would take already affect would be e.g. CONSUL_NAMESPACE , CONSUL_HTTP_TOKEN_FILE or even CONSUL_HTTP_ADDR, CONSUL_HTTP_TOKEN if the passed arguments would be empty ("")

The TLS related variables (CONSUL_CACERT, CONSUL_CLIENT_CERT, ...) are unfortunatelly not taken into account as they're skipped when a custom http client is passed in.

If you pass in the transport instead of the client, they would be used as far as I see it, hence the PR to align the usage of env vars.

The only downside is that that you would have to "sneak" in the timeout parameter after the init, which I fully understand looks very wired.
Preferably I would like to change consul clients parameters to pass in the timeout directly, but I thought I try this one first :)

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.

consider consul with TLS
3 participants