feature - [NET - 4005] - [Supportability] Reloadable Configuration - enable_debug

[absolutelightning]

Description

Making enable_debug a Reloadable Configuration

Summary of customer problem and expectations:
In order to capture pprof to analyze Golang heap, CPU, goroutine, and trace profiling "enable_debug" must be set in the configuration and for it to take effect, Consul process must be restarted. Thereafter the "consul debug" command with the appropriate flags can be run to capture the pprof. Having the ability to apply this update to the configuration through "consul reload" would ensure cluster stability.

What currently happens or is possible:
Currently the Consul process must be restarted for this configuration to take effect.

In this PR - I am making enable_debug a reloadable configuration and adding http handlers before the enableDebug flag check. Earlier we use to check for enable debug and then add http handlers. Now I am adding http handlers and checking for enableDebug and checkACLDisabled() inside the handler when the handler is already attached.

Testing & Reproduction steps

Replicate the issue by -

• Git clone this repo
• Run make dev
• create a config file server.json

{
  "node_name": "consul-server",
  "server": true,
  "bootstrap": true,
  "ui_config": {
    "enabled": true
  },
  "datacenter": "dc1",
  "data_dir": "consul/data",
  "log_level": "INFO",
  "addresses": {
    "http": "0.0.0.0"
  },
  "connect": {
    "enabled": true
  },
  "enable_debug": false
}

• start consul agent ./bin/consul agent --config-file=server.json
• open http://localhost:8500/v1/agent/self -> observer EnableDebug is false
• change enable_debug: true in the server.json file
• ./bin/consul reload
• Open http://localhost:8500/v1/agent/self see - EnableDebug flag does not change to true

Testing

• Git clone this repo
• Run make dev
• create a config file server.json

{
  "node_name": "consul-server",
  "server": true,
  "bootstrap": true,
  "ui_config": {
    "enabled": true
  },
  "datacenter": "dc1",
  "data_dir": "consul/data",
  "log_level": "INFO",
  "addresses": {
    "http": "0.0.0.0"
  },
  "connect": {
    "enabled": false
  },
  "enable_debug": false
}

• start consul agent ./bin/consul agent --config-file=server.json
• open http://localhost:8500/v1/agent/self -> observer EnableDebug is false
• change enable_debug: true in the server.json file
• ./bin/consul reload
• Open http://localhost:8500/v1/agent/self see - EnableDebug flag changes to true
• Next test for PProf Run - ./bin/consul debug -capture pprof
• Notice command runs successfully.

PR Checklist

• updated test coverage
• external facing docs updated
• appropriate backport labels added
• not a security concern

[huikang]

is this a breaking change?

[absolutelightning]

yes. if clients are using 400 response for this API. this will be a breaking change as now it will return 200.

[absolutelightning]

do we have a procedure to handle breaking changes @huikang ?

[huikang]

I guess we can add a note of breaking change in the changelog. Here is an example:

https://github.com/hashicorp/consul/pull/17415/files#diff-0687bd929af1acf8b825fd03984e85c2f561626921a76a9d150c8c1dc06933a5

[huikang]

In addition to asserting the configuration, is there a way to verify the debug mode is active? Or maybe somewhere in http_oss_test.go.

[huikang]

Left a small suggestion for test automation. Otherwise looks good.

[dhiaayachi]

Hey @absolutelightning
Thank you for adding this change 🙂! I left few comments mostly around a possible race condition.

[dhiaayachi]

This is the read part of the race, see my comment above.

[dhiaayachi]

@absolutelightning I added some details on how you can implement this. Let me know if you have other questions.

[dhiaayachi]

I think this should stay as a bool as this RuntimeConfig data structure could be copied and copying an atomic have unwanted side effects.

instead we will need to apply the config to the agent every time it's updated. You can add an atomic here in the agent struct and init that atomic with the value in the agent Start and everytime the config is reloaded apply it to the atomic.
Then you can use the atomic safely in the http handler.

[absolutelightning]

@dhiaayachi comments resolved. really appreciate the detailed help regarding the changes. thanks.

[hashicorp-cla]

All committers have signed the CLA.

[david-yu]

@dhiaayachi Are there any other outstanding changes to address in this PR?

[dhiaayachi]

Thank you for working on this and taking the time to go through my comments @absolutelightning
LGTM!

[david-yu]

Added backports to 1.16, 1.15, and 1.14 which are the currently maintained versions.

[absolutelightning]

Thank you for working on this and taking the time to go through my comments @absolutelightning LGTM!

Thanks @dhiaayachi for the review. Got to learn a lot.

[kisunji]

@absolutelightning Could you make sure patches are applied in enterprise to unblock merges across all backports? There are some enterprise files where the handler needs to be updated with the new signatures. Drop a note in our slack if you need assistance!