Don't set the value of TLSConfig.Address explicitly.
This will make sure env vars like CONSUL_TLS_SERVER_NAME are taken into account for the connection. Fixes#4718.
In a real agent the `cache` instance is alive until the agent shuts down so this is not a real leak in production, however in out test suite, every testAgent that is started and stops leaks goroutines that never get cleaned up which accumulate consuming CPU and memory through subsequent test in the `agent` package which doesn't help our test flakiness.
This adds a Close method that doesn't invalidate or clean up the cache, and still allows concurrent blocking queries to run (for up to 10 mins which might still affect tests). But at least it doesn't maintain them forever with background refresh and an expiry watcher routine.
It would be nice to cancel any outstanding blocking requests as well when we close but that requires much more invasive surgery right into our RPC protocol since we don't have a way to cancel requests currently.
Unscientifically this seems to make tests pass a bit quicker and more reliably locally but I can't really be sure of that!
cli: forward SIGTERM to child process of 'lock' and 'watch' subcommands on unix
This also removes the signal handler for SIGKILL as it's impossible to receive these signals.
* Fix CA pruning when CA config uses string durations.
The tl;dr here is:
- Configuring LeafCertTTL with a string like "72h" is how we do it by default and should be supported
- Most of our tests managed to escape this by defining them as time.Duration directly
- Out actual default value is a string
- Since this is stored in a map[string]interface{} config, when it is written to Raft it goes through a msgpack encode/decode cycle (even though it's written from server not over RPC).
- msgpack decode leaves the string as a `[]uint8`
- Some of our parsers required string and failed
- So after 1 hour, a default configured server would throw an error about pruning old CAs
- If a new CA was configured that set LeafCertTTL as a time.Duration, things might be OK after that, but if a new CA was just configured from config file, intialization would cause same issue but always fail still so would never prune the old CA.
- Mostly this is just a janky error that got passed tests due to many levels of complicated encoding/decoding.
tl;dr of the tl;dr: Yay for type safety. Map[string]interface{} combined with msgpack always goes wrong but we somehow get bitten every time in a new way :D
We already fixed this once! The main CA config had the same problem so @kyhavlov already wrote the mapstructure DecodeHook that fixes it. It wasn't used in several places it needed to be and one of those is notw in `structs` which caused a dependency cycle so I've moved them.
This adds a whole new test thta explicitly tests the case that broke here. It also adds tests that would have failed in other places before (Consul and Vaul provider parsing functions). I'm not sure if they would ever be affected as it is now as we've not seen things broken with them but it seems better to explicitly test that and support it to not be bitten a third time!
* Typo fix
* Fix bad Uint8 usage
If you provide an invalid HTTP configuration consul will still start again instead of failing. But if you do so the build-in proxy won't be able to start which you might need for connect.
This implements parts of RFC 7871 where Consul is acting as an authoritative name server (or forwarding resolver when recursors are configured)
If ECS opt is present in the request we will mirror it back and return a response with a scope of 0 (global) or with the same prefix length as the request (indicating its valid specifically for that subnet).
We only mirror the prefix-length (non-global) for prepared queries as those could potentially use nearness checks that could be affected by the subnet. In the future we could get more sophisticated with determining the scope bits and allow for better caching of prepared queries that don’t rely on nearness checks.
The other thing this does not do is implement the part of the ECS RFC related to originating ECS headers when acting as a intermediate DNS server (forwarding resolver). That would take a quite a bit more effort and in general provide very little value. Consul will currently forward the ECS headers between recursors and the clients transparently, we just don't originate them for non-ECS clients to get potentially more accurate "location aware" results.
Fixes: #4578
Prior to this fix if there was an error binding to ports for the DNS servers the error would be swallowed by the gated log writer and never output. This fix propagates the DNS server errors back to the shell with a multierror.
* Implementation of Weights Data structures
Adding this datastructure will allow us to resolve the
issues #1088 and #4198
This new structure defaults to values:
```
{ Passing: 1, Warning: 0 }
```
Which means, use weight of 0 for a Service in Warning State
while use Weight 1 for a Healthy Service.
Thus it remains compatible with previous Consul versions.
* Implemented weights for DNS SRV Records
* DNS properly support agents with weight support while server does not (backwards compatibility)
* Use Warning value of Weights of 1 by default
When using DNS interface with only_passing = false, all nodes
with non-Critical healthcheck used to have a weight value of 1.
While having weight.Warning = 0 as default value, this is probably
a bad idea as it breaks ascending compatibility.
Thus, we put a default value of 1 to be consistent with existing behaviour.
* Added documentation for new weight field in service description
* Better documentation about weights as suggested by @banks
* Return weight = 1 for unknown Check states as suggested by @banks
* Fixed typo (of -> or) in error message as requested by @mkeeler
* Fixed unstable unit test TestRetryJoin
* Fixed unstable tests
* Fixed wrong Fatalf format in `testrpc/wait.go`
* Added notes regarding DNS SRV lookup limitations regarding number of instances
* Documentation fixes and clarification regarding SRV records with weights as requested by @banks
* Rephrase docs
* Added log-file flag to capture Consul logs in a user specified file
* Refactored code.
* Refactored code. Added flags to rotate logs based on bytes and duration
* Added the flags for log file and log rotation on the webpage
* Fixed TestSantize from failing due to the addition of 3 flags
* Introduced changes : mutex, data-dir log writes, rotation logic
* Added test for logfile and updated the default log destination for docs
* Log name now uses UnixNano
* TestLogFile is now uses t.Parallel()
* Removed unnecessary int64Val function
* Updated docs to reflect default log name for log-file
* No longer writes to data-dir and adds .log if the filename has no extension
Fixes#4515
This just slightly refactors the logic to only attempt to set the serf wan reconnect timeout when the rest of the serf wan settings are configured - thus avoiding a segfault.
* Display more information about check being not properly added when it fails
It follows an incident where we add lots of error messages:
[WARN] consul.fsm: EnsureRegistration failed: failed inserting check: Missing service registration
That seems related to Consul failing to restart on respective agents.
Having Node information as well as service information would help diagnose the issue.
* Renamed ensureCheckIfNodeMatches() as requested by @banks
- Add WaitForTestAgent to tests flaky due to missing serfHealth registration
- Fix bug in retries calling Fatalf with *testing.T
- Convert TestLockCommand_ChildExitCode to table driven test
* Allow to rename nodes with IDs, will fix#3974 and #4413
This change allow to rename any well behaving recent agent with an
ID to be renamed safely, ie: without taking the name of another one
with case insensitive comparison.
Deprecated behaviour warning
----------------------------
Due to asceding compatibility, it is still possible however to
"take" the name of another name by not providing any ID.
Note that when not providing any ID, it is possible to have 2 nodes
having similar names with case differences, ie: myNode and mynode
which might lead to DB corruption on Consul server side and
lead to server not properly restarting.
See #3983 and #4399 for Context about this change.
Disabling registration of nodes without IDs as specified in #4414
should probably be the way to go eventually.
* Removed the case-insensitive search when adding a node within the else
block since it breaks the test TestAgentAntiEntropy_Services
While the else case is probably legit, it will be fixed with #4414 in
a later release.
* Added again the test in the else to avoid duplicated names, but
enforce this test only for nodes having IDs.
Thus most tests without any ID will work, and allows us fixing
* Added more tests regarding request with/without IDs.
`TestStateStore_EnsureNode` now test registration and renaming with IDs
`TestStateStore_EnsureNodeDeprecated` tests registration without IDs
and tests removing an ID from a node as well as updated a node
without its ID (deprecated behaviour kept for backwards compatibility)
* Do not allow renaming in case of conflict, including when other node has no ID
* Fixed function GetNodeID that was not working due to wrong type when searching node from its ID
Thus, all tests about renaming were not working properly.
Added the full test cas that allowed me to detect it.
* Better error messages, more tests when nodeID is not a valid UUID in GetNodeID()
* Added separate TestStateStore_GetNodeID to test GetNodeID.
More complete test coverage for GetNodeID
* Added new unit test `TestStateStore_ensureNoNodeWithSimilarNameTxn`
Also fixed comments to be clearer after remarks from @banks
* Fixed error message in unit test to match test case
* Use uuid.ParseUUID to parse Node.ID as requested by @mkeeler
* Fixes TestAgent_IndexChurn
* Fixes TestPreparedQuery_Wrapper
* Increased sleep in agent_test for IndexChurn to 500ms
* Made the comment about joinWAN operation much less of a cliffhanger
* Revert "BUGFIX: Unit test relying on WaitForLeader() did not work due to wrong test (#4472)"
This reverts commit cec5d72396.
* Revert "CA initialization while boostrapping and TestLeader_ChangeServerID fix. (#4493)"
This reverts commit 589b589b53.
- Improve resilience of testrpc.WaitForLeader()
- Add additionall retry to CI
- Increase "go test" timeout to 8m
- Add wait for cluster leader to several tests in the agent package
- Add retry to some tests in the api and command packages
* Fixes the DNS recursor properly resolving the requests
* Added a test case for the recursor bug
* Refactored code && added a test case for all failing recursors
* Inner indentation moved into else if check
Fixes: #4441
This fixes the issue with Connect Managed Proxies + ACLs being broken.
The underlying problem was that the token parsed for most http endpoints was sent untouched to the servers via the RPC request. These changes make it so that at the HTTP endpoint when parsing the token we additionally attempt to convert potential proxy tokens into regular tokens before sending to the RPC endpoint. Proxy tokens are only valid on the agent with the managed proxy so the resolution has to happen before it gets forwarded anywhere.
* New Providers added and updated vendoring for go-discover
* Vendor.json formatted using make vendorfmt
* Docs/Agent/auto-join: Added documentation for the new providers introduced in this PR
* Updated the golang.org/x/sys/unix in the vendor directory
* Agent: TestGoDiscoverRegistration updated to reflect the addition of new providers
* Deleted terraform.tfstate from vendor.
* Deleted terraform.tfstate.backup
Deleted terraform state file artifacts from unknown runs.
* Updated x/sys/windows vendor for Windows binary compilation
* Fix theoretical cache collision bug if/when we use more cache types with same result type
* Generalized fix for blocking query handling when state store methods return zero index
* Refactor test retry to only affect CI
* Undo make file merge
* Add hint to error message returned to end-user requests if Connect is not enabled when they try to request cert
* Explicit error for Roots endpoint if connect is disabled
* Fix tests that were asserting old behaviour
Also change how loadProxies works. Now it will load all persisted proxies into a map, then when loading config file proxies will look up the previous proxy token in that map.
UUID auto-generation here causes trouble in a few cases. The biggest being older
nodes reregistering will fail when the UUIDs are different and the names match
This reverts commit 0f70034082.
This reverts commit d1a8f9cb3f.
This reverts commit cf69ec42a4.
Since DNS is case insensitive and DB as issues when similar names with different
cases are added, check for unicity based on case insensitivity.
Following another big incident we had in our cluster, we also validate
that adding/renaming a not does not conflicts with case insensitive
matches.
We had the following error once:
- one node called: mymachine.MYDC.mydomain was shut off
- another node (different ID) was added with name: mymachine.mydc.mydomain before
72 hours
When restarting the consul server of domain, the consul server restarted failed
to start since it detected an issue in RAFT database because
mymachine.MYDC.mydomain and mymachine.mydc.mydomain had the same names.
Checking at registration time with case insensitivity should definitly fix
those issues and avoid Consul DB corruption.
It fixes the following warnings:
agent/config/builder.go:1201: Errorf format %q has arg s of wrong type *string
agent/config/builder.go:1240: Errorf format %q has arg s of wrong type *string
This also changes where the enforcement of the enable_additional_node_meta_txt configuration gets applied.
formatNodeRecord returns the main RRs and the meta/TXT RRs in separate slices. Its then up to the caller to add to the appropriate sections or not.
This just makes sure that if multiple services are registered with unique service addresses that we don’t blast back multiple CNAMEs for the same service DNS name and keeps us within the DNS specs.
It will allow the following:
* when connectivity is limited (saturated linnks between DCs), only one
single request to refresh an ACL will be sent to ACL master DC instead
of statcking ACL refresh queries
* when extend-cache is used for ACL, do not wait for result, but refresh
the ACL asynchronously, so no delay is not impacting slave DC
* When extend-cache is not used, keep the existing blocking mechanism,
but only send a single refresh request.
This will fix https://github.com/hashicorp/consul/issues/3524
These were only added as SPIFFE intends to use the in the future but currently does not mandate their usage due to patch support in common TLS implementations and some ambiguity over how to use them with URI SAN certificates. We included them because until now everything seem fine with it, however we've found the latest version of `openssl` (1.1.0h) fails to validate our certificats if its enabled. LibreSSL as installed on OS X by default doesn’t have these issues. For now it's most compatible not to have them and later we can find ways to add constraints with wider compatibility testing.
Few other fixes in here just to get a clean run locally - they are all also fixed in other PRs but shouldn't conflict.
This should be robust to timing between goroutines now.
Igal Shprincis
Paul Banks
Paul O'Connor
R.B. Boyer
Alex Dadgar
Kyle Havlovitz
mkeeler
Hans Hasselberg
Pierre Souchay
Matt Keeler
Mitchell Hashimoto
Mitchell Hashimoto
Martin
Siva Prasad
Freddy
Miroslav Bagljas
nickmy9729
Armon Dadgar
azam
Pierre Souchay
MagnumOpus21
Abhishek Chanda
Jack Pearkes