Dataplane conformance tests

[vishal-chdhry]

Part of #6796
Part of #5782

Proposed Changes

• Fixes unimplimented tests in dataplane

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

Fixed Dataplane conformance  unimplemented test cases.

Docs

[knative-prow]

Hi @vishal-chdhry. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[vishal-chdhry]

@creydr @pierDipi
Isn't

eventing/test/rekt/features/broker/data_plane.go

Lines 60 to 63 in 1ff36e1

	Should("It SHOULD support Binary Content Mode of the HTTP Protocol Binding for CloudEvents.",
	brokerAcceptsBinaryContentMode).
	Should("It SHOULD support Structured Content Mode of the HTTP Protocol Binding for CloudEvents.",
	brokerAcceptsStructuredContentMode).

and

eventing/test/rekt/features/broker/data_plane.go

Lines 112 to 113 in 1ff36e1

	Should("The Broker SHOULD support delivering events via Binary Content Mode or Structured Content Mode of the HTTP Protocol Binding for CloudEvents.",
	todo).

the same?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.96%. Comparing base (034bec9) to head (29d1058).
Report is 425 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6811   +/-   ##
=======================================
  Coverage   79.96%   79.96%           
=======================================
  Files         237      237           
  Lines       12354    12354           
=======================================
  Hits         9879     9879           
  Misses       1981     1981           
  Partials      494      494           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vishal-chdhry]

Hi @pierDipi, can you please explain these to me? What does Events contained in delivery responses mean, and how does reply event work

eventing/test/rekt/features/broker/data_plane.go

Lines 134 to 141 in 1ff36e1

	Should("Events contained in delivery responses SHOULD be published to the Broker ingress and processed as if the event had been produced to the Broker's addressable endpoint.",
	todo).
	Should("Events contained in delivery responses that are malformed SHOULD be treated as if the event delivery had failed.",
	todo).
	May("The subscriber MAY receive a confirmation that a reply event was accepted by the Broker.",
	todo).
	Should("If the reply event was not accepted, the initial event SHOULD be redelivered to the subscriber.",
	todo)

And for the observability tests

eventing/test/rekt/features/broker/data_plane.go

Lines 154 to 169 in 1ff36e1

	f.Stable("Conformance").
	Should("The Broker SHOULD expose a variety of metrics*.",
	todo).
	// * including, but not limited to:
	//- Number of malformed produce requests (400-level responses)
	//- Number of accepted produce requests (200-level responses)
	//- Number of events delivered
	Should("Metrics SHOULD be enabled by default, with a configuration parameter included to disable them if desired.",
	todo).
	Should("Upon receiving an event with context attributes defined in the CloudEvents Distributed Tracing extension the Broker SHOULD preserve that trace header on delivery to subscribers and on reply events, unless the reply is sent with a different set of tracing attributes.",
	todo).
	Should("Forwarded trace headers SHOULD be updated with any intermediate spans emitted by the broker.",
	todo).
	Should("Spans emitted by the Broker SHOULD follow the OpenTelemetry Semantic Conventions for Messaging System*.",
	todo)

I have to get the number of events delivered using the brokerName, can you point me to where i can find the metrics functions?

[pierDipi]

Yes, correct

[pierDipi]

What does Events contained in delivery responses mean, and how does reply event work

This is explained shortly here: https://knative.dev/docs/eventing/ with:

Sinks can also be configured to respond to HTTP requests by sending a response event

For the Trigger's subscriber the "reply event" is the HTTP response that contains a new event and it is sent back by the Trigger to the associated Broker.
We need to make sure that another trigger will receive the "reply event".

Now, provided what is the reply event

 Should("Events contained in delivery responses that are malformed SHOULD be treated as if the event delivery had failed.", 
 	todo). 
 May("The subscriber MAY receive a confirmation that a reply event was accepted by the Broker.", 
 	todo). 
 Should("If the reply event was not accepted, the initial event SHOULD be redelivered to the subscriber.", 
 	todo) 

These 3 tests need to test all the cases, in particular:

1. Events contained in delivery responses that are malformed SHOULD be treated as if the event delivery had failed.

this is the case where the reply event is malformed/not an actual cloudevent, in that case, we expect that the original event (the one in request, so not the reply event) will be resent to the Trigger's subscriber ( see comment on case 3).

2. The subscriber MAY receive a confirmation that a reply event was accepted by the Broker.

I would leave this as todo for now, since we don't have defined a way of doing this.

3. If the reply event was not accepted, the initial event SHOULD be redelivered to the subscriber.

This is basically case 1 + making sure that the delivery is retried when 1 happens, so we can have a single test for 3 and test case 1 as well (therefore we can remove case 1)

[pierDipi]

Thanks for doing this! Let me know if you need further information!

[pierDipi]

For observability tests I would recommend doing a separate PR and connect with @mgencur (on Slack or on the original GitHub issue) since Martin worked on some of those implementation specific tests in the past.

 f.Stable("Conformance"). 
 	Should("The Broker SHOULD expose a variety of metrics*.", 
 		todo). 
 	// * including, but not limited to: 
 	//- Number of malformed produce requests (400-level responses) 
 	//- Number of accepted produce requests (200-level responses) 
 	//- Number of events delivered 
  
 	Should("Metrics SHOULD be enabled by default, with a configuration parameter included to disable them if desired.", 
 		todo). 
 	Should("Upon receiving an event with context attributes defined in the CloudEvents Distributed Tracing extension the Broker SHOULD preserve that trace header on delivery to subscribers and on reply events, unless the reply is sent with a different set of tracing attributes.", 
 		todo). 
 	Should("Forwarded trace headers SHOULD be updated with any intermediate spans emitted by the broker.", 
 		todo). 
 	Should("Spans emitted by the Broker SHOULD follow the OpenTelemetry Semantic Conventions for Messaging System*.", 
 		todo) 

We can only tests tracing related ones, in particular, we need to verify that:

Upon receiving an event with context attributes defined in the CloudEvents Distributed Tracing extension the Broker SHOULD preserve that trace header on delivery to subscribers and on reply events, unless the reply is sent with a different set of tracing attributes.

For this case, we need to verify that the Trigger's subscriber receives a traceparent header when the source/sender is configured to start a tracing context (eventshub as source should do that by default).

Forwarded trace headers SHOULD be updated with any intermediate spans emitted by the broker.

This should verify that the tracing backend contains the additional tracing spans in addition to the source and the subscriber ones

Spans emitted by the Broker SHOULD follow the OpenTelemetry Semantic Conventions for Messaging System*.

This should verify that there is at least one tracing span in the tracing span tree that contains the tags matching the Semantic Conventions for Messaging Systems

[pierDipi]

/ok-to-test

[pierDipi]

/assign @creydr

[creydr]

Hello @vishal-chdhry,
Thanks a lot for working on this 👍 . This is really great to have some more conformance tests implemented.
I left a few comments on it so far. I hope they help

[creydr]

This returns a StepFn only. Maybe you could check on AcceptsCEVersions how it is done there.

As deliveredCESpecs itself is a StepFn, maybe you could do something like this too:

Suggested change

	assert.OnStore(sink).MatchEvent(test.HasId(event.ID()), test.HasSpecVersion(version)).Exact(1)
	assert.OnStore(sink).MatchEvent(test.HasId(event.ID()), test.HasSpecVersion(version)).Exact(1)(ctx, t)

[vishal-chdhry]

is it better to do

assert.OnStore(sink).MatchEvent(test.HasId(event.ID()), test.HasSpecVersion(version)).Exact(1)(ctx, t)

or

store := eventshub.StoreFromContext(ctx, source)
events := knconf.Correlate(store.AssertAtLeast(t, 2, knconf.SentEventMatcher("")))
for _, e := range events {
	if e.Response.StatusCode < 200 || e.Response.StatusCode > 299 {
		t.Errorf("Expected statuscode 2XX for sequence %d got %d", e.Response.Sequence, e.Response.StatusCode)
	}
}

[vishal-chdhry]

@creydr For now, I have done what was suggested in this comment at every place because I observed that we are using event assert in recent commits and eventshub.StoreFromContext in older ones.

I can definitely use the latter if i should

[creydr]

Hello @vishal-chdhry,
thanks for the update. Can you check, why the reconciler-tests are failing (maybe only a rebase needed), so we can check if these tests pass?

[creydr]

If no ready Trigger would match an accepted event, the Broker MAY drop that event without notifying the producer.
@pierDipi @matzew do you have/know a handy way, how to check if the producer was notified or not?

[creydr]

Why is this test removed?

[vishal-chdhry]

We can add that case to If the reply event was not accepted, the initial event SHOULD be redelivered to the subscriber.
Here is a great explainer from PierDipi #6811 (comment)

[creydr]

Since trigger2 does not have any filters, wouldn't sink2 receive the original event too?

[knative-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

• test/OWNERS

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

[vishal-chdhry]

/test reconciler-tests

[knative-prow]

@vishal-chdhry: 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
reconciler-tests_eventing_main	29d1058	link	true	/test reconciler-tests

Your PR dashboard.

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. I understand the commands that are listed here.

[vishal-chdhry]

@creydr In some places, it is failing because of the error, pod is not running

wait.go:413: Pod test-inkspplu/source-ttdrjpyd is not running...
wait.go:413: Pod test-inkspplu/source-ffsjcstg is not running...
wait.go:413: Pod test-inkspplu/source-gizoauac is not running...

I need to check isReady() in setup phase, but feature.feature is not available in the tests (feature.T) is, how do I specify setup phases?

[creydr]

@creydr In some places, it is failing because of the error, pod is not running

wait.go:413: Pod test-inkspplu/source-ttdrjpyd is not running...
wait.go:413: Pod test-inkspplu/source-ffsjcstg is not running...
wait.go:413: Pod test-inkspplu/source-gizoauac is not running...

I need to check isReady() in setup phase, but feature.feature is not available in the tests (feature.T) is, how do I specify setup phases?

That's a good question, as we have a bit different setup for the conformance tests 🤔
@matzew @pierDipi are you aware of a way, how to add a step e.g. to the Setup phase from inside of a data plane conformance tests. Maybe I am simply not seeing it ATM

[pierDipi]

@creydr In some places, it is failing because of the error, pod is not running

wait.go:413: Pod test-inkspplu/source-ttdrjpyd is not running...
wait.go:413: Pod test-inkspplu/source-ffsjcstg is not running...
wait.go:413: Pod test-inkspplu/source-gizoauac is not running...

I need to check isReady() in setup phase, but feature.feature is not available in the tests (feature.T) is, how do I specify setup phases?

That's a good question, as we have a bit different setup for the conformance tests thinking @matzew @pierDipi are you aware of a way, how to add a step e.g. to the Setup phase from inside of a data plane conformance tests. Maybe I am simply not seeing it ATM

hmm, no, there isn't, I was thinking to new rekt APIs for basically branching a feature to a "sub-feature" but I'm not too convinced, the other approach is to have separate tests instead of being "assertions / steps"

[creydr]

Hey @vishal-chdhry,
could you recheck on this? What do you think about having separate tests instead so you have feature.T available? Or any other ideas?

[vishal-chdhry]

I will just convert all these tests to being seperate tests, that seems like the more straight forward approach

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

[creydr]

/remove-lifecycle stale
Hi @vishal-chdhry,
any plans to recheck on this, or do you want to close this PR?

[vishal-chdhry]

Oh I totally forgot about this 😅

I will try to fix this, thanks for the ping @creydr!

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

[sadath-12]

@vishal-chdhry do let me know if your continuing this or else I'm happy to continue from here

[github-actions]

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.