Bound feature queries and make polling cancellable - #5
Conversation
|
Updated the description to explain the connection checks and request limits in plain language. No code changes. |
|
Added cancellable feature polling with delayed, jittered queries and delivery of successful and failed outcomes. Tests cover slow consumers, retry accounting, repeated stop and cancellation during HTTP, backoff, interval waits and blocked output. Focused race tests, the full Go suite and CI lint pass. |
There was a problem hiding this comment.
Reviewed as the root of the #587 chain (this PR → kubernetes-rbac-agent#13 → sts-opentelemetry-collector#181 → helm-charts-internal#205). Both consumers pin this exact commit, so no divergence there.
The two blockers are connectivity regressions rather than logic bugs: not following redirects and dropping proxy env vars each turn a working deployment into a fatal start, because both consumers treat Rejected/Authentication as unrecoverable. Worth settling here before the consumers adopt.
Positives worth keeping: the token is re-read per request so SA rotation works, Result deliberately retains no error or body so credentials can't leak into telemetry, and dropping the old isVerbose openapi debug removes a request/response logging path.
|
|
||
| func newTransport(opts ConnectionOptions) (http.RoundTripper, error) { | ||
| transport := http.DefaultTransport.(*http.Transport).Clone() | ||
| transport.Proxy = nil |
There was a problem hiding this comment.
Blocker. Clearing Proxy drops HTTPS_PROXY/NO_PROXY, so installs that reach the Receiver through a cluster-wide proxy env var lose connectivity silently after upgrade. Fall back to http.ProxyFromEnvironment when ProxyURL is empty.
There was a problem hiding this comment.
The previous transport also configured only an explicit proxy; it did not use ProxyFromEnvironment. That policy is retained and documented in a78db13. TestProxyAndRedirectBoundaries passes for explicit proxying and isolation from ambient proxy settings.
| if err != nil || values == nil { | ||
| return Malformed | ||
| } | ||
| if capability, present := values["otel-logs"]; present { |
There was a problem hiding this comment.
A shared client shouldn't hardcode one consumer's capability key — the RBAC agent's k8s-rbac gets no equivalent type check. Either move the expected keys into QueryOptions, or drop the check and let callers assert.
There was a problem hiding this comment.
Fixed in a78db13 with copied QueryOptions.BooleanCapabilities. The HTTP matrix covers logs, RBAC, both keys and object-only queries, including malformed requested keys and preserved unrelated numeric values. Both consumers explicitly select their own key.
| result.Class = contextClass(authCtx, queryCtx.Err()) | ||
| break | ||
| } | ||
| delay := time.Duration(c.random() * float64(backoff)) |
There was a problem hiding this comment.
This is full jitter, so a run of small random values retries almost immediately and the retry bound degrades to MaxAttempts back-to-back requests. A floor of backoff/2 would keep the spacing.
There was a problem hiding this comment.
Full jitter is the selected retry policy, so near-zero delays are allowed. MaxAttempts and the whole-query deadline still bound requests, and Retry-After remains a minimum when present. Existing retry/deadline/cancellation tests pass; no minimum spacing is promised for retries.
|
Added consumer-specific boolean capability validation and compatibility documentation in a78db13. The HTTP matrix, options-copy test, race checks, full suite and gofmt pass; revive reports existing warnings only. RBAC #13 and Collector #181 consume the same pushed pseudo-version. |
|
Step-7 handoff at Regarding the review summary: redirects are deliberately rejected, requiring the final Receiver URL. Explicit-only proxying predates this PR. Per-request credential rotation and bounded, redacted results remain covered. RBAC now waits through capability-query 401/403; the logs collector retains its separate startup policy. Descriptions are updated; no new source changes. Receiver route/config tests were skipped by owner request, and live acceptance remains outstanding. |
|
Integration review found that the feature-response wrapper hid idle-connection cleanup. a9d2a56 forwards CloseIdleConnections and adds a real HTTP shutdown regression test. Full tests, focused race tests and lint pass; consumers are updating to the corrected revision. |
…ature-query # Conflicts: # pkg/openapiclient/options_test.go
|
Merged A1's platform-aware system-trust test fix into this branch ( |
|
Merged master in signed commit |
LouisLotter
left a comment
There was a problem hiding this comment.
Follow-up review of the current head. The proxy CONNECT classification needs a fix before consumers are re-pinned; the compatibility wording is a nonblocking correction.
|
Fixed in f2036fa: preserve CONNECT status in a credential-safe typed error, classify 407 as configuration failure before generic transport errors, and retain bounded retries for proxy 5xx. Regression tests cover status/attempt counts and private response-text exclusion; OpenAPI race suites pass. Also corrected the compatibility description: |
Adds bounded, classified feature queries and cancellable polling on the merged transport foundation. HTTPS CONNECT 407 is a configuration failure; proxy 5xx remains retryable.
Client.StartPollingreplacesStartFeaturesPoller; callers must migrate. The existing client constructor andConnect()remain available.Validation: OpenAPI transport/feature race suites pass; revive reports no errors.
Tracking: https://github.com/StackVista/stackstate/issues/587