feat(iac): wire RevokeProviderCredential through remoteIaCProvider#574
Merged
Conversation
Add RevokeProviderCredential to remoteIaCProvider so that wfctl's plugin-backed IaC provider participates in the ProviderCredentialRevoker interface. Without this method, the type assertion in infra_bootstrap.go always fails for remote (plugin-loaded) providers, silently skipping revocation. Uses the existing context-aware / context-free invoker duality (remoteServiceContextInvoker) so cancellation is honoured where the plugin supports it. Includes compile-time interface assertion test to catch signature drift between interface and dispatch layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires RevokeProviderCredential through remoteIaCProvider so plugin-backed IaC providers can participate in credential revocation during wfctl infra bootstrap --force-rotate, instead of being skipped by the ProviderCredentialRevoker type assertion path.
Changes:
- Added
RevokeProviderCredential(ctx, source, credentialID)toremoteIaCProvider, dispatching toIaCProvider.RevokeProviderCredentialvia the remote service invoker (context-aware when available). - Added unit tests covering happy path forwarding, error propagation, context-aware invocation, and interface satisfaction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/wfctl/deploy_providers.go | Adds remoteIaCProvider.RevokeProviderCredential dispatch to the plugin subprocess. |
| cmd/wfctl/deploy_providers_remote_iac_test.go | Adds tests validating forwarding/behavior for RevokeProviderCredential and interface implementation. |
Comment on lines
+628
to
+629
| "source": source, | ||
| "credentialID": credentialID, |
Comment on lines
+717
to
+718
| if si.args["credentialID"] != "AKID123" { | ||
| t.Errorf("args[credentialID]: got %q, want AKID123", si.args["credentialID"]) |
The rest of the remoteIaCProvider wire protocol uses snake_case keys (provider_id, resource_type, ref_name). Rename camelCase credentialID → credential_id to stay consistent and avoid silent mismatches if/when args are serialised via proto JSON. Update the test assertion to match the corrected key name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
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
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.
Summary
RevokeProviderCredentialtoremoteIaCProviderso that plugin-backed IaC providers participate in theProviderCredentialRevokerinterfaceinfra_bootstrap.goline 331 always fails for remote/plugin-loaded providers, logging "warn: IaC provider does not implement ProviderCredentialRevoker" and skipping revocation entirelyremoteServiceContextInvokerduality so context cancellation is honoured when the plugin supports itRoot cause
The
tc2-rotate-spaces-keys.ymlrotation workflow (run 25503485781) successfully minted new SPACES credentials and stored them in GH secrets, but the old DO Spaces key was NOT revoked at DO becauseremoteIaCProviderlacked this method. This PR closes that gap.Still required (DO plugin side)
workflow-plugin-digitalocean/internal/module_instance.goInvokeMethodContextswitch also needscase "IaCProvider.RevokeProviderCredential"— that fix is a separate DO plugin PR that will follow this one, then core-dump will get a pin bump to bring both fixes through.Test plan
TestRemoteIaCProvider_RevokeProviderCredential— happy path; verifies method name and args forwarded to invokerTestRemoteIaCProvider_RevokeProviderCredential_PropagatesError— error from invoker surfaces throughTestRemoteIaCProvider_RevokeProviderCredential_UsesContext— context-aware invoker branch used when availableTestRemoteIaCProvider_ImplementsProviderCredentialRevoker— compile-time assertionvar _ interfaces.ProviderCredentialRevoker = pTestRemoteIaC*tests pass (GOWORK=off go test -count=1 -run TestRemoteIaC ...)🤖 Generated with Claude Code