fix(client): plumb Server.ClientTimeout into the rebuilt auth config - #59
Open
spbsoluble wants to merge 7 commits into
Open
fix(client): plumb Server.ClientTimeout into the rebuilt auth config#59spbsoluble wants to merge 7 commits into
spbsoluble wants to merge 7 commits into
Conversation
…mand API
Command's TemplateUpdateRequest.KeyUsage and TemplateRetrievalResponse.KeyUsage
are both {"type":"integer","format":"int32"} per the v25.5 swagger — an int32
bitmask (e.g. 160 = digitalSignature|keyEncipherment). UpdateTemplateArg.KeyUsage
was typed *bool, which serializes as a JSON boolean and produces a live HTTP 400
from Command ("Unexpected character encountered while parsing value: t. Path
'KeyUsage'"), making the field unusable as-is.
GetTemplateResponse.KeyUsage was already int, so this also fixes the type
mismatch between the get and update models for the same field.
Also fixes the identical defect in v2/api/template_models.go for consistency;
v2 is tagged/released independently and is not part of this v3.6.0 change.
Adds TestUpdateTemplateArg_KeyUsage_SerializesAsInt to v3/api/template_test.go,
which fails to compile against the pre-fix *bool field and asserts the wire
payload is a JSON number.
NewKeyfactorClient rebuilds a fresh CommandAuthConfig from the caller's *auth_providers.Server instead of reusing the one that produced it, but never carried over ClientTimeout. Every consumer -- including the Terraform provider's request_timeout setting -- ended up authenticating and issuing requests with DefaultClientTimeout (60s) regardless of what was configured, causing "net/http: timeout awaiting response headers" on long-running calls like PFX enrollment. Set HttpClientTimeout: cfg.ClientTimeout in the baseConfig literal so it flows into BuildTransport()/SetClient() for both the basic and oauth auth paths. Depends on github.com/Keyfactor/keyfactor-auth-client-go#51 being fixed upstream (Server.ClientTimeout field). go.mod is bumped to the not-yet-tagged v1.6.0-rc.1 and pinned locally via a `replace` directive at /tmp/kf-worktrees/kfc-auth for testing; once that tag is cut, drop the replace and re-run `go mod tidy`.
Removes the local replace directive and TODO now that the ClientTimeout fix is published, and validates against the published dependency.
Client.sendRequest called AuthConfig.GetHttpClient() on every single request. Both CommandConfigOauth and CommandAuthConfigBasic in keyfactor-auth-client-go build a brand new http.Transport (and therefore a brand new, empty connection pool) on each call, and that transport's IdleConnTimeout is derived from the configured HttpClientTimeout - so every API call opened its own never-reused connection whose socket lingered until IdleConnTimeout fired. This leak predates this branch at the fixed 60s default; plumbing a caller-configured ClientTimeout through (which can be arbitrarily large, e.g. 1800s for slow enrollments) widens the linger window proportionally, so cache the *http.Client on Client and reuse it across requests instead of rebuilding it per call. The OAuth token source is still consulted (and refreshed) on every RoundTrip independent of how many times the *http.Client is reused, and NewKeyfactorClientWithAuth (used by VCR/unit tests) still works by lazily populating the cache on first use.
TestNewKeyfactorClient_PlumbsClientTimeout and TestNewKeyfactorClient_DefaultClientTimeout build a Server config with fields intentionally left at their zero value to exercise ValidateAuthConfig's environment-variable fallback path. Because ValidateAuthConfig only falls back to KEYFACTOR_CLIENT_TIMEOUT/ KEYFACTOR_PORT/KEYFACTOR_CA_CERT when the struct field is unset, and unconditionally overwrites SkipVerify from KEYFACTOR_SKIP_VERIFY regardless of the struct field, ambient values for these variables (e.g. from a sourced lab env file) broke both tests: KEYFACTOR_CLIENT_TIMEOUT=120 flips the expected default from 60 to 120, and KEYFACTOR_SKIP_VERIFY=false clobbers SkipTLSVerify:true and rejects the tests' self-signed httptest TLS certificate. Add isolateKeyfactorEnv to unset the relevant variables for the duration of each test and restore their original values afterward. t.Setenv(key, "") does not work here since an empty value is still "present" to os.LookupEnv.
Picks up the round-4 convergence fixes: ClientTimeout persistence gated across all three concrete auth types via delegation to the base type, a BOM-prefix bypass fix in nested-JSON secret redaction, MaxConnsPerHost widened to unbounded, and body redaction extended to cover JSON-in-string values.
…rHost=10 Closes the loop on a finding this package's own http.Client-caching fix could not verify end-to-end: caching a single *http.Client turns the transport's MaxConnsPerHost into a permanent, unqueued-timeout concurrency ceiling for the process, since the cached client has no Timeout and requests carry no deadline. keyfactor-auth-client-go's fix (MaxConnsPerHost widened from a hardcoded 10 to unbounded) was only verified there by inspecting the constructed transport's field value. Add an end-to-end regression test that builds a real Client via NewKeyfactorClient, retrieves its cached *http.Client, and drives 25 concurrent requests through it against a real httptest server, asserting the server observes well more than 10 requests in flight at once. Confirmed this fails against v1.6.0-rc.2 (10 in-flight, ~620ms) and passes against v1.6.0-rc.3 (25 in-flight, ~225ms).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to Keyfactor/keyfactor-auth-client-go#52 (fixes Keyfactor/keyfactor-auth-client-go#51 downstream).
Problem
NewKeyfactorClientrebuilds aCommandAuthConfigfrom the incoming*auth_providers.Serverbut never carried a client timeout — the field didn't exist onServeruntil keyfactor-auth-client-go v1.6.0-rc.2. The rebuilt config therefore always fell back to the 60s default (or theKEYFACTOR_CLIENT_TIMEOUTenv var), ignoring whatever the caller had configured viaWithClientTimeout()before handing off theServer. Symptom in terraform-provider-keyfactor:request_timeout = 300still timed out at ~60s (net/http: timeout awaiting response headers) on slow PFX enrollments.Fix
keyfactor-auth-client-gotov1.6.0-rc.2(addsServer.ClientTimeout).NewKeyfactorClient: copycfg.ClientTimeoutintobaseConfig.HttpClientTimeoutso the rebuilt auth config (and the transport built from it) honors the caller's timeout.Tests
New test drives
NewKeyfactorClientagainst an httptest fake Command server withServer.ClientTimeoutset and asserts the resulting transport uses it. Red before the fix, green after.GOWORK=off go test -mod=mod ./...green against the published dep.Based on
v3.6.0-rc.0; RC tagv3.6.0-rc.1was cut from this branch and terraform-provider-keyfactor's full unit suite passes against it.