test/integration/client: add TestExecPluginRotationViaInformer #101726
Conversation
k8s-ci-robot
added
release-note-none
|
/sig auth |
k8s-ci-robot
added
the
sig/auth
| @@ -18,4 +18,9 @@ set -o errexit | |||
| set -o nounset | |||
| set -o pipefail | |||
|
|
|||
| if [[ -n "${EXEC_PLUGIN_OUTPUT_FILE-""}" ]]; then | |||
There was a problem hiding this comment.
I really wanted to collapse this shell script into a single line: cat "$EXEC_PLUGIN_OUTPUT_FILE". I got a little bit lazy because it would take an hour or two of refactoring the rest of the test file. I was also cognizant of trying to keep the blast radius smaller on this PR. I can certainly do the refactor, though, if you think that would be valuable.
ankeesler
force-pushed
the
exec-plugin-integration-test
branch
5 times, most recently
from
f6706c6
to
3df7846
Compare
|
/assign @enj |
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
| } else { | ||
| if !errors.Is(err, wait.ErrWaitTimeout) { |
There was a problem hiding this comment.
😬 so we wait for 30 seconds in this case?
There was a problem hiding this comment.
Yeah :(. Same with waitForInformerSync when !wantSynced. I could shorten the timeout to like 10 seconds when !wantEvents, but I was worried that would produce some false positives. I looked around in other integration test code and it seemed like there was some precedent of 30 second timeouts. I tried changing the timeouts to 20 seconds and the test failed locally...
test/integration/client/exec_test.go
Outdated
| func waitForInformerSync(t *testing.T, informer cache.SharedIndexInformer, wantSynced bool, lastSyncResourceVersion string) { | ||
| t.Helper() | ||
|
|
||
| err := wait.PollImmediate(time.Second, time.Second*30, func() (bool, error) { | ||
| return informer.HasSynced(), nil | ||
| }) | ||
| if wantSynced { | ||
| if err != nil { | ||
| t.Fatalf("wanted sync, but got error: %v", err) | ||
| } | ||
| } else { | ||
| if !errors.Is(err, wait.ErrWaitTimeout) { | ||
| if err != nil { | ||
| t.Fatalf("wanted no sync, but got error: %v", err) | ||
| } else { | ||
| t.Fatalf("wanted no sync, but got sync") | ||
| } | ||
| } |
There was a problem hiding this comment.
I was originally using WaitForCacheSync, but switched to this way because...I can't remember? I'll switch back to WaitForCacheSync until I remember why I did it this more complicated way...I should really comment my code more...shakes fist at my time in a previous organization where I picked up this habit. Fixed here: 3b447d4a53a.
| @@ -18,4 +18,9 @@ set -o errexit | |||
| set -o nounset | |||
| set -o pipefail | |||
|
|
|||
| if [[ -n "${EXEC_PLUGIN_OUTPUT_FILE-""}" ]]; then | |||
| time.Sleep(lifetime) | ||
| } | ||
|
|
||
| func TestExecPluginRotationViaInformer(t *testing.T) { |
There was a problem hiding this comment.
t.Parallel()? How long does this test take to run?
There was a problem hiding this comment.
On my machine, make test-integration WHAT="./test/integration/client" KUBE_TEST_ARGS="-run TestExecPluginRotat" runs in 138.622s (before any changes made in response to PR comments).
Is it OK for an integration test to run in parallel that makes writes to the API? I was worried that I would screw up other tests (e.g., we create a Namespace and a ConfigMap in this test). I'm all for adding a t.Parallel() if that wouldn't mess anything else up: e9c17c84b6a.
There was a problem hiding this comment.
Is it OK for an integration test to run in parallel that makes writes to the API?
Each test (or maybe it is only test suite) is supposed to have it own sub section of etcd so it should be fine to run in parallel.
test/integration/client/exec_test.go
Outdated
| // Wait for old token to expire. | ||
| time.Sleep(lifetime) |
There was a problem hiding this comment.
I am not sure I understand what this does in the context of the test which always seems to pass in 5 seconds?
There was a problem hiding this comment.
This line pauses the test so that the exec authenticator will need to be triggered again to pick up the new token. If we didn't have this, a line like this might fail because the informer recreates its watch within the lifetime of the clientAuthorizedToken: https://github.com/kubernetes/kubernetes/blob/3df78461ad2cdf161d09c681372b3b99412cb435/test/integration/client/exec_test.go#L642.
I suppose everywhere else we could get away without this pause since the informer would eventually pick up the new token over the course of our timeouts (e.g., https://github.com/kubernetes/kubernetes/blob/3df78461ad2cdf161d09c681372b3b99412cb435/test/integration/client/exec_test.go#L631).
I wrote this a different way - do you think this is better? This would get rid of 15s of unnecessary waiting (new test runs in 125.988s) and potentially some confusing test code: 180e7c4aa86.
ankeesler
force-pushed
the
exec-plugin-integration-test
branch
from
3df7846
to
e9c17c8
Compare
Signed-off-by: Andrew Keesler <akeesler@vmware.com>
ankeesler
force-pushed
the
exec-plugin-integration-test
branch
from
e9c17c8
to
a14cd8e
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankeesler, enj 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
/cc @enj @liggitt