Delete options removed by controller runtime intermediary client

[ryanmcnamara]

Expected: Delete options passed into Resources.Delete are used by controller-runtime
Actual: Delete options passed into Resources.Delete are removed by controller-runtime

Calling like

err = r.Delete(ctx, &job, resources.WithDeletePropagation(string(metav1.DeletePropagationBackground)))

The cr delete options are set so that all fields are empty except for Raw.
See

e2e-framework/klient/k8s/resources/resources.go

Line 139 in ce6e190

o := &cr.DeleteOptions{Raw: deleteOptions}

Controller runtime constructs its own DeleteOptions from these options here https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/typed_client.go#L80

This ends up calling https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/options.go#L300 which will result in still only Raw being set

then AsDeleteOptions is called at the end of typedClient.Delete which sets the fields of Raw directly from the member fields of the DeleteOptions https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/options.go#L273, those fields are not set so it ends up zeroing out the fields of Raw.

It seems like controller runtime does not expect Raw to be set and overrides all fields set in it.

Confirmed the following workaround works:

deletionPropagation := metav1.DeletePropagationBackground
err = r.GetControllerRuntimeClient().Delete(ctx, &job, &cr.DeleteOptions{PropagationPolicy: &deletionPropagation})

[harshanarayana]

@ryanmcnamara Oh thanks for noticing this one. I believe you are right.

I think the following should take care of the necessary configuration. (I still have to test this out, but this should take care of the issue you are talking about)

func (r *Resources) Delete(ctx context.Context, obj k8s.Object, opts ...DeleteOption) error {
	deleteOptions := &metav1.DeleteOptions{}
	for _, fn := range opts {
		fn(deleteOptions)
	}

	o := &cr.DeleteOptions{
		Raw:                deleteOptions,
		GracePeriodSeconds: deleteOptions.GracePeriodSeconds,
		Preconditions:      deleteOptions.Preconditions,
		PropagationPolicy:  deleteOptions.PropagationPolicy,
		DryRun:             deleteOptions.DryRun,
	}
	return r.client.Delete(ctx, obj, o)
}

[harshanarayana]

Or we could just turn the type DeleteOption func(*metav1.DeleteOptions) into type DeleteOption func(*cr.DeleteOptions)

type DeleteOption func(*cr.DeleteOptions)

func (r *Resources) Delete(ctx context.Context, obj k8s.Object, opts ...DeleteOption) error {
	deleteOptions := &cr.DeleteOptions{}
	for _, fn := range opts {
		fn(deleteOptions)
	}

	return r.client.Delete(ctx, obj, deleteOptions)
}

func WithGracePeriod(gpt time.Duration) DeleteOption {
	t := gpt.Milliseconds()
	return func(do *cr.DeleteOptions) { do.GracePeriodSeconds = &t }
}

func WithDeletePropagation(prop string) DeleteOption {
	p := metav1.DeletionPropagation(prop)
	return func(do *cr.DeleteOptions) { do.PropagationPolicy = &p }
}

I hope doing this will not break some backward compatibility of the framework.

[harshanarayana]

cc @vladimirvivien which one do you think is the best way to get this one fixed ?

[negz]

FWIW I just hit the same thing with resources.PatchOption.

https://github.com/kubernetes-sigs/controller-runtime/blob/451530c09937350ff7f5ebf7a00d059b4334e8ee/pkg/client/options.go#L761

[vladimirvivien]

Thanks @ryanmcnamara for the comprehensive explanation of this issue.

@harshanarayana I think your second proposal is closer to how other operations are done in the package.

[harshanarayana]

I will open a PR with these modified.. We can discuss further in that if we want to change something

[vladimirvivien]

@harshanarayana I am taking a closer look. Using the cr.DeleteOption will be a one-of and will break backward compatibility. Let's do the first one.

[vladimirvivien]

Sorry for confusion.

[harshanarayana]

No worries.. But it looks like PatchOption needs a fix too.