EventType autocreate #3585

Cali0707 · 2024-01-10T18:09:54Z

Proposed Changes

Create an EventTypeCreator class to handle the creation of eventtypes
Call the create method in the receiver ingress on successfully receiving an event

These changes work as follows:

kn broker create my-broker --class Kafka
Broker 'my-broker' successfully created in namespace 'default'.

 kn-event-linux-amd64 send --to Broker:eventing.knative.dev/v1:my-broker --type type.eventing.example.1
Jan 10 12:35:42.087   INFO Event (ID: e0a551a7-7f1e-482e-aaa0-10fc5c076326) have been sent.

kubectl get eventtypes.eventing.knative.dev -A
No resources found

kubectl apply -f enable-autocreate.yaml                           
configmap/config-features configured

kn-event-linux-amd64 send --to Broker:eventing.knative.dev/v1:my-broker --type type.eventing.example.1
Jan 10 12:52:07.555   INFO Event (ID: a8c83ebf-d8a6-4fb0-9fb1-c2d67f8a21e9) have been sent.

kubectl get eventtypes.eventing.knative.dev -A                                                        
NAMESPACE   NAME                                            TYPE                      SOURCE             SCHEMA   REFERENCE NAME   REFERENCE KIND   DESCRIPTION                             READY   REASON
default     et-my-broker-cc78b6ed9ee7697817cf61de134725aa   type.eventing.example.1   kn-event/v1.10.0            my-broker        Broker           Event Type auto-created by controller   True

Release Note

:gift: EventType Autocreate now works for the Eventing Kafka Broker and Eventing Kafka Channel

Docs

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-01-10T18:10:07Z

/cc @Leo6Leo @pierDipi @matzew

codecov · 2024-01-10T18:14:04Z

Codecov Report

Attention: Patch coverage is 56.42857% with 61 lines in your changes are missing coverage. Please review.

Project coverage is 62.10%. Comparing base (049d0f0) to head (49c3372).
Report is 2 commits behind head on main.

Files	Patch %	Lines
...tive/eventing/kafka/broker/receiver/main/Main.java	0.00%	23 Missing ⚠️
...ing/kafka/broker/core/eventtype/EventTypeSpec.java	35.71%	18 Missing ⚠️
...ceiver/impl/handler/IngressRequestHandlerImpl.java	38.46%	7 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/broker/controller.go	42.85%	2 Missing and 2 partials ⚠️
control-plane/pkg/reconciler/sink/controller.go	42.85%	2 Missing and 2 partials ⚠️
...ka/broker/core/eventtype/EventTypeCreatorImpl.java	89.47%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3585      +/-   ##
============================================
- Coverage     62.16%   62.10%   -0.06%     
- Complexity      859      874      +15     
============================================
  Files           189      190       +1     
  Lines         12937    13037     +100     
  Branches        276      284       +8     
============================================
+ Hits           8042     8097      +55     
- Misses         4265     4303      +38     
- Partials        630      637       +7

Flag	Coverage Δ
java-unittests	`74.13% <57.93%> (-0.39%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Cali0707 · 2024-01-10T18:54:01Z

/test upgrade-tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

Leo6Leo · 2024-01-10T20:36:20Z

@Cali0707 Tested in my local environment, the event type works perfectly! Great work!!!

pierDipi

Are you also planning to add E2E tests ?

pierDipi · 2024-01-15T16:34:49Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+        this.messageDigest.reset();
+        this.messageDigest.update(suffixString.getBytes());


what happens when this is called concurrently ? I guess this is not thread safe ?

Yeah I doubt this is thread safe, but currently it is just used in the IngressRequestHandlerImpl which I thought was 1 per verticle (so no concurrency worries?). Should I change this to be thread safe?

Yes, I would opt to being safer here

pierDipi · 2024-01-15T16:35:11Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+        final var suffix = Hex.encodeHexString(this.messageDigest.digest());
+        final var name = String.format("et-%s-%s", reference.getName(), suffix).toLowerCase();
+        if (name.length() > DNS1123_SUBDOMAIN_MAX_LENGTH) {
+            return name.substring(0, DNS1123_SUBDOMAIN_MAX_LENGTH);
+        }
+        return name;


Is this the same algorithm we use in Eventing core ?

I think so, I tried to copy from https://github.com/knative/eventing/blob/42af160c3cfc244e26fa7bcd5db2c670ebdcf326/pkg/eventtype/eventtypes.go#L48-L52

pierDipi · 2024-01-15T16:35:55Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+    private boolean eventTypeExists(String etName, DataPlaneContract.Reference reference) {
+        var et = this.eventTypeClient
+                .inNamespace(reference.getNamespace())
+                .withName(etName)
+                .get();
+        return et != null;
+    }


Are we using an actual client ? should we use a lister here ?

This is an actual client as I wasn't sure how to make a lister to our custom class - I'll try and update it to be a lister

pierDipi · 2024-01-15T16:37:14Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+                .withSchema(event.getDataSchema())
+                .withDescription("Event Type auto-created by controller");
+
+        this.eventTypeClient.resource(et.build()).create();


we should avoid failing when the resource exist (like when it is created by some other process between checking exists and this line)

Hm yeah, as far as I can tell this method doesn't declare that it returns an error or throws an exception. Should I wrap this in a try catch just in case?

We need to be sure on what happens when already exists and handle that case accordingly

I looked on their github and they said that the methods generally throw exceptions, so I'll add a try-catch to handle that case

Cali0707 · 2024-01-15T16:49:47Z

For E2E tests, I was planning on porting those from eventing core: https://github.com/knative/eventing/blob/42af160c3cfc244e26fa7bcd5db2c670ebdcf326/test/experimental/eventtype_autocreate_test.go#L32-L74

But, I'm not sure if we have a job for experimental tests so I think I need to set that up first

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-01-15T19:18:18Z

/test

knative-prow · 2024-01-15T19:18:21Z

@Cali0707: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test build-tests
/test channel-integration-tests-sasl-plain
/test channel-integration-tests-sasl-ssl
/test channel-integration-tests-ssl
/test channel-reconciler-tests-sasl-plain
/test channel-reconciler-tests-sasl-ssl
/test channel-reconciler-tests-ssl
/test integration-tests
/test reconciler-tests
/test reconciler-tests-keda
/test reconciler-tests-namespaced-broker
/test unit-tests
/test upgrade-tests

The following commands are available to trigger optional jobs:

/test reconciler-tests-loom
/test reconciler-tests-namespaced-broker-loom

Use /test all to run the following jobs that were automatically triggered:

build-tests_eventing-kafka-broker_main
channel-integration-tests-sasl-plain_eventing-kafka-broker_main
channel-integration-tests-sasl-ssl_eventing-kafka-broker_main
channel-integration-tests-ssl_eventing-kafka-broker_main
channel-reconciler-tests-sasl-plain_eventing-kafka-broker_main
channel-reconciler-tests-sasl-ssl_eventing-kafka-broker_main
channel-reconciler-tests-ssl_eventing-kafka-broker_main
integration-tests_eventing-kafka-broker_main
reconciler-tests-keda_eventing-kafka-broker_main
reconciler-tests-namespaced-broker_eventing-kafka-broker_main
reconciler-tests_eventing-kafka-broker_main
unit-tests_eventing-kafka-broker_main
upgrade-tests_eventing-kafka-broker_main

In response to this:

/test

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.

Signed-off-by: Calum Murray <cmurray@redhat.com>

pierDipi · 2024-02-20T13:33:05Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+        try {
+            this.messageDigest = MessageDigest.getInstance("MD5");
+        } catch (NoSuchAlgorithmException ignored) {
+            this.messageDigest = null;
+        }


Catching and leaving it to null will just delay the exception to the runtime, can we just not catch it or if it's unchecked exception, we could wrap it into a runtime exception?

Currently it doesn't throw a runtime exception, the autocreate just becomes a no-op (no eventtype gets created). Do you think it's better to fail here instead?

pierDipi · 2024-02-20T13:37:58Z

...ore/src/main/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImpl.java

+        if (this.eventTypeLister == null) {
+            logger.debug("no eventtype lister, creating eventtype and might run into failure");
+            return null;
+        }


Should we fail fast here in the constructor?

pierDipi · 2024-02-20T13:41:17Z

...src/test/java/dev/knative/eventing/kafka/broker/core/eventtype/EventTypeCreatorImplTest.java

+                    return eventTypeCreator
+                            .create(event, reference)
+                            .onFailure((exception) -> {
+                                logger.warn("failure occured, closing informer", exception);


"occured" is a misspelling of "occurred"

Good catch, thanks!

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-02-20T14:42:36Z

/cc @pierDipi

pierDipi

/lgtm
/approve

Signed-off-by: Calum Murray <cmurray@redhat.com>

Leo6Leo · 2024-02-21T19:33:18Z

/lgtm

pierDipi

/lgtm
/approve

knative-prow · 2024-02-26T15:58:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, matzew, pierDipi

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:

~~OWNERS~~ [Cali0707,matzew,pierDipi]

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

pierDipi · 2024-02-26T15:58:52Z

/test reconciler-tests-namespaced-broker

Cali0707 · 2024-02-26T18:21:57Z

/test reconciler-tests-namespaced-broker

Cali0707 · 2024-02-26T20:50:49Z

/retest-required

pierDipi · 2024-02-27T06:46:56Z

/test reconciler-tests

Cali0707 added 8 commits January 9, 2024 14:27

updated EventType fabricio classes

0167ad4

Signed-off-by: Calum Murray <cmurray@redhat.com>

Created EventTypeCreator class

f17d817

Signed-off-by: Calum Murray <cmurray@redhat.com>

added rbac for eventtypes to dataplane clusterroles

0a974f8

Signed-off-by: Calum Murray <cmurray@redhat.com>

fix clusterroles for data plane

a07e07a

Signed-off-by: Calum Murray <cmurray@redhat.com>

refactored EventTypeCreator class

3e898f0

Signed-off-by: Calum Murray <cmurray@redhat.com>

use the EventTypeCreator in receiver

3ba269c

Signed-off-by: Calum Murray <cmurray@redhat.com>

reconcile brokers when config features are changed

a1db185

Signed-off-by: Calum Murray <cmurray@redhat.com>

cleanup of a few things

6eab9af

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested review from creydr and Leo6Leo January 10, 2024 18:10

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane labels Jan 10, 2024

knative-prow bot requested review from matzew and pierDipi January 10, 2024 18:10

Cali0707 changed the title ~~Et autocreate~~ EventType autocreate Jan 10, 2024

Cali0707 linked an issue Jan 10, 2024 that may be closed by this pull request

Implement the auto-create feature flag for the Apache Kafka backed Knative Channel #3181

Closed

fix controller clusterrole to fix namespaced broker permission error

54388de

Signed-off-by: Calum Murray <cmurray@redhat.com>

pierDipi reviewed Jan 15, 2024

View reviewed changes

refactor to use one eventtypecreator per receiver verticle

f39d2cb

Signed-off-by: Calum Murray <cmurray@redhat.com>

fix formatting and permissions for eventtype lister

f4dc347

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2024

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2024

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2024

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2024

fixed merge conflicts

7168c9c

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 9, 2024

fix merge conflicts again

28f0077

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024

pierDipi reviewed Feb 20, 2024

View reviewed changes

fix: fail fast in constructor

6c9d12d

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from pierDipi February 20, 2024 14:42

pierDipi approved these changes Feb 21, 2024

View reviewed changes

knative-prow bot assigned pierDipi Feb 21, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024

Cali0707 added 2 commits February 21, 2024 14:27

Merge branch 'main' into et-autocreate

749d7c0

./hack/update-codegen.sh

49c3372

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024

pierDipi approved these changes Feb 26, 2024

View reviewed changes

knative-prow bot merged commit f85af06 into knative-extensions:main Feb 27, 2024
36 of 37 checks passed

creydr mentioned this pull request Mar 18, 2024

Enable eventtype-auto-create feature flag openshift-knative/eventing-kafka-broker#1028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventType autocreate #3585

EventType autocreate #3585

Cali0707 commented Jan 10, 2024

Cali0707 commented Jan 10, 2024

codecov bot commented Jan 10, 2024 •

edited

Loading

Cali0707 commented Jan 10, 2024

Leo6Leo commented Jan 10, 2024

pierDipi left a comment

pierDipi Jan 15, 2024

Cali0707 Jan 15, 2024 •

edited

Loading

pierDipi Feb 20, 2024

pierDipi Jan 15, 2024

Cali0707 Jan 15, 2024

pierDipi Jan 15, 2024

Cali0707 Jan 15, 2024

pierDipi Jan 15, 2024

Cali0707 Jan 15, 2024

pierDipi Jan 15, 2024

Cali0707 Jan 15, 2024

Cali0707 commented Jan 15, 2024

Cali0707 commented Jan 15, 2024

knative-prow bot commented Jan 15, 2024

pierDipi Feb 20, 2024 •

edited

Loading

Cali0707 Feb 20, 2024

pierDipi Feb 20, 2024

pierDipi Feb 20, 2024

Cali0707 Feb 20, 2024

Cali0707 commented Feb 20, 2024

pierDipi left a comment

Leo6Leo commented Feb 21, 2024

pierDipi left a comment

knative-prow bot commented Feb 26, 2024

pierDipi commented Feb 26, 2024

Cali0707 commented Feb 26, 2024

Cali0707 commented Feb 26, 2024

pierDipi commented Feb 27, 2024

		this.messageDigest.reset();
		this.messageDigest.update(suffixString.getBytes());

EventType autocreate #3585

EventType autocreate #3585

Conversation

Cali0707 commented Jan 10, 2024

Proposed Changes

Cali0707 commented Jan 10, 2024

codecov bot commented Jan 10, 2024 • edited Loading

Codecov Report

Cali0707 commented Jan 10, 2024

Leo6Leo commented Jan 10, 2024

pierDipi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented Jan 15, 2024

Cali0707 commented Jan 15, 2024

knative-prow bot commented Jan 15, 2024

pierDipi Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented Feb 20, 2024

pierDipi left a comment

Choose a reason for hiding this comment

Leo6Leo commented Feb 21, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Feb 26, 2024

pierDipi commented Feb 26, 2024

Cali0707 commented Feb 26, 2024

Cali0707 commented Feb 26, 2024

pierDipi commented Feb 27, 2024

codecov bot commented Jan 10, 2024 •

edited

Loading

Cali0707 Jan 15, 2024 •

edited

Loading

pierDipi Feb 20, 2024 •

edited

Loading