Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

add hostIP as env to server-statefulset and substitute env variables in extraConfig #1042

Merged
merged 12 commits into from
Jul 21, 2021

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Jul 16, 2021

Changes proposed in this PR:

  • Add hostIP as env variable to server statefulset
  • Copies the extraConfig configMap file to /consul/extra-config/extra-from-values.json and uses sed to replace HOST_IP/POD_IP/HOSTNAME at runtime with the environment.
  • Passes -config-file=/consul/extra-config/extra-from-values.json to server-statefulset and client-daemonset.

Resolves #657.

How I've tested this PR:
unit tests updated and pass, manually tested using the extra-env-args telemetry specified in issue #657 :

global:
  imageK8S: kschoche/consul-k8s-dev
  image: hashicorp/consul:1.10.0
  tls:
    enabled: true
  acls:
    manageSystemACLs: true
  metrics:
    enabled: true
controller:
  enabled: true
server:
  extraConfig: '{ "telemetry": [ { "disable_hostname": true, "dogstatsd_addr": "HOST_IP:8125", "dogstatsd_tags": ["env:weweq","service:consul","version:1.10.0"] } ] }'
  replicas: 1

And confirmed that the server and clients comes online with the changes and fails to come online with existing code.

How I expect reviewers to test this PR:
unit tests pass, code review.

Note to reviewers:

  1. Given this is not configurable and just becomes hard-coded text in server-statefulset and client-daemonset is there anything worth additionally testing in bats?
  2. In the current design consul will parse the config-file twice, once because it's in the configMap file /consul/config/extra-from-values.json and once again in the new one. Based on https://www.consul.io/docs/agent/options#_config_file merging should be "ok". I'm not sure this is guaranteed. If we are concerned about that, I can rename the configMap entry to extra-from-values.tmp and Consul will ignore it when processing /consul/config/ and only catch the one we modify. Thoughts?

Checklist:

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

@kschoche kschoche self-assigned this Jul 16, 2021
@kschoche kschoche added area/chart-only Related to changes that simply require yaml chart changes, e.g. exposing a new field bug Something isn't working labels Jul 16, 2021
@kschoche kschoche changed the title as hostIP as env to server-statefulset and substitute env variables in extraConfig add hostIP as env to server-statefulset and substitute env variables in extraConfig Jul 16, 2021
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

I looked at the comment regarding the testing. I think i agree with the complexity of it. I havent tested this PR, but if you have and it does solve the problem, even when the input json is complicated, this LGMT!!

Thanks for putting in the hard work on this one. It definitely was quite a wild journey for a PR!!

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks good!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Ashwin Venkatesh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/chart-only Related to changes that simply require yaml chart changes, e.g. exposing a new field bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for environment variable HOST_IP
3 participants