Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support Knative broker/trigger model #153

Merged
merged 1 commit into from
Oct 14, 2019
Merged

Support Knative broker/trigger model #153

merged 1 commit into from
Oct 14, 2019

Conversation

lburgazzoli
Copy link
Contributor

Fixes #150
Fixes #151

@lburgazzoli
Copy link
Contributor Author

@nicolaferraro rewrote it, it should now support all the cases you mentioned.

davsclaus
davsclaus requested changes Oct 13, 2019
View reviewed changes
...-knative-http/src/main/java/org/apache/camel/component/knative/http/KnativeHttpProducer.java Outdated Show resolved Hide resolved
...knative-http/src/main/java/org/apache/camel/component/knative/http/KnativeHttpTransport.java Show resolved Hide resolved
...knative/camel-knative/src/main/java/org/apache/camel/component/knative/KnativeComponent.java Outdated Show resolved Hide resolved
...knative/camel-knative/src/main/java/org/apache/camel/component/knative/KnativeComponent.java Outdated Show resolved Hide resolved
...ive/camel-knative/src/main/java/org/apache/camel/component/knative/KnativeConfiguration.java Show resolved Hide resolved
...-knative/camel-knative/src/main/java/org/apache/camel/component/knative/KnativeEndpoint.java Outdated Show resolved Hide resolved
@lburgazzoli
Copy link
Contributor Author

@davsclaus fixed

nicolaferraro
nicolaferraro requested changes Oct 14, 2019
View reviewed changes
Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good. Just a naming issue to fix..

...-knative/camel-knative-api/src/main/java/org/apache/camel/component/knative/spi/Knative.java
}

public enum Protocol {
http,
https
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A specific reason for removing this? Afaik only http is used now, but why not just leaving it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we do not have a real support for https now, i.t. it lacks all the options to set up the ssl context so removed to avoid confusion

...-knative/camel-knative-api/src/main/java/org/apache/camel/component/knative/spi/Knative.java Outdated
@@ -40,13 +37,18 @@
private Knative() {
}

public enum Kind {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on the operator to make a version that can work with broker/trigger model. The problem is that the pair (name, source/sink) is not sufficient to identify a resource in all cases. There are some edge cases we have to live with, now that Channels have been moved into CRDs. E.g. one many define a KafkaChannel named "mychan" and a InMemoryChannel named "mychan" as well and we should allow to target both.

Being this an edge case, we can open a issue to solve it later in runtime, but I'm adding support for it in the operator, so I've added not 1 but 3 fields in the CamelServiceDefinition:

  • apiVersion
  • kind
  • this field that you called kind but better we call kind the Kubernetes Kind. I've called it consumption, with possible values consumer or producer, but it's probably a name worse than kind

Just.. let's find a better name for this, I'm fine with the enum values proposed here.

Copy link
Contributor Author

@lburgazzoli lburgazzoli Oct 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use some "namespace" to distinguish things like:

  • camel.endpoint.kind for source vs sink
  • knative.kind and knative.apiVersion for additional kantive metadata

So a service is always identified by is name then some additional filtering logic can be applied on the endpoint side depending on the options provided

...mel-knative-api/src/main/java/org/apache/camel/component/knative/spi/KnativeEnvironment.java Outdated
.filter(Optional::isPresent)
.map(Optional::get)
.findFirst();
}

public KnativeServiceDefinition mandatoryLookupService(Knative.Type type, String name) {
return lookupService(type, name).orElseThrow(
public KnativeServiceDefinition mandatoryLookupService(Knative.Kind endpointType, Knative.Type type, String name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case you want to add support for 4 fields lookup in this PR, apiVersion and (Kubernetes) kind are optional, so you should filter them only when explicitly set on the endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have cases in which we only have the kind set and not the apiVersion ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's possible that one uses kind=Channel and it can mean messaging.knative.dev/v1alpha1 or eventing.knative.dev/v1alpha1 (the operator will resolve the right one based on what is present in the cluster). Or in the future there can be multiple possible API versions of the same group and a Camel user will just declare the kind.

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Oct 14, 2019 •
edited
Loading

@nicolaferraro reworked and now you can specify camel endpoint kind and kubernetes kind + version as metadata

something like

{
  "type": "channel|endpoint|event”,
  "name": "",
  "host": "",
  "port": "",
  "metadata": {
      "filter.header": "value",
            
      "knative.kind": "",
      "knative.apiVersion": "",

      "camel.endpoint.kind": "source|sink",
  }
}

@nicolaferraro
Copy link
Member

I'll have a look soon, thanks

@nicolaferraro
Copy link
Member

nicolaferraro commented Oct 14, 2019 •
edited
Loading

Can type also be "event"? @lburgazzoli

[replied] Yes, it can..

nicolaferraro
nicolaferraro approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@nicolaferraro nicolaferraro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, let's merge it and open new issues after some integration rounds..

@nicolaferraro nicolaferraro merged commit a0b31d2 into apache:master Oct 14, 2019
@lburgazzoli lburgazzoli deleted the github-151 branch October 14, 2019 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolaferraro nicolaferraro nicolaferraro approved these changes

@davsclaus davsclaus davsclaus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Knative broker/trigger model Support explicit source/target in knative endpoint
3 participants
@lburgazzoli @nicolaferraro @davsclaus