Fake client does not delete objects containing deletion timestamp

[lleshchi]

The fake client Get(), List(), Patch() methods do not account for objects that have been marked for deletion, allowing for viewing of and changes to objects that are expected to be deleted. The Update() method does delete the object, but I believe it should return an error, since the update of a deleted object has failed.

This is a separate issue from #138

[2uasimojo]

The Update() method does delete the object, but I believe it should return an error, since the update of a deleted object has failed.

So actually there are two possibilities here:

• If this Update() removed the last finalizer(s), then the current behavior is correct. However...
• If there were no finalizers before this Update() was called, we should simulate gc having happened prior to the Update() and error NotFound.

Glancing at the code, it should be easy enough to make this distinction:

if len(oldAccessor.GetFinalizers()) > 0 && len(accessor.GetFinalizers()) == 0 {

[alvaroaleman]

How did you get an object with a non-nil deletionTimestamp into the fakeclient in the first place?

[lleshchi]

Hi @alvaroaleman, we use WithRuntimeObjects() here to add a DeletionTimestamp. Since the k8s api does not allow a user to add a deletion timestamp when creating an object, and ignores attempts to add/edit the timestamp, perhaps the fake client should handle deletion timestamps similarly, in order to prevent tests such as the one above.

[2uasimojo]

Right, so to expand on this:

In general we want to be able to test reconciler behavior on deleted objects. With the fake client as it stands, we've been able to write test cases where we just init the fake client With[Runtime]Objects() that have deletionTimestamp set, but no finalizers -- which is not a valid state in real k8s (ignoring delete propagation, which we're not looking to solve here).

The argument of this issue is that test authors should be disallowed from getting into this state, as it throws the validity of the test case into question.

Originally we were thinking that the fake client should just stealthily treat those objects the way k8s would: by deleting them from the database any time it sees them. But your comment has made us discuss and realize that it would be better to prevent getting into that situation at all. So I think we're now proposing:

• Reader methods (Get(), List()) remain unchanged.
• Update() and Patch() should reject changes that set a previously-unset deletionTimestamp, like real k8s does.
• Create() should ignore a deletionTimestamp on an object, like real k8s does. (This doesn't seem ideal as it may be tough for test authors to figure out what broke when they upgrade their fake client to include this fix -- but getting closer to parity with real k8s is the only thing that makes sense. IDK if there's a way for the fake client to generate a warning message?)
• The builder should error if initialized with an object that has a deletionTimestamp but no finalizers; but accept a deleted object (including keeping its deletionTimestamp set) if it has finalizers.

That last bullet is important: We don't have equivalent k8s behavior to guide us, so we have the freedom to make it as useful as possible for test writers. If the builder ignored deletionTimestamp like Create(), test authors would need to explicitly Delete() the object after building. In table-driven tests like the ones hive uses, that would often/usually entail adding a field to the test config struct telling the driver to issue the Delete(). And if the test relies on multiple objects being deleted, that would need to be done for each. Also, this behavior change would be difficult to understand when upgrading the fake client to include this change. Whereas if we throw an error, we can make it nice and descriptive: "Refusing to initialize with an object that has a deletionTimestamp but no finalizers. You must either include one or more finalizers or explicitly Delete() the object via the built client."

[vincepri]

/milestone v0.15.x

[vincepri]

/assign @lleshchi
/lifecycle active

[vincepri]

/kind bug