700c693b33
* server: add placeholder glue for rate limit handler This commit adds a no-op implementation of the rate-limit handler and adds it to the `consul.Server` struct and setup code. This allows us to start working on the net/rpc and gRPC interceptors and config logic. * Add handler errors * Set the global read and write limits * fixing multilimiter moving packages * Fix typo * Simplify globalLimit usage * add multilimiter and tests * exporting LimitedEntity * Apply suggestions from code review Co-authored-by: John Murret <john.murret@hashicorp.com> * add config update and rename config params * add doc string and split config * Apply suggestions from code review Co-authored-by: Dan Upton <daniel@floppy.co> * use timer to avoid go routine leak and change the interface * add comments to tests * fix failing test * add prefix with config edge, refactor tests * Apply suggestions from code review Co-authored-by: Dan Upton <daniel@floppy.co> * refactor to apply configs for limiters under a prefix * add fuzz tests and fix bugs found. Refactor reconcile loop to have a simpler logic * make KeyType an exported type * split the config and limiter trees to fix race conditions in config update * rename variables * fix race in test and remove dead code * fix reconcile loop to not create a timer on each loop * add extra benchmark tests and fix tests * fix benchmark test to pass value to func * server: add placeholder glue for rate limit handler This commit adds a no-op implementation of the rate-limit handler and adds it to the `consul.Server` struct and setup code. This allows us to start working on the net/rpc and gRPC interceptors and config logic. * Set the global read and write limits * fixing multilimiter moving packages * add server configuration for global rate limiting. * remove agent test * remove added stuff from handler * remove added stuff from multilimiter * removing unnecessary TODOs * Removing TODO comment from handler * adding in defaulting to infinite * add disabled status in there * adding in documentation for disabled mode. * make disabled the default. * Add mock and agent test * addig documentation and missing mock file. * Fixing test TestLoad_IntegrationWithFlags * updating docs based on PR feedback. * Updating Request Limits mode to use int based on PR feedback. * Adding RequestLimits struct so we have a nested struct in ReloadableConfig. * fixing linting references * Update agent/consul/rate/handler.go Co-authored-by: Dan Upton <daniel@floppy.co> * Update agent/consul/config.go Co-authored-by: Dan Upton <daniel@floppy.co> * removing the ignore of the request limits in JSON. addingbuilder logic to convert any read rate or write rate less than 0 to rate.Inf * added conversion function to convert request limits object to handler config. * Updating docs to reflect gRPC and RPC are rate limit and as a result, HTTP requests are as well. * Updating values for TestLoad_FullConfig() so that they were different and discernable. * Updating TestRuntimeConfig_Sanitize * Fixing TestLoad_IntegrationWithFlags test * putting nil check in place * fixing rebase * removing change for missing error checks. will put in another PR * Rebasing after default multilimiter config change * resolving rebase issues * updating reference for incomingRPCLimiter to use interface * updating interface * Updating interfaces * Fixing mock reference Co-authored-by: Daniel Upton <daniel@floppy.co> Co-authored-by: Dhia Ayachi <dhia@hashicorp.com>
203 lines
11 KiB
Markdown
203 lines
11 KiB
Markdown
# Adding a Consul Config Field
|
|
|
|
This is a checklist of all the places you need to update when adding a new field
|
|
to config. There may be a few other special cases not included but this covers
|
|
the majority of configs.
|
|
|
|
We suggest you copy the raw markdown into a gist or local file and check them
|
|
off as you go (you can mark them as done by replace `[ ]` with `[x]` so github
|
|
renders them as checked). Then **please include the completed lists you worked
|
|
through in your PR description**.
|
|
|
|
Examples of special cases this doesn't cover are:
|
|
- If the config needs special treatment like a different default in `-dev` mode
|
|
or differences between OSS and Enterprise.
|
|
- If custom logic is needed to support backwards compatibility when changing
|
|
syntax or semantics of anything
|
|
|
|
There are four specific cases covered with increasing complexity:
|
|
1. adding a simple config field only used by client agents
|
|
1. adding a CLI flag to mirror that config field
|
|
1. adding a config field that needs to be used in Consul servers
|
|
1. adding a field to the Service Definition
|
|
|
|
## Adding a Simple Config Field for Client Agents
|
|
|
|
- [ ] Add the field to the Config struct (or an appropriate sub-struct) in
|
|
`agent/config/config.go`.
|
|
- [ ] Add the field to the actual RuntimeConfig struct in
|
|
`agent/config/runtime.go`.
|
|
- [ ] Add an appropriate parser/setter in `agent/config/builder.go` to
|
|
translate.
|
|
- [ ] Add the new field with a random value to both the JSON and HCL files in
|
|
`agent/config/testdata/full-config.*`, which should cause the test to fail.
|
|
Then update the expected value in `TestLoad_FullConfig` in
|
|
`agent/config/runtime_test.go` to make the test pass again.
|
|
- [ ] Run `go test -run TestRuntimeConfig_Sanitize ./agent/config -update` to update
|
|
the expected value for `TestRuntimeConfig_Sanitize`. Look at `git diff` to
|
|
make sure the value changed as you expect.
|
|
- [ ] **If** your new config field needed some validation as it's only valid in
|
|
some cases or with some values (often true).
|
|
- [ ] Add validation to Validate in `agent/config/builder.go`.
|
|
- [ ] Add a test case to the table test `TestLoad_IntegrationWithFlags` in
|
|
`agent/config/runtime_test.go`.
|
|
- [ ] **If** your new config field needs a non-zero-value default.
|
|
- [ ] Add that to `DefaultSource` in `agent/config/defaults.go`.
|
|
- [ ] Add a test case to the table test `TestLoad_IntegrationWithFlags` in
|
|
`agent/config/runtime_test.go`.
|
|
- [ ] If the config needs to be defaulted for the test server used in unit tests,
|
|
also add it to `DefaultConfig()` in `agent/consul/config.go`.
|
|
- [ ] **If** your config should take effect on a reload/HUP.
|
|
- [ ] Add necessary code to to trigger a safe (locked or atomic) update to
|
|
any state the feature needs changing. This needs to be added to one or
|
|
more of the following places:
|
|
- `ReloadConfig` in `agent/agent.go` if it needs to affect the local
|
|
client state or another client agent component.
|
|
- `ReloadConfig` in `agent/consul/client.go` if it needs to affect
|
|
state for client agent's RPC client.
|
|
- [ ] Add a test to `agent/agent_test.go` similar to others with prefix
|
|
`TestAgent_reloadConfig*`.
|
|
- [ ] Add documentation to `website/content/docs/agent/config/config-files.mdx`.
|
|
|
|
Done! You can now use your new field in a client agent by accessing
|
|
`s.agent.Config.<FieldName>`.
|
|
|
|
If you need a CLI flag, access to the variable in a Server context, or touched
|
|
the Service Definition, make sure you continue on to follow the appropriate
|
|
checklists below.
|
|
|
|
## Adding a CLI Flag Corresponding to the new Field
|
|
If the config field also needs a CLI flag, then follow these steps.
|
|
|
|
- [ ] Do all of the steps in [Adding a Simple Config
|
|
Field For Client Agents](#adding-a-simple-config-field-for-client-agents).
|
|
- [ ] Add the new flag to `agent/config/flags.go`.
|
|
- [ ] Add a test case to TestParseFlags in `agent/config/flag_test.go`.
|
|
- [ ] Add a test case (or extend one if appropriate) to the table test
|
|
`TestLoad_IntegrationWithFlags` in `agent/config/runtime_test.go` to ensure setting the
|
|
flag works.
|
|
- [ ] Add flag (as well as config file) documentation to
|
|
`website/source/docs/agent/config/config-files.mdx` and `website/source/docs/agent/config/cli-flags.mdx`.
|
|
|
|
## Adding a Simple Config Field for Servers
|
|
Consul servers have a separate Config struct for reasons. Note that Consul
|
|
server agents are actually also client agents, so in some cases config that is
|
|
only destined for servers doesn't need to follow this checklist provided it's
|
|
only needed during the bootstrapping of the server (which is done in code shared
|
|
by both server and client components in `agent.go`). For example WAN Gossip
|
|
configs are only valid on server agents but since WAN Gossip is setup in
|
|
`agent.go` they don't need to follow this checklist. The simplest (and mostly
|
|
accurate) rule is:
|
|
|
|
> If you need to access the config field from code in `agent/consul` (e.g. RPC
|
|
> endpoints), then you need to follow this. If it's only in `agent` (e.g. HTTP
|
|
> endpoints or agent startup) you don't.
|
|
|
|
A final word of warning - **you should never need to pass config into the FSM
|
|
(`agent/consul/fsm`) or state store (`agent/consul/state`)**. Doing so is **_very
|
|
dangerous_** and can violate consistency guarantees and corrupt databases. If
|
|
you think you need this then please discuss the design with the Consul team
|
|
before writing code!
|
|
|
|
Consul's server components for historical reasons don't use the `RuntimeConfig`
|
|
struct they have their own struct called `Config` in `agent/consul/config.go`.
|
|
|
|
- [ ] Do all of the steps in [Adding a Simple Config
|
|
Field For Client Agents](#adding-a-simple-config-field-for-client-agents).
|
|
- [ ] Add the new field to Config struct in `agent/consul/config.go`
|
|
- [ ] Add code to set the values from the `RuntimeConfig` in the confusingly
|
|
named `consulConfig` method in `agent/agent.go`
|
|
- [ ] **If needed**, add a test to `agent_test.go` if there is some non-trivial
|
|
behavior in the code you added in the previous step. We tend not to test
|
|
simple assignments from one to the other since these are typically caught by
|
|
higher-level tests of the actual functionality that matters but some examples
|
|
can be found prefixed with `TestAgent_consulConfig*`
|
|
- [ ] **If** your config should take effect on a reload/HUP
|
|
- [ ] Add necessary code to `ReloadConfig` in `agent/consul/server.go` this
|
|
needs to be adequately synchronized with any readers of the state being
|
|
updated.
|
|
- [ ] Add a new test or a new assertion to `TestServer_ReloadConfig`
|
|
|
|
You can now access that field from `s.srv.config.<FieldName>` inside an RPC
|
|
handler.
|
|
|
|
## Adding a New Field to Service Definition
|
|
The [Service Definition](https://www.consul.io/docs/agent/services.html) syntax
|
|
appears both in Consul config files but also in the `/v1/agent/service/register`
|
|
API.
|
|
|
|
For wonderful historical reasons, our config files have always used `snake_case`
|
|
attribute names in both JSON and HCL (even before we supported HCL!!) while our
|
|
API uses `CamelCase`.
|
|
|
|
Because we want documentation examples to work in both config files and API
|
|
bodies to avoid needless confusion, we have to accept both snake case and camel
|
|
case field names for the service definition.
|
|
|
|
Finally, adding a field to the service definition implies adding the field to
|
|
several internal structs and to all API outputs that display services from the
|
|
catalog. That explains the multiple layers needed below.
|
|
|
|
This list assumes a new field in the base service definition struct. Adding new
|
|
fields to health checks is similar but mostly needs `HealthCheck` structs and
|
|
methods updating instead. Adding fields to embedded structs like `ProxyConfig`
|
|
is largely the same pattern but may need different test methods etc. updating.
|
|
|
|
- [ ] Do all of the steps in [Adding a Simple Config
|
|
Field For Client Agents](#adding-a-simple-config-field-for-client-agents).
|
|
- [ ] `agent/structs` package
|
|
- [ ] Add the field to `ServiceDefinition` (`service_definition.go`)
|
|
- [ ] Add the field to `NodeService` (`structs.go`)
|
|
- [ ] Add the field to `ServiceNode` (`structs.go`)
|
|
- [ ] Update `ServiceDefinition.ToNodeService` to translate the field
|
|
- [ ] Update `NodeService.ToServiceNode` to translate the field
|
|
- [ ] Update `ServiceNode.ToNodeService` to translate the field
|
|
- [ ] Update `TestStructs_ServiceNode_Conversions`
|
|
- [ ] Update `ServiceNode.PartialClone`
|
|
- [ ] Update `TestStructs_ServiceNode_PartialClone` (`structs_test.go`)
|
|
- [ ] If needed, update `NodeService.Validate` to ensure the field value is
|
|
reasonable
|
|
- [ ] Add test like `TestStructs_NodeService_Validate*` in
|
|
`structs_test.go`
|
|
- [ ] Add comparison in `NodeService.IsSame`
|
|
- [ ] Update `TestStructs_NodeService_IsSame`
|
|
- [ ] Add comparison in `ServiceNode.IsSameService`
|
|
- [ ] Update `TestStructs_ServiceNode_IsSameService`
|
|
- [ ] **If** your field name has MultipleWords,
|
|
- [ ] Add it to the `aux` inline struct in
|
|
`ServiceDefinition.UnmarshalJSON` (`service_defintion.go`).
|
|
- Note: if the field is embedded higher up in a nested struct,
|
|
follow the chain and update the necessary struct's `UnmarshalJSON`
|
|
method - you may need to add one if there are no other case
|
|
transformations being done, copy and existing example.
|
|
- Note: the tests that exercise this are in agent endpoint for
|
|
historical reasons (this is where the translation used to happen).
|
|
- [ ] `agent` package
|
|
- [ ] Update `testAgent_RegisterService` and/or add a new test to ensure
|
|
your fields register correctly via API (`agent_endpoint_test.go`)
|
|
- [ ] **If** your field name has MultipleWords,
|
|
- [ ] Update `testAgent_RegisterService_TranslateKeys` to include
|
|
examples with it set in `snake_case` and ensure it is parsed
|
|
correctly. Run this via `TestAgent_RegisterService_TranslateKeys`
|
|
(agent_endpoint_test.go).
|
|
- [ ] `api` package
|
|
- [ ] Add the field to `AgentService` (`agent.go`)
|
|
- [ ] Add/update an appropriate test in `agent_test.go`
|
|
- (Note you need to use `make test` or ensure the `consul` binary on
|
|
your `$PATH` is a build with your new field - usually `make dev`
|
|
ensures this unless you're path is funky or you have a consul binary
|
|
even further up the shell's `$PATH`).
|
|
- [ ] Docs
|
|
- [ ] Update docs in `website/source/docs/agent/services.html.md`
|
|
- [ ] Consider if it's worth adding examples to feature docs or API docs
|
|
that show the new field's usage.
|
|
|
|
Note that although the new field will show up in the API output of
|
|
`/agent/services` , `/catalog/services` and `/health/services`, those tests
|
|
right now don't exercise anything that's super useful unless custom logic is
|
|
required since they don't even encode the response object as JSON and just
|
|
assert on the structs you already modified. If custom presentation logic is
|
|
needed, tests for these endpoints might be warranted too. It's usual to use
|
|
`omit-empty` for new fields that will typically not be used by existing
|
|
registrations although we don't currently test for that systematically.
|