use admission.Handler readyFunc for CEL Admission plugin

[alexzielenski]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This replaces a usage of cache.WaitForNamedCacheSync with the standard readyfunc seen in other admission plugins. The old usage was very verbose in logs and would log two unnecessary messages shown for every object create/update/delete

This is for the CEL ValidatingAdmissionPolicy plugin about to become alpha in 1.26.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/cc @andrewsykim @cici37 @jpbetz

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexzielenski

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

The pull request process is described here

Needs approval from an approver in each of these files:

• staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/OWNERS [alexzielenski]

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

[alexzielenski]

this test takes 10s to complete due to:

kubernetes/staging/src/k8s.io/apiserver/pkg/admission/handler.go

Lines 26 to 28 in 76b4699

	// timeToWaitForReady is the amount of time to wait to let an admission controller to be ready to satisfy a request.
	// this is useful when admission controllers need to warm their caches before letting requests through.
	timeToWaitForReady = 10 * time.Second

I'm not sure if it is best to include it or selectively enable it, etc. Or is it ok because it is run in parallel with many others?

[andrewsykim]

You could create a new timeout field in handler that defaults to timeToWaitForReady, and a new method SetTimeout() to override it from tests.

[alexzielenski]

I agree that is probably ideal. I was hoping to keep the change as low impact as possible due to code freeze/approvals. I wonder if that is something the release managers might allow to go in past code freeze?

[cici37]

It is ok to update with suggested fix if you like since it is considered as a fix plus reviewed/approved before code freeze.

[cici37]

The code changes in admission.go lgtm. Would love to have someone else confirm with the test time

[andrewsykim]

I would consider this a bug found from integration tests in #113738 -- logging V=0 on every requests is too verbose. Also tagging v1.26 since this PR had approval yesterday. The test fix is probably fine as a follow-up before test freeze (or a tracking issue at the very least).

/kind bug
/milestone v1.26

[andrewsykim]

/lgtm

[andrewsykim]

/hold

The CI failure looks legitimate

[alexzielenski]

@andrewsykim this pr conflicted with the other I had that merged. rebased and fixed test again. PTAL

[andrewsykim]

@andrewsykim this pr conflicted with the other I had that merged. rebased and fixed test again. PTAL

I might be missing something, but it seemed like the test just timed out, unrelated to other change

I1109 22:17:29.485159   72342 shared_informer.go:273] Waiting for caches to sync for cel-policy-definitions
panic: test timed out after 3m0s

goroutine 11 [running]:
testing.(*M).startAlarm.func1()
	/usr/local/go/src/testing/testing.go:2036 +0xbb
created by time.goFunc
	/usr/local/go/src/time/sleep.go:176 +0x48

goroutine 1 [chan receive, 3 minutes]:
testing.(*T).Run(0xc000309380, {0x2bc23bd, 0x12}, 0x2ca17a8)
	/usr/local/go/src/testing/testing.go:1494 +0x789
testing.runTests.func1(0x0?)
	/usr/local/go/src/testing/testing.go:1846 +0x9a
testing.tRunner(0xc000309380, 0xc0006dfba0)
	/usr/local/go/src/testing/testing.go:1446 +0x217
testing.runTests(0xc0004d4fa0?, {0x413ba60, 0xc, 0xc}, {0x40?, 0x7f337874f140?, 0x41599c0?})
	/usr/local/go/src/testing/testing.go:1844 +0x7ed
testing.(*M).Run(0xc0004d4fa0)
	/usr/local/go/src/testing/testing.go:1726 +0xa85
main.main()
	_testmain.go:69 +0x2ea

[alexzielenski]

but it seemed like the test just timed out, unrelated to other change

It's related. There is a log not included in your post Waiting for caches to sync for initial sync. This is the new sync added by the other pr that blocks forever if the informer is not started. Since this pr adds the possibility to not stop the informer, they conflicted with each other in that sense.

To solve that, I changed setupTestCommon to early return before the WaitForNamedCacheSync: https://github.com/kubernetes/kubernetes/pull/113758/files#diff-c66c2bd982171e72da2080e5a373924930ecdd30597aab5e413cc798e52d2c7cR287-R289

[andrewsykim]

/lgtm

[andrewsykim]

/hold cancel

[alexzielenski]

/triage accepted