-
Notifications
You must be signed in to change notification settings - Fork 589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Apiserversource webhook test #5327
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've added a test for ApiServerSource to make sure webhook is triggered and it does the validation correctly. I didn't want to go beyond this point w/o checking with you first. I wrote the test as if it is a conformance test but there is no spec for ApiServerSource in https://github.com/knative/specs/tree/main/specs/eventing. I see there's a task for that here: #4933. Question: The tests in |
…d to refetch the resource. ...and, we do that already!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Couple of tiny things since still in wip mode :)
|
||
sinkRef := &duckv1.KReference{ | ||
Kind: "sinkkind", | ||
Namespace: "sinknamespace", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if above this we might want to make the comment about namespace being not used? Or above where you skipped it? Cause here when you create it with this namespace, it might not be clear to the user why L136 is not sinknamespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the namespace shouldn't be skipped here as ApiServerSource allows cross namespace sink. I was think it doesn't. So, I will cancel the skipping and also fix the test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit for not skipping the namespace.
// TODO: skip for now | ||
//"resourceNames": resource.ResourceNames, | ||
//"nonResourceURLs": resource.NonResourceURLs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to add these for now in order not to waste time and focus on getting feedback on the general idea of the PR.
Would we ever need these? Happy to add code that handles these too, if necessary.
{{ end }} | ||
|
||
{{ if .rules }} | ||
rules: | ||
{{ range $_, $rule := .rules }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to extend and reuse the existing account_role
here.
In the future if we need to add more functionality to this template, I would create a separate template; but it is manageable for now.
sourceName := "myapiserversource" | ||
|
||
// Install and wait for a Ready ApiServerSource. | ||
env.Prerequisite(ctx, t, apiserversource.GoesReady(sourceName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the source to have an install step or have the name of the resource passed in but I am not seeing either.
// wait until the ApiServerSource is ready or fail. | ||
// otherwise, some race happens and Kubernetes tells us to refetch the object as it was updated, although | ||
// we just refetched the object a couple of lines above. | ||
apiserversource.IsReady(apiServerSource.Name)(ctx, t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will race with the update, likely you will test IsReady before the reconciler acts on the above Update and never see the resource go unready and then ready again.
|
||
f.Stable("EventMode"). | ||
Must("ApiServerSource MUST allow retrieving the event payloads of ObjectReference or ResourceEvent", | ||
eventMode()). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test does not seem to test what the spec is saying. The test confirms we can set the value, but the conformance line is talking about the shape of the event payload I believe, so your test will need to get more involved and see the event on the data plane side.
|
||
sacmName := feature.MakeRandomK8sName("apiserversource") | ||
f.Setup("Create Service Account for ApiServerSource", | ||
account_role.Install(sacmName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the broker/channel changes we found that this pattern of combining Install
with GoesReady
was not idea for test composition (my bad). I would recommend this being split into two methods: GoesReady
which tests that the thing you pass is does go ready (and we have some helpers for that. and Install
which installs the ApiServerSource resource.
@@ -196,3 +197,76 @@ func Example_addressableResolver() { | |||
// name: addressable-resolver-collector-foo | |||
// apiGroup: rbac.authorization.k8s.io | |||
} | |||
|
|||
func Example_withRoleAndRules() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like this testing pattern
/close I will close this one. I will write a separate test that checks the webhook configuration. |
@aliok: Closed this PR. In response to this:
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/test-infra repository. |
Part of #4921
Proposed Changes
Pre-review Checklist
Release Note
Docs