Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 25 additions & 37 deletions cmd/wfctl/deploy_providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -487,37 +487,36 @@ func (r *remoteIaCProvider) DetectDrift(_ context.Context, refs []interfaces.Res
return drifts, nil
}

// DetectDriftWithApplied dispatches to the v2 RPC if the remote plugin
// supports it, falling back to legacy DetectDrift when the remote returns
// "method not found". This is the wire-format counterpart of the type-
// assertion fallback in callers; both layers need the fallback because:
// - Caller-side: provider may not implement the interface (type-
// assertion fails → legacy path).
// - Wire-side: provider implements the interface in newer code but the
// LOADED plugin binary on disk is older (manifest accepted but RPC
// dispatch returns method-not-found).
func (r *remoteIaCProvider) DetectDriftWithApplied(ctx context.Context, refs []interfaces.ResourceRef, applied map[string]map[string]any) ([]interfaces.DriftResult, error) {
// DetectDriftWithSpecs dispatches IaCProvider.DetectDrift with a "specs" arg
// map so the remote plugin can perform config-level drift detection (in
// addition to ghost/in-sync existence checks). This aligns with the DO plugin
// v0.10.5 wire protocol: the plugin dispatches on "IaCProvider.DetectDrift"
// and checks for a "specs" key in the args to activate the spec-injection
// path. No separate RPC method name is needed.
//
// The fallback to legacy DetectDrift (no specs) is preserved at the caller
// level (infra_apply_refresh.go, infra_status_drift.go) when buildAppliedSpecMap
// returns nil — this function is only invoked when specs are available.
func (r *remoteIaCProvider) DetectDriftWithSpecs(ctx context.Context, refs []interfaces.ResourceRef, specs map[string]interfaces.ResourceSpec) ([]interfaces.DriftResult, error) {
refsAny, err := jsonToAny(refs)
if err != nil {
return nil, fmt.Errorf("IaCProvider.DetectDriftWithApplied: marshal refs: %w", err)
return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal refs: %w", err)
}
appliedAny, err := jsonToAny(applied)
specsAny, err := jsonToAny(specs)
if err != nil {
return nil, fmt.Errorf("IaCProvider.DetectDriftWithApplied: marshal applied: %w", err)
return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): marshal specs: %w", err)
}
res, err := r.invoker.InvokeService("IaCProvider.DetectDriftWithApplied", map[string]any{
"refs": refsAny, "applied": appliedAny,
})
if err != nil {
// Method-not-found from the plugin: fall back to legacy DetectDrift.
// Match on substring to tolerate framework-specific error wrappers
// (HashiCorp go-plugin emits "method not found"; future schemes may
// wrap the literal differently). DO NOT fall back on other errors —
// a transient cloud failure must propagate so callers don't silently
// lose drift signal.
if isMethodNotFound(err) {
return r.DetectDrift(ctx, refs)
args := map[string]any{"refs": refsAny, "specs": specsAny}
var res map[string]any
if invoker, ok := r.invoker.(remoteServiceContextInvoker); ok {
res, err = invoker.InvokeServiceContext(ctx, "IaCProvider.DetectDrift", args)
} else {
if err := ctx.Err(); err != nil {
return nil, err
}
res, err = r.invoker.InvokeService("IaCProvider.DetectDrift", args)
}
if err != nil {
return nil, err
}
raw, ok := res["drifts"]
Expand All @@ -526,22 +525,11 @@ func (r *remoteIaCProvider) DetectDriftWithApplied(ctx context.Context, refs []i
}
var drifts []interfaces.DriftResult
if err := anyToStruct(raw, &drifts); err != nil {
return nil, fmt.Errorf("IaCProvider.DetectDriftWithApplied: decode result: %w", err)
return nil, fmt.Errorf("IaCProvider.DetectDrift(specs): decode result: %w", err)
}
return drifts, nil
}

// isMethodNotFound reports whether err is the canonical signal a remote
// plugin emits when it has no case for the requested RPC method. Tolerant
// substring match across known wrapper formats.
func isMethodNotFound(err error) bool {
if err == nil {
return false
}
msg := err.Error()
return strings.Contains(msg, "method not found") || strings.Contains(msg, "Method not found")
}

func (r *remoteIaCProvider) Import(_ context.Context, cloudID string, resourceType string) (*interfaces.ResourceState, error) {
res, err := r.invoker.InvokeService("IaCProvider.Import", map[string]any{
"provider_id": cloudID,
Expand Down
46 changes: 17 additions & 29 deletions cmd/wfctl/deploy_providers_remote_iac_compat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,54 +2,42 @@ package main

import (
"context"
"errors"
"testing"

"github.com/GoCodeAlone/workflow/interfaces"
)

// TestRemoteIaC_OptionalDriftConfigDetector_FallsBackOnLegacyPlugin pins the
// pipeline's compat story: when a remote plugin does NOT support
// IaCProvider.DetectDriftWithApplied (legacy plugin: returns "method not
// found"), the wfctl caller falls through to legacy IaCProvider.DetectDrift.
//
// This is the wire-format counterpart of the type-assertion fallback in
// runInfraApplyRefreshPhase / runInfraStatusDrift; without this gate, a v0.22
// wfctl talking to a v0.10.3 DO plugin (or an out-of-tree plugin) could trip
// a hard error instead of falling back.
func TestRemoteIaC_OptionalDriftConfigDetector_FallsBackOnLegacyPlugin(t *testing.T) {
// TestRemoteIaC_DriftConfigDetector_SendsSpecsViaDetectDrift pins the wire
// protocol: DetectDriftWithSpecs sends "IaCProvider.DetectDrift" with a
// "specs" arg map. The DO plugin v0.10.5+ dispatches spec-injection inside
// IaCProvider.DetectDrift when the "specs" key is present — no separate RPC
// method name is required.
func TestRemoteIaC_DriftConfigDetector_SendsSpecsViaDetectDrift(t *testing.T) {
si := &multiMethodStubInvoker{
// Method not found = canonical signal a legacy plugin emits when
// it doesn't have a case for this RPC name.
errByMethod: map[string]error{
"IaCProvider.DetectDriftWithApplied": errors.New("method not found: IaCProvider.DetectDriftWithApplied"),
},
// Legacy DetectDrift returns success.
respByMethod: map[string]map[string]any{
"IaCProvider.DetectDrift": {
"drifts": []any{map[string]any{"name": "x", "type": "infra.test", "drifted": false, "class": "in-sync"}},
"drifts": []any{map[string]any{"name": "x", "type": "infra.test", "drifted": true, "class": "config"}},
},
},
}
p := &remoteIaCProvider{invoker: si}

// Caller-side type-assertion: remoteIaCProvider implements
// DetectDriftWithApplied (it always does, since the wfctl-side wrapper
// ALWAYS exposes the method). The fallback happens INSIDE the wrapper:
// if the remote returns method-not-found, the wrapper retries with
// legacy DetectDrift.
refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}}
drifts, err := p.DetectDriftWithApplied(context.Background(), refs, nil)
specs := map[string]interfaces.ResourceSpec{
"x": {Name: "x", Type: "infra.test", Config: map[string]any{"region": "nyc3"}},
}
drifts, err := p.DetectDriftWithSpecs(context.Background(), refs, specs)
if err != nil {
t.Fatalf("DetectDriftWithApplied should NOT propagate method-not-found; should fall back. err=%v", err)
t.Fatalf("DetectDriftWithSpecs: unexpected error: %v", err)
}
if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassInSync {
t.Errorf("expected fallback InSync drift; got %+v", drifts)
if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig {
t.Errorf("expected config-drift result; got %+v", drifts)
}

// Verify the second call to invoker WAS the legacy method.
// Verify the invoker was called with IaCProvider.DetectDrift (not a
// separate DetectDriftWithSpecs method) — this is the wire contract.
if !si.calledMethods["IaCProvider.DetectDrift"] {
t.Errorf("expected fallback to call IaCProvider.DetectDrift; called methods: %v", si.calledMethods)
t.Errorf("DetectDriftWithSpecs must invoke IaCProvider.DetectDrift; called methods: %v", si.calledMethods)
}
}

Expand Down
24 changes: 17 additions & 7 deletions cmd/wfctl/deploy_providers_remote_iac_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,9 +473,9 @@ func TestRemoteIaC_DetectDrift_Empty(t *testing.T) {
}
}

// ── DetectDriftWithApplied ─────────────────────────────────────────────────────
// ── DetectDriftWithSpecs ───────────────────────────────────────────────────────

func TestRemoteIaC_DetectDriftWithApplied_HappyPath(t *testing.T) {
func TestRemoteIaC_DetectDriftWithSpecs_HappyPath(t *testing.T) {
si := &stubInvoker{resp: map[string]any{
"drifts": []any{map[string]any{
"name": "x",
Expand All @@ -487,13 +487,23 @@ func TestRemoteIaC_DetectDriftWithApplied_HappyPath(t *testing.T) {
}}
p := newIaCProvider(si)
refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.test"}}
applied := map[string]map[string]any{"x": {"region": "nyc1"}}
drifts, err := p.DetectDriftWithApplied(context.Background(), refs, applied)
specs := map[string]interfaces.ResourceSpec{
"x": {Name: "x", Type: "infra.test", Config: map[string]any{"region": "nyc1"}},
}
drifts, err := p.DetectDriftWithSpecs(context.Background(), refs, specs)
if err != nil {
t.Fatalf("DetectDriftWithApplied: %v", err)
t.Fatalf("DetectDriftWithSpecs: %v", err)
}
// Wire protocol: specs are sent via IaCProvider.DetectDrift with "specs" arg.
if si.method != "IaCProvider.DetectDrift" {
t.Errorf("method: got %q, want IaCProvider.DetectDrift", si.method)
}
Comment thread
intel352 marked this conversation as resolved.
// "specs" key must be present; legacy "applied" key must not be present.
if _, ok := si.args["specs"]; !ok {
t.Errorf("InvokeService args must contain 'specs' key; got %v", si.args)
}
if si.method != "IaCProvider.DetectDriftWithApplied" {
t.Errorf("method: got %q, want IaCProvider.DetectDriftWithApplied", si.method)
if _, ok := si.args["applied"]; ok {
t.Errorf("InvokeService args must NOT contain legacy 'applied' key; got %v", si.args)
}
if len(drifts) != 1 || drifts[0].Class != interfaces.DriftClassConfig {
t.Errorf("drifts: %+v", drifts)
Expand Down
8 changes: 4 additions & 4 deletions cmd/wfctl/infra_apply_refresh.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,14 @@ func runInfraApplyRefreshPhase(
}

// Use DriftConfigDetector when the provider supports it (optional interface).
// Short-circuits to legacy DetectDrift when appliedMap is nil (no "apply"-
// Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"-
// provenance entries available) to avoid unnecessary RPC round-trips.
var results []interfaces.DriftResult
var err error
if d, ok := provider.(interfaces.DriftConfigDetector); ok {
appliedMap := buildAppliedSpecMap(states, refs)
if appliedMap != nil {
results, err = d.DetectDriftWithApplied(ctx, refs, appliedMap)
specsMap := buildAppliedSpecMap(states, refs)
if specsMap != nil {
results, err = d.DetectDriftWithSpecs(ctx, refs, specsMap)
} else {
results, err = provider.DetectDrift(ctx, refs)
}
Expand Down
24 changes: 17 additions & 7 deletions cmd/wfctl/infra_drift_applied.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ package main

import "github.com/GoCodeAlone/workflow/interfaces"

// buildAppliedSpecMap walks states + refs and returns the per-ref applied-
// config map for DriftConfigDetector.DetectDriftWithApplied. Entries whose
// AppliedConfigSource is anything other than "apply" are OMITTED from the
// returned map — providers cannot meaningfully compute config-drift against
// buildAppliedSpecMap walks states + refs and returns the per-ref
// ResourceSpec map for DriftConfigDetector.DetectDriftWithSpecs. Entries
// whose AppliedConfigSource is anything other than "apply" are OMITTED from
// the returned map — providers cannot meaningfully compute config-drift against
// adoption-shaped Outputs (would yield false-positives) and legacy state
// (no provenance recorded) defaults to adoption treatment per ADR 0010.
//
Expand All @@ -14,15 +14,15 @@ import "github.com/GoCodeAlone/workflow/interfaces"
//
// Returns nil when no safe entries exist, so callers can short-circuit the
// type-assertion entirely and fall back to legacy DetectDrift.
func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.ResourceRef) map[string]map[string]any {
func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.ResourceRef) map[string]interfaces.ResourceSpec {
if len(states) == 0 || len(refs) == 0 {
return nil
}
byName := make(map[string]*interfaces.ResourceState, len(states))
for i := range states {
byName[states[i].Name] = &states[i]
}
out := make(map[string]map[string]any, len(refs))
out := make(map[string]interfaces.ResourceSpec, len(refs))
for _, ref := range refs {
st, ok := byName[ref.Name]
if !ok {
Expand All @@ -42,7 +42,17 @@ func buildAppliedSpecMap(states []interfaces.ResourceState, refs []interfaces.Re
for k, v := range st.AppliedConfig {
cfg[k] = v
}
out[ref.Name] = cfg
// Prefer st.Type (canonical from state) over ref.Type (from grouping
// which may be a lightweight ref without full type info).
specType := st.Type
if specType == "" {
specType = ref.Type
}
out[ref.Name] = interfaces.ResourceSpec{
Name: ref.Name,
Type: specType,
Config: cfg,
}
}
if len(out) == 0 {
return nil
Expand Down
48 changes: 37 additions & 11 deletions cmd/wfctl/infra_drift_applied_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,42 @@ func TestBuildAppliedSpecMap_OmitsAdoptionAndLegacy(t *testing.T) {
}

got := buildAppliedSpecMap(states, refs)
want := map[string]map[string]any{
"apply-resource": {"k": "v"},
// adoption-resource: omitted (refuse false-positive on adoption)
// legacy-resource: omitted (legacy default-to-adoption)
// nil-config-resource: omitted (nil AppliedConfig)
// empty-map-config-resource: omitted (len 0 — same branch as nil)
// missing-from-state: omitted (no state)
// adoption-resource: omitted (refuse false-positive on adoption)
// legacy-resource: omitted (legacy default-to-adoption)
// nil-config-resource: omitted (nil AppliedConfig)
// empty-map-config-resource: omitted (len 0 — same branch as nil)
// missing-from-state: omitted (no state)
if len(got) != 1 {
t.Fatalf("expected 1 entry in result, got %d: %v", len(got), got)
}
spec, ok := got["apply-resource"]
if !ok {
t.Fatalf("expected 'apply-resource' in result; got %v", got)
}
if spec.Name != "apply-resource" {
t.Errorf("spec.Name: got %q, want %q", spec.Name, "apply-resource")
}
if !reflect.DeepEqual(spec.Config, map[string]any{"k": "v"}) {
t.Errorf("spec.Config: got %v, want %v", spec.Config, map[string]any{"k": "v"})
}
}

if !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
// TestBuildAppliedSpecMap_PrefersStateTypeOverRefType verifies that when
// ResourceState.Type is set, it takes precedence over ref.Type in the
// ResourceSpec output (state is canonical; ref may be a lightweight lookup).
func TestBuildAppliedSpecMap_PrefersStateTypeOverRefType(t *testing.T) {
states := []interfaces.ResourceState{
{Name: "x", Type: "infra.database", AppliedConfig: map[string]any{"k": "v"}, AppliedConfigSource: "apply"},
}
refs := []interfaces.ResourceRef{{Name: "x", Type: "infra.other"}} // ref has wrong type

got := buildAppliedSpecMap(states, refs)
spec, ok := got["x"]
if !ok {
t.Fatalf("expected 'x' in result")
}
if spec.Type != "infra.database" {
t.Errorf("spec.Type: got %q, want %q (state type should win)", spec.Type, "infra.database")
}
}

Expand Down Expand Up @@ -75,8 +100,9 @@ func TestBuildAppliedSpecMap_ShallowCopyPreventsCallerMutation(t *testing.T) {
refs := []interfaces.ResourceRef{{Name: "x"}}

got := buildAppliedSpecMap(states, refs)
// Mutate the returned map; verify the source state is not affected.
got["x"]["k"] = "mutated"
// Mutate the returned spec's Config; verify the source state is not affected.
spec := got["x"]
spec.Config["k"] = "mutated"
if states[0].AppliedConfig["k"] == "mutated" {
t.Errorf("buildAppliedSpecMap must return a shallow copy; source state was mutated")
}
Expand Down
8 changes: 4 additions & 4 deletions cmd/wfctl/infra_status_drift.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,13 @@ func driftInfraModules(ctx context.Context, cfgFile, envName string) error {
}

// Use DriftConfigDetector when the provider supports it (optional interface).
// Short-circuits to legacy DetectDrift when appliedMap is nil (no "apply"-
// Short-circuits to legacy DetectDrift when specsMap is nil (no "apply"-
// provenance entries available) to avoid unnecessary RPC round-trips.
var results []interfaces.DriftResult
if d, ok := provider.(interfaces.DriftConfigDetector); ok {
appliedMap := buildAppliedSpecMap(states, g.refs)
if appliedMap != nil {
results, err = d.DetectDriftWithApplied(ctx, g.refs, appliedMap)
specsMap := buildAppliedSpecMap(states, g.refs)
if specsMap != nil {
results, err = d.DetectDriftWithSpecs(ctx, g.refs, specsMap)
} else {
results, err = provider.DetectDrift(ctx, g.refs)
}
Expand Down
Loading
Loading