Skip to content

Commit 3a21b69

Browse files
fix(live): propagate caller ctx into ResourceGroup inventory ops
Apply/ApplyWithPrune/ResourceGroupCRDMatched passed context.TODO(), so Ctrl-C and command-level timeouts could not cancel in-flight apply/destroy/plan calls. Carry ctx on the InventoryResourceGroup struct and inject it via new WrapInventoryObjWithContext, NewClusterClientFactoryWithContext, ResourceGroupCRDMatchedWithContext, and NewClusterPlannerWithContext. Legacy entry points kept as thin back-compat wrappers; nil ctx normalized to Background(). Signed-off-by: Jaisheesh-2006 <jaicodes2006@gmail.com>
1 parent 9f6dd90 commit 3a21b69

6 files changed

Lines changed: 95 additions & 12 deletions

File tree

commands/alpha/live/plan/command.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ func (r *Runner) RunE(c *cobra.Command, args []string) error {
133133
}
134134

135135
// Create and execute the planner.
136-
planner, err := kptplanner.NewClusterPlanner(r.ctx, r.factory)
136+
planner, err := kptplanner.NewClusterPlannerWithContext(r.ctx, r.factory)
137137
if err != nil {
138138
return err
139139
}

commands/live/apply/cmdapply.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func runApply(r *Runner, invInfo inventory.Info, objs []*unstructured.Unstructur
228228
if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil {
229229
return err
230230
}
231-
} else if !live.ResourceGroupCRDMatched(r.ctx, f) {
231+
} else if !live.ResourceGroupCRDMatchedWithContext(r.ctx, f) {
232232
if err = cmdutil.InstallResourceGroupCRD(r.ctx, f); err != nil {
233233
return &cmdutil.ResourceGroupCRDNotLatestError{
234234
Err: err,

pkg/live/inventoryrg.go

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,15 @@ func WrapInventoryObj(obj *unstructured.Unstructured) inventory.Storage {
109109
// inventory.NewClient's WrapObjFunc parameter. The returned function
110110
// produces an InventoryResourceGroup that carries ctx, so subsequent
111111
// Apply / ApplyWithPrune calls honor the caller's cancellation and
112-
// timeout. ctx MUST NOT be nil; pass context.Background() explicitly if
113-
// you truly want no cancellation.
112+
// timeout.
113+
//
114+
// If ctx is nil it is normalized to context.Background() so callers
115+
// cannot accidentally trigger a nil-deref inside client-go. Prefer
116+
// passing a real ctx (e.g. cmd.Context()) to actually gain cancellation.
114117
func WrapInventoryObjWithContext(ctx context.Context) func(*unstructured.Unstructured) inventory.Storage {
118+
if ctx == nil {
119+
ctx = context.Background()
120+
}
115121
return func(obj *unstructured.Unstructured) inventory.Storage {
116122
if obj != nil {
117123
klog.V(4).Infof("wrapping Inventory obj with ctx: %s/%s\n", obj.GetNamespace(), obj.GetName())
@@ -427,9 +433,22 @@ func ResourceGroupCRDApplied(factory cmdutil.Factory) bool {
427433
// ResourceGroupCRDMatched checks if the ResourceGroup CRD
428434
// in the cluster matches the CRD in the kpt binary.
429435
//
430-
// ctx is used for the live cluster Get call; cancelling it (e.g. via
431-
// Ctrl-C or a command-level timeout) aborts the check.
432-
func ResourceGroupCRDMatched(ctx context.Context, factory cmdutil.Factory) bool {
436+
// This signature is preserved for backward compatibility with external
437+
// callers; it delegates to ResourceGroupCRDMatchedWithContext with
438+
// context.Background(). Prefer ResourceGroupCRDMatchedWithContext when
439+
// you have a caller context so Ctrl-C and timeouts can abort the check.
440+
func ResourceGroupCRDMatched(factory cmdutil.Factory) bool {
441+
return ResourceGroupCRDMatchedWithContext(context.Background(), factory)
442+
}
443+
444+
// ResourceGroupCRDMatchedWithContext is the context-aware variant of
445+
// ResourceGroupCRDMatched. ctx is used for the live cluster Get call;
446+
// cancelling it (e.g. via Ctrl-C or a command-level timeout) aborts the
447+
// check. A nil ctx is normalized to context.Background().
448+
func ResourceGroupCRDMatchedWithContext(ctx context.Context, factory cmdutil.Factory) bool {
449+
if ctx == nil {
450+
ctx = context.Background()
451+
}
433452
mapper, err := factory.ToRESTMapper()
434453
if err != nil {
435454
klog.V(4).Infof("error retrieving RESTMapper when checking ResourceGroup CRD: %s\n", err)

pkg/live/inventoryrg_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020

2121
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2222
"k8s.io/apimachinery/pkg/runtime/schema"
23+
cmdutil "k8s.io/kubectl/pkg/cmd/util"
2324
"sigs.k8s.io/cli-utils/pkg/apis/actuation"
2425
"sigs.k8s.io/cli-utils/pkg/common"
2526
"sigs.k8s.io/cli-utils/pkg/inventory"
@@ -277,6 +278,41 @@ func TestWrapInventoryObj_LeavesContextNil(t *testing.T) {
277278
}
278279
}
279280

281+
// TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground proves the
282+
// factory is nil-safe: passing a nil ctx cannot produce a wrapper that
283+
// would nil-deref inside client-go. The stored ctx is normalized to
284+
// context.Background() at factory construction time.
285+
func TestWrapInventoryObjWithContext_NilCtxDefaultsToBackground(t *testing.T) {
286+
//nolint:staticcheck // SA1012: deliberately passing a nil context to exercise the nil-safety guard.
287+
storage := WrapInventoryObjWithContext(nil)(inventoryObj)
288+
icm, ok := storage.(*InventoryResourceGroup)
289+
if !ok {
290+
t.Fatalf("WrapInventoryObjWithContext(nil) produced unexpected type %T", storage)
291+
}
292+
if icm.ctx == nil {
293+
t.Fatal("expected nil ctx to be normalized to Background(); got nil")
294+
}
295+
// Background() never cancels; Done() returns a nil channel.
296+
if icm.ctx.Done() != nil {
297+
t.Fatalf("expected Background()-equivalent ctx; Done() returned non-nil")
298+
}
299+
}
300+
301+
// TestResourceGroupCRDMatched_BackCompatSignaturePreserved is a
302+
// compile-time guard that the legacy ResourceGroupCRDMatched(factory)
303+
// signature is still exported, alongside the new context-aware
304+
// ResourceGroupCRDMatchedWithContext(ctx, factory). If either function
305+
// is renamed, removed, or has its signature changed, this test stops
306+
// compiling and the API-compat break is visible immediately.
307+
//
308+
// We don't invoke them here because both require a live cmdutil.Factory
309+
// and short-circuit only after several cluster/mapper calls; their
310+
// runtime behavior is exercised by the apply/destroy e2e tests.
311+
func TestResourceGroupCRDMatched_BackCompatSignaturePreserved(t *testing.T) {
312+
var _ func(cmdutil.Factory) bool = ResourceGroupCRDMatched
313+
var _ func(context.Context, cmdutil.Factory) bool = ResourceGroupCRDMatchedWithContext
314+
}
315+
280316
// TestContextOrBackground covers both the override path (caller-supplied
281317
// ctx is returned verbatim, including cancellation state) and the
282318
// fallback path (nil ctx becomes context.Background()).

pkg/live/planner/cluster.go

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,23 @@ type ClusterPlanner struct {
4747
resourceFetcher ResourceFetcher
4848
}
4949

50-
// NewClusterPlanner builds a ClusterPlanner. ctx is the caller's context
51-
// and is plumbed into the inventory wrapper so Apply / ApplyWithPrune on
52-
// the underlying ResourceGroup honor caller cancellation (Ctrl-C,
53-
// deadlines).
54-
func NewClusterPlanner(ctx context.Context, f util.Factory) (*ClusterPlanner, error) {
50+
// NewClusterPlanner builds a ClusterPlanner using context.Background()
51+
// for the underlying inventory-client cluster calls.
52+
//
53+
// This signature is preserved for backward compatibility with external
54+
// callers; it delegates to NewClusterPlannerWithContext. Prefer the
55+
// context-aware constructor when you have a caller context so Ctrl-C
56+
// and command-level timeouts can cancel inventory I/O.
57+
func NewClusterPlanner(f util.Factory) (*ClusterPlanner, error) {
58+
return NewClusterPlannerWithContext(context.Background(), f)
59+
}
60+
61+
// NewClusterPlannerWithContext is the context-aware variant of
62+
// NewClusterPlanner. ctx is plumbed into the inventory wrapper so
63+
// Apply / ApplyWithPrune on the underlying ResourceGroup honor caller
64+
// cancellation (Ctrl-C, deadlines). A nil ctx is normalized to
65+
// context.Background() by WrapInventoryObjWithContext.
66+
func NewClusterPlannerWithContext(ctx context.Context, f util.Factory) (*ClusterPlanner, error) {
5567
fetcher, err := NewResourceFetcher(f)
5668
if err != nil {
5769
return nil, err

pkg/live/planner/cluster_test.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/google/go-cmp/cmp"
2222
"github.com/stretchr/testify/require"
2323
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
24+
"k8s.io/kubectl/pkg/cmd/util"
2425
"sigs.k8s.io/cli-utils/pkg/apply"
2526
"sigs.k8s.io/cli-utils/pkg/apply/event"
2627
"sigs.k8s.io/cli-utils/pkg/inventory"
@@ -166,3 +167,18 @@ func (fii *FakeInventoryInfo) ID() string {
166167
func (fii *FakeInventoryInfo) Strategy() inventory.Strategy {
167168
return inventory.NameStrategy
168169
}
170+
171+
// TestNewClusterPlanner_BackCompatSignaturePreserved is a compile-time
172+
// guard that both the legacy NewClusterPlanner(f) entry point and the
173+
// new context-aware NewClusterPlannerWithContext(ctx, f) remain
174+
// exported with their current signatures. If either is renamed,
175+
// removed, or has its parameter list changed, this test stops
176+
// compiling and the API-compat break is visible immediately.
177+
//
178+
// Runtime behavior of both constructors is exercised through the
179+
// command-level tests that instantiate the planner via the real
180+
// factory in commands/alpha/live/plan.
181+
func TestNewClusterPlanner_BackCompatSignaturePreserved(t *testing.T) {
182+
var _ func(util.Factory) (*ClusterPlanner, error) = NewClusterPlanner
183+
var _ func(context.Context, util.Factory) (*ClusterPlanner, error) = NewClusterPlannerWithContext
184+
}

0 commit comments

Comments
 (0)