Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API #1178

jmurret · 2022-04-19T19:02:32Z

Related to issue: #714

Note: This PR imports the consul api package from the branch in this PR rather than a released version. We have validated that this change works for the customer and will update this PR when the COnsul change gets merged.

Changes proposed in this PR:

Defined global. consulAPITimeout with a default of 5s
Everything in charts/consul/templates passes -consul-api-timeout={{ .Values.global.consulAPITimeout }} to control-plane commands
Everything in charts/consul/test/unit asserts that those argument passings occur.
Everything in control-plane/ ensures:
- -consul-api-timeout is greater than zero
- -consul-api-timeout is used when creating consul Client

How I've tested this PR:

Unit test added
Acceptance test passed

How I expect reviewers to test this PR:

👀

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

thisisnotashwin

Excellent work @jmurret !!!

thisisnotashwin · 2022-04-26T18:07:01Z

control-plane/consul/consul_test.go

@@ -43,3 +43,13 @@ func TestNewClient(t *testing.T) {
 		UserAgentHeader: fmt.Sprintf("consul-k8s/%s", version.GetHumanVersion()),
 	}, consulAPICalls[0])
 }
+
+func TestNewClient_httpClientDefaultTimeout(t *testing.T) {


…perly set.

…t when new is passed with a config that contains an http client.

… SetClientTransportWithTLSConfig() have been exposed to allow getting access to Transport and TLSConfig defaulting when passing in your own http client.

…FromNonDefaultPartition since it timed out.

…omNonDefaultPartition to 30s to see if it does not timeout and the test passes.

…romNonDefaultPartition as it was using 5ns and timing out.

jmurret · 2022-05-02T20:11:34Z

control-plane/go.mod

 	github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510
-	github.com/hashicorp/consul/api v1.10.1-0.20220413234639-3e88f579fce3
+	github.com/hashicorp/consul/api v1.10.1-0.20220425143126-6d0162a58a94


this temporarily points to consul core dev branch.

jmurret · 2022-05-02T20:11:47Z

control-plane/go.sum

+github.com/hashicorp/consul/api v1.10.1-0.20220421045807-e7bb156a82e2 h1:kwb0ddQK6XTg9AVQgxe517Q0vrJ7MIPMIrp0yP/EZbE=
+github.com/hashicorp/consul/api v1.10.1-0.20220421045807-e7bb156a82e2/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ=
+github.com/hashicorp/consul/api v1.10.1-0.20220425143126-6d0162a58a94 h1:mPhpaeGO4BmD0Fi9gmevT7kYDyDml1kNjf0HKCFF5xM=
+github.com/hashicorp/consul/api v1.10.1-0.20220425143126-6d0162a58a94/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ=


this temporarily points to consul core dev branch.

thisisnotashwin

Excellent work on the PR @jmurret !!

thisisnotashwin · 2022-05-03T15:57:55Z

control-plane/connect-inject/endpoints_controller_test.go

+			// Splitting this into two asserts on fragments of the error because the error thrown
+			// can be either of the two below and matching on the whole string causes the test tobe flakey
+			// "1 error occurred:\n\t* Get \"http://126.0.0.1:31200/v1/agent/services?filter=Meta%5B%22k8s-service-name%22%5D+%3D%3D+%22service-created%22+and+Meta%5B%22k8s-namespace%22%5D+%3D%3D+%22default%22+and+Meta%5B%22managed-by%22%5D+%3D%3D+%22consul-k8s-endpoints-controller%22\": context deadline exceeded (Client.Timeout exceeded while awaiting headers)\n\n"
+			// "1 error occurred:\n\t* Get \"http://126.0.0.1:31200/v1/agent/services?filter=Meta%5B%22k8s-service-name%22%5D+%3D%3D+%22service-created%22+and+Meta%5B%22k8s-namespace%22%5D+%3D%3D+%22default%22+and+Meta%5B%22managed-by%22%5D+%3D%3D+%22consul-k8s-endpoints-controller%22\": dial tcp 126.0.0.1:31200: i/o timeout (Client.Timeout exceeded while awaiting headers)\n\n"


Thanks for the rationale! This makes sense.

thisisnotashwin · 2022-05-03T15:59:52Z

control-plane/subcommand/connect-init/command.go

@@ -234,6 +228,23 @@ func (c *Command) Run(args []string) int {
 	return 0
 }

+func (c *Command) validateFlags() error {


thisisnotashwin · 2022-05-03T16:02:10Z

control-plane/subcommand/flags/http.go

@@ -161,3 +169,43 @@ func (s *StringValue) String() string {
 	}
 	return current
 }
+


This is excellent!!!

…efaultConfig()

…nsport defined.

ndhanushkodi

So thorough!! LGTM!

ndhanushkodi · 2022-05-03T22:21:42Z

control-plane/subcommand/flags/http.go

@@ -161,3 +169,43 @@ func (s *StringValue) String() string {
 	}
 	return current
 }
+
+// DurationValue provides a flag value that's aware if it has been set.
+type DurationValue struct {


How are these DurationValue functions being used/meant to be used? Was it because we didn't have any duration related flags before and all duration flags need to implement these?

Yes, the StringValue was copied from this same file from consul core: https://github.com/hashicorp/consul/blob/21e855d860865f1472b3fdcb64a916e92bc83a9f/command/flags/config.go#L91 and there is a comment at the top of this file from a while ago about why we have copied them.

I originally had this an IntValue but ultimately it is going to become a duration and we already have some places in our helm values where you can configure duration like "5s", "500ms", etc., so I changed it to make it consistent and more flexible.

jmurret force-pushed the jm/endpoints-bug branch from dd25c96 to c6357b7 Compare April 19, 2022 20:37

jmurret mentioned this pull request Apr 20, 2022

Endpoints Controller queuing up service registrations/deregistrations when request to agent on a terminated pod does not time out #714

Closed

jmurret requested review from a team, ndhanushkodi and thisisnotashwin and removed request for a team April 25, 2022 15:11

jmurret marked this pull request as ready for review April 25, 2022 15:13

thisisnotashwin approved these changes Apr 26, 2022

View reviewed changes

jmurret added 22 commits April 29, 2022 17:20

Providing endpoints controller with an http client with a timeout

1a91234

handling TLS transport when providing our own httpCLient

e732e1a

importing consul client change from a branch where transport gets pro…

8778611

…perly set.

ran go mod tidy

3d54428

Removing transport remnant

025fa91

getting latest changes from consul work

3a19398

using exposed methods to set config

2f58db7

adding checks to functions

b6258f9

Adding unit test

4348956

Revising test after enabling the consul side just to process transpor…

61f41cd

…t when new is passed with a config that contains an http client.

Updating to latest changes in Consul

de87e5f

Centralizing timeout in control-plane/consul.NewClient

a98f800

fix remnants

480ef10

Using latest version of changes in consul where SetClientConfig() and…

307e2bc

… SetClientTransportWithTLSConfig() have been exposed to allow getting access to Transport and TLSConfig defaulting when passing in your own http client.

Updating acceptance test to haveright message

902a591

Changing back to 2 seconds

baeebdf

fixing test

219378c

Added flags for configuration of timeout to commands

55bb207

Fix broken test

e1a4dd3

Adding validation that consul-api-timeout is greater than zero

60719ca

Fixing 100 broken tests

7000772

fix broken tests 2:electric bugaloo

8005b40

fixing flakey test

42b4394

jmurret force-pushed the jm/endpoints-bug branch from 430b09b to 42b4394 Compare April 29, 2022 23:20

jmurret added 10 commits April 29, 2022 17:38

Changing from int to duration

844b45f

Fixing lint

8b47b83

Updating values.yaml file to be a duration.

dc07dae

Fix broken test

1e43632

Fixing broken container-init tests

035f65a

increasing value of timeout to 10s for TestRun_AnonymousToken_Created…

46aadb4

…FromNonDefaultPartition since it timed out.

Trying to increase client timeout on TestRun_AnonymousToken_CreatedFr…

16255e6

…omNonDefaultPartition to 30s to see if it does not timeout and the test passes.

Fixing the setting of the timeout for TestRun_AnonymousToken_CreatedF…

9374004

…romNonDefaultPartition as it was using 5ns and timing out.

adding missing time import

5e58984

Fixing apiGateway bats test

d54c7f7

jmurret commented May 2, 2022

View reviewed changes

updated comment and test

d1ec7ae

jmurret requested a review from thisisnotashwin May 2, 2022 20:14

jmurret added 3 commits May 2, 2022 16:02

Moving all logic to consul k8s side

dad6855

fixing go.mod and go.sum

25b4137

formatting

adbeab4

thisisnotashwin approved these changes May 3, 2022

View reviewed changes

jmurret added 5 commits May 3, 2022 10:54

Utilizing Default config andmanually setting transport

ea6bd5a

replace instances of api config struct instatiation with use of api.D…

2e196b2

…efaultConfig()

Fixig create-federation-secret command to use defaultConfig from api.

448542d

fixing broken tests

af0c64d

adding clause in consul.NewClient for when config does not have a Tra…

8a470f6

…nsport defined.

ndhanushkodi approved these changes May 3, 2022

View reviewed changes

jmurret changed the title ~~Providing endpoints controller with an http client with a timeout~~ Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API May 3, 2022

Update CHANGELOG

16371b9

jmurret merged commit 488e8ac into main May 4, 2022

jmurret deleted the jm/endpoints-bug branch May 4, 2022 02:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API #1178

Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API #1178

jmurret commented Apr 19, 2022 •

edited

Loading

thisisnotashwin left a comment

thisisnotashwin Apr 26, 2022

jmurret May 2, 2022

jmurret May 2, 2022

thisisnotashwin left a comment

thisisnotashwin May 3, 2022

thisisnotashwin May 3, 2022

thisisnotashwin May 3, 2022

ndhanushkodi left a comment

ndhanushkodi May 3, 2022

jmurret May 3, 2022

Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API #1178

Enable the ability to configure the ClientTimeout of the http client that interacts with the Consul API #1178

Conversation

jmurret commented Apr 19, 2022 • edited Loading

thisisnotashwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnotashwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmurret commented Apr 19, 2022 •

edited

Loading