Handle DeletedFinalStateUnknown in k8s OnDelete

[ChrsMark]

What does this PR do?

We can get DeletedFinalStateUnknown instead of *kubernetes.Resource on OnDelete method of ResourceEventer and we need to handle that correctly.

Why is it important?

Without this Beats will fail with panic.

Related issues

• Closes Occasional panic in kubernetes autodiscovery on pod deletion #23385

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #23419 updated

• Start Time: 2021-01-12T12:13:26.578+0000

• Duration: 46 min 22 sec

Test stats 🧪

Test	Results
Failed	0
Passed	17370
Skipped	1345
Total	18715

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	17370
Skipped	1345
Total	18715

[elasticmachine]

Pinging @elastic/integrations (Team:Integrations)

[jsoriano]

Looks good, but maybe we should do it at the watcher level, let me know what you think.

[jsoriano]

Suggested change

	service, isNode := obj.(*kubernetes.Service)
	// We can get DeletedFinalStateUnknown instead of *kubernetes.Service here and we need to handle that correctly. #23385
	if !isNode {
	service, isService := obj.(*kubernetes.Service)
	// We can get DeletedFinalStateUnknown instead of *kubernetes.Service here and we need to handle that correctly. #23385
	if !isService {

[jsoriano]

This is exposing details from the watcher and the client cache in the handlers. DeleteFinalStateUnknown happens when a informer misses the delete event, and it sees in a resync that an object is not there anymore. As it doesn't know what was the final state, it includes the last known state, that in our case is probably enough to remove configurations.

Also, handling this in the handler level forces us to repeat some code in each handler we add.

Wdyt about trying to handle it in the watcher directly?

For example watcher.enqueue() already does something related to DeletedFinalStateUnknown when it calls cache.DeletionHandlingMetaNamespaceKeyFunc. We could maybe modify enqueue to do something like this:

func (w *watcher) enqueue(obj interface{}, state string) {
        // DeletionHandlingMetaNamespaceKeyFunc that we get a key only if the resource's state is not Unknown.
        key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj)
        if err != nil {
                return
        }
        if deleted, ok := obj.(cache.DeletedFinalStateUnknown); ok {
                w.logger.Debugf(.....)
                obj = deleted.Obj
        }
        w.queue.Add(&item{key, obj, state})
}

[ChrsMark]

Looks good, but maybe we should do it at the watcher level, let me know what you think.

It is better at watcher level yes, thank you! I moved the check there.

[jsoriano]

Looks good, thanks! Would it be possible to add some test for this?

[jsoriano]

Would it be possible to add some test?

[ChrsMark]

I’m afraid it is not possible right now since we need to trigger the watcher with an actual event and even the fake k8s client cannot achieve that.