feat(accesscontrol): wire AccessControlModule into plugin setup#595
Conversation
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The PR correctly wires AccessControlModule into the plugin setup alongside the existing SecretsProvider path. The type switch in setup.go, the new NewAccessControlModule constructor, the registration proto builder, and the Config.Valid() validator all follow the existing patterns consistently.
All hypotheses raised during review were dismissed on verification:
- The silent no-op in the type switch and the
anyparameter innewStubare protected by Go's type system — the public constructors (NewSecretsProvider/NewAccessControlModule) accept typed interfaces, so only correct values reachsetup(). - The
AccessControlModule_builder{}.Build()with no fields is consistent with the AccessControlModule proto definition having no required identifying fields analogous to SecretsProvider's pattern.
Low-severity observations (no action required):
time.AfterinsideregistrationGate.await(inherited from the existing resolver pattern) leaks timer goroutines until they fire when concurrent requests arrive before setup completes — this is a known Go idiom trade-off and mirrors the pre-existing resolver service behavior.- The timeout test case in
lifecycle_test.gousesregistrationTimeout: 0which fires immediately; this is technically nondeterministic but in practice works due to the timer firing atomically before the goroutine can closesetupCompleted.
Complement the SecretsProvider switch case in plugin setup with an AccessControlModule branch. Add the connect handler adapter and platform-specific proto->domain signing-info mapping, plus a registration-gating wrapper mirroring the resolver service. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
445b943 to
98872aa
Compare
| type cfg struct { | ||
| Config | ||
| plugin ExternalPlugin | ||
| plugin any |
There was a problem hiding this comment.
i don't like any.
Is there no way to have a generic or interface here?
| // If launched by the secrets runtime, they are ignored. | ||
| // If logger is nil, a default logger will be created and used. | ||
| func New(p ExternalPlugin, config Config, opts ...ManualLaunchOption) (Stub, error) { | ||
| func newStub(p any, config Config, opts ...ManualLaunchOption) (Stub, error) { |
There was a problem hiding this comment.
Correctness (pre-existing, in this touched function) — stub timeout accessors always return 0.
newStub builds &stub{name, factory} (around line 81) and never sets registrationTimeout / requestTimeout, so Stub.RegistrationTimeout() and Stub.RequestTimeout() always return 0 — not the default the Stub interface doc comment promises ("the default timeout if the plugin has not been started"). cfg.registrationTimeout is populated (default or WithRegistrationTimeout) but never copied into the stub, and the RequestTimeout returned in the RegisterPlugin response is never consumed either.
Fix: stub{registrationTimeout: cfg.registrationTimeout, ...} and plumb the response RequestTimeout back into the stub. Not introduced by this PR, but flagged since the function was renamed/edited here.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestSetupInterceptor(t *testing.T) { |
There was a problem hiding this comment.
Test coverage — the gate is no longer tested against the real handlers.
TestSetupInterceptor exercises the interceptor in isolation with a next that returns (nil, nil). Nothing now verifies that setup() actually wires the interceptor onto the resolver / access-control handlers, or that a real GetSecrets passes through after setup — coverage the deleted resolver_test.go used to provide (Test_setup only checks shutdown).
A future change that dropped the gated option from httpMux.Handle in setup.go, or mis-wired the access-control handler, would pass all current tests. Consider a setup-level test that asserts requests are blocked before setupCompleted closes and flow through afterwards.
Drop the any-typed plugin field from cfg in favor of typed secretsProviderPlugin/accessControlModule fields. Constructors assign the matching field via a typed closure, so newStub, newCfg, restoreConfig and newCfgForManualLaunch no longer thread an any value. setup dispatches on which typed field is set instead of a runtime type switch.
select evaluates time.After(timeout) on every call, leaking a Timer that lives for the full timeout even after setup completes. Add a non-blocking select fast path that returns immediately once chSetup is closed, so the timer is only allocated while registration is still in progress.
3 participants
Complement the SecretsProvider switch case in plugin setup with an AccessControlModule branch. Add the connect handler adapter and platform-specific proto->domain signing-info mapping, plus a registration-gating wrapper mirroring the resolver service.