🌱 Fake client should set CreationTimestamp

[fabriziopandini]

Fixes #3402

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fabriziopandini
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alvaroaleman]

This is be a very breaking change for any unit test that compares content of the fake client with a reference. I am also not sure if using time.Now is a good idea as opposed to some hardcoded fixed tomrstamp to make comparison easier

[k8s-ci-robot]

@fabriziopandini: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-test	8229d71	link	true	/test pull-controller-runtime-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[fabriziopandini]

@alvaroaleman that's a fair comment, this is why I asked in #3402 before proposing a PR

What do you suggest as a next step? use a const instead of time.Now or do you prefer dropping the change entirely?

[alvaroaleman]

I think the ask will come up sooner or later anyways, so doing this seems fine to me in principle, we just have to mark it as breaking change. Using a static const as creation timestamp allows the comparison to still work and I think doesn't really have much drawback other than being different from what the real KAS does - Any opinion on this @sbueringer ?

[sbueringer]

I think the only problem with using a constant is that it won't work in cases where either prod or test code uses the creationTimestamp to check if it's before/after other timestamps

(I don't have a use case for this though)

[sbueringer]

Just remembered a case. In CAPI we have a controller that rolls out new Machines if they have not been created after a certain timestamp.

Example

• MachineDeployment: rolloutAfter 2025.12.2022 13:00
• controller creates MachineSets/Machines (if there is no MachineSet with creationTimestamp after the timestamp above)

If now the timestamps would be always in the past the controller would never stop creating new MachineSets/Machines.

I imagine other folks might have similar use cases.

[sbueringer]

IIRC we already have the comparison problem with managedFields (only if enabled?)

Maybe also with deletionTimestamp, although not that often with deletionTimestamp of course.

I'm leaning towards moving the fake client closer to reality (i.e. using time.Now or something similar)

We could have an option similar to WithReturnManagedFields to enable returning a specific timestamp (potentially a timestamp passed into that option)

[alvaroaleman]

Hm yeah, fair point regarding the comparison problem. I was going to point out that creationTimestamp isn't very useful for validating one thing happens after another since it only has second granularity but if there is logic around "happened N time in the past" this still causes issues.

It is already possible to set creationTimestamp through the interceptor (Which I think this change breaks?) - Maybe that is enough, as it already provides a configurable opt-in?