-
Notifications
You must be signed in to change notification settings - Fork 652
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
feat(cdevents-notification): Implementing produce CDEvents using Notification #1295
Conversation
Signed-off-by: Jalander Ramagiri <[email protected]>
*/ | ||
public HttpURLConnection sendCDEvent(CloudEvent ceToSend, URL url) throws IOException { | ||
HttpURLConnection httpUrlConnection = createConnection(url); | ||
MessageWriter messageWriter = createMessageWriter(httpUrlConnection); |
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.
A concern here is this is REALLY raw java layer for HTTP communications. The standard in spinnaker is to use retrofit & ok-http client libs. These automatically add things LIKE timeouts, URL validation, etc. that are pretty key settings for most environments. I've not looked at the CloudEvents MessageWriter system... so may HAVE to do this, but definitely have concerns.
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.
+1 on this. Let's use an actual HTTP client, please.
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 am trying to use Retrofit client to send CDEvent but facing difficulties in deserializing as Retrofit doesn't directly use CloudEventHttpMessageConverter
is the issue.
Can I use spring RestTemplate to send the CDEvent instead, please share your ideas?
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.
With retrofit and a return type of Response (e.g. here), I believe you'll get flexibility on converting the response.
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 could able to solve this issue by creating a custom CDEventsHTTPMessageConverter as retrofit doesn't directly support CloudEventHttpMessageConverter, this converter uses Jackson converter to map the CloudEvent as String and send this String with the Requestbody
...otifications/src/main/groovy/com/netflix/spinnaker/echo/cdevents/CDEventsBuilderService.java
Outdated
Show resolved
Hide resolved
|
||
} catch (Exception e) { | ||
log.error("Exception occurred while sending CDEvent {}", e); | ||
throw new CDEventsException("Exception occurred while sending CDEvent", e); |
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.
As a rule, we should be throwing spinnaker exceptions not custom exceptions for these.
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.
One thing to call out here, is unless you are modifying the retrofit exception handler, these custom exceptions may not be handled properly (I dont think). One thing we could consider doing is having some base spinnaker exception that custom exceptions could inherit from which could make throwing custom exceptions a little more flexible
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.
CDEventsException
is not needed now as using the retrofit request/response to send CDEvents notification
String status, | ||
String spinnakerUrl) { | ||
|
||
String configType = Optional.ofNullable(config).map(c -> (String) c.get("type")).orElse(null); |
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.
is null
valid? We probably want to throw an exception here?
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.
yeah, throwing an existing FieldNotFoundException here
String spinnakerUrl) { | ||
|
||
String configType = Optional.ofNullable(config).map(c -> (String) c.get("type")).orElse(null); | ||
String configLink = Optional.ofNullable(config).map(c -> (String) c.get("link")).orElse(null); |
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 is odd too, considering both these fields could not be present, but we would still try to build an executionUrl
from it. I think we should probably throw an exception, unless we have proper defaults
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.
yeah, throwing an existing FieldNotFoundException here
String status) { | ||
CloudEvent ceToSend = null; | ||
switch (cdEventsType) { | ||
case "dev.cdevents.pipelinerun.queued": |
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.
Aren't these defined in the CDEvents SDK? I'd rather pull the enums from there
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.
Yes I can pull that, there are Enum constants created with event types in the CDEvents SDK,
PipelineRunFinishedEvent("dev.cdevents.pipelinerun.finished."),
PipelineRunQueuedEvent("dev.cdevents.pipelinerun.queued."),
May be I need to compare with substring of a cdEventsType
to use with Enum constants
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.
Yes, please do that :)
executionId, executionUrl, executionName, spinnakerUrl, status); | ||
break; | ||
default: | ||
throw new CDEventsException( |
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.
Are we only interesting int he pipelinerun
and taskrun
events?
Also, most of these have the same parameter, is there an interface that can be used here? That way we can just have a map of those interfaces. Should simplify this some and makes adding to that map easier in the future
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.
As per my comment in the PR description, I will update the same PR/create another PR for all other events once this review is closed.
Current PR has the implementation to produce [Core CDEvents](https://github.com/cdevents/spec/blob/v0.1.2/core.md) for now, other events will be implemented after this PR review and discussion.
My question here is does Spinnaker needs all types of events which CDEvents spec provides(https://cdevents.dev/docs/) or needs a subset of event types based on any Spinnaker requirements.
Like I am not sure whether Source Code Control Events are required for Spinnaker to send.
There is no such interface to create CDEvents(might be a good idea to propose this change from SDK itself as that interface/implementations can be used as common),
I can update the current switch case to a map for flexibility and extensibility purpose.
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.
There is no such interface to create CDEvents(might be a good idea to propose this change from SDK itself as that interface/implementations can be used as common)
What I was meaning though is:
public interface CDEventCreateTask {
public CloudEvent create(String executionId, String executionUrl, String executionName, String spinnakerUrl, String status);
}
public class CDEventCreateTaskRunFinished implements CDEventCreateTask {
// Code here
}
Then in this class
private final Map<CDEventTypeEnum, CDEventCreateTask> createTasks = Map.of (
CDEventTaskRunFinished, new CDEventCreateTaskRunFinished(),
);
That way the switch statement goes away and instead becomes something like
CDEventCreateTask createTask = createTasks.get(eventTypeEnum);
if (createTask == null) {
throw new SomeException("Invalid CDEvent create task: " + eventTypeEnum.toString());
}
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.
yes I'm in the same direction, was thinking aloud rather implementing in the Spinnaker is that a good idea to create these interface and classes in the CDEvents Java-SDK itself,
but I know It will restrict to create CDEvents the way Spinnaker wants with any additional changes.
Will create these classes with little modifications here.
public interface CDEventCreator {
//other type of events can have different parameters, so will remove the params
CloudEvent createCDEvent();
}
Implement the classes for each event
public class CDEventPipelineRunQueued implements CDEventCreator {
//each event can have different parameters, can go here
private String source;
private String subjectId;
private String subjectSource;
private String subjectPipelineName;
private String subjectUrl;
public CDEventPipelineRunQueued(String executionId, String executionUrl, String executionName, String spinnakerUrl) {
this.source = spinnakerUrl;
this.subjectId = executionId;
this.subjectSource = spinnakerUrl;
this.subjectPipelineName = executionName;
this.subjectUrl = executionUrl;
}
@Override
public CloudEvent createCDEvent() {
PipelineRunQueuedCDEvent cdEvent = new PipelineRunQueuedCDEvent();
//set the params
}
creating a map of different events in this class,
Map<CDEventTypeEnum, CDEventCreator> createTasks = Map.of (
PipelineRunQueuedEnum, new CDEventPipelineRunQueued(executionId, executionUrl, executionName, spinnakerUrl)
);
*/ | ||
public HttpURLConnection sendCDEvent(CloudEvent ceToSend, URL url) throws IOException { | ||
HttpURLConnection httpUrlConnection = createConnection(url); | ||
MessageWriter messageWriter = createMessageWriter(httpUrlConnection); |
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.
+1 on this. Let's use an actual HTTP client, please.
|
||
} catch (Exception e) { | ||
log.error("Exception occurred while sending CDEvent {}", e); | ||
throw new CDEventsException("Exception occurred while sending CDEvent", e); |
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.
One thing to call out here, is unless you are modifying the retrofit exception handler, these custom exceptions may not be handled properly (I dont think). One thing we could consider doing is having some base spinnaker exception that custom exceptions could inherit from which could make throwing custom exceptions a little more flexible
String executionName, | ||
String spinnakerUrl, | ||
String status) { | ||
TaskRunFinishedCDEvent cdEvent = new TaskRunFinishedCDEvent(); |
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.
Then all these classes can just implement that interface I mentioned above
|
||
public class CDEventTaskRunStarted implements CDEventCreator { | ||
|
||
private String source; |
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.
We can probably move a lot of these to some base CDEvent
class, since I imagine some of these will always be required for all CDEventCreator
s.
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.
yes, making CDEventCreator
class as an abstract and moving the common properties to this class
this.subjectUrl = subjectUrl; | ||
} | ||
|
||
public String getSource() { |
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.
Lombok can be used here
See
@Getter
@Setter
Makes the code a little more clean :)
So you'd have something like
public abstract class CDEventCreator {
@Getter private String source;
}
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.
yeah that looks good, using Lombok in other sub classes too.
|
||
import io.cloudevents.CloudEvent; | ||
|
||
public abstract class CDEventCreator { |
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 name is a little confusing. CDEventCreator
? It's not really creating a CDEvent. It's more of a BaseCDEvent
or something of that nature. Im sure there are better names than BaseCDEvent
, but I think I'd much prefer that over CDEventCreator
since no creating is happening here.
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.
renamed to BaseCDEvent
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 just had a couple comments, but overall looks good. Can we also add docs around the CDEventCreator
? Just something to outline what it is and link to CDEvents in general
String configType = | ||
Optional.ofNullable(config) | ||
.map(c -> (String) c.get("type")) | ||
.orElseThrow(FieldNotFoundException::new); | ||
String configLink = | ||
Optional.ofNullable(config) | ||
.map(c -> (String) c.get("link")) | ||
.orElseThrow(FieldNotFoundException::new); | ||
|
||
String executionId = | ||
Optional.ofNullable(event.content) | ||
.map(e -> (Map) e.get("execution")) | ||
.map(e -> (String) e.get("id")) | ||
.orElseThrow(FieldNotFoundException::new); |
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.
can we add the field name to the exceptions?
or log what is the missing field
} else { | ||
return cdEventCreator.createCDEvent(); | ||
} |
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.
else not needed
for (String keyType : cdEventsMap.keySet()) { | ||
if (keyType.contains(cdEventsType)) { | ||
cdEventCreator = cdEventsMap.get(keyType); | ||
break; | ||
} | ||
} |
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.
can we use a stream and a filter?
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.
updated to use stream and filter
|
||
private final ObjectMapper objectMapper; | ||
|
||
private static final String MIME_TYPE = "application/json"; |
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 believe there is a constant in spring for this
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.
yes, using MediaType.APPLICATION_JSON_VALUE from spring
} catch (JsonParseException e) { | ||
throw new ConversionException(e); | ||
} catch (JsonMappingException e) { | ||
throw new ConversionException(e); | ||
} catch (IOException e) { | ||
throw new ConversionException(e); | ||
} |
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.
seems like we can use catch (JsonParseException | JsonMappingException | IOException e)
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.
IOException
can not be used with multi-catch as the other Exception classes are subclass of IOException
Using multi-catch for subclasses alone,
catch (JsonParseException | JsonMappingException e)
package com.netflix.spinnaker.echo.cdevents; | ||
|
||
import retrofit.client.Response; | ||
import retrofit.http.*; |
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 believe we should avoid wildcards
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.
yeah corrected, it was auto imported,
request.addHeader("Ce-Specversion", cdEvent.getSpecVersion().V1.toString()); | ||
request.addHeader("Ce-Source", cdEvent.getSource().toString()); | ||
request.addHeader("Ce-Type", cdEvent.getType()); | ||
request.addHeader("Content-Type", "application/json"); |
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.
reuse a constant
if (subjectError.equals("complete")) { | ||
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.SUCCESS); | ||
} else if (subjectError.equals("failed")) { | ||
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.FAILURE); | ||
} |
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.
these could produce a NPE let's change them to "complete".equals(subjectError)
if (getSubjectError().equals("complete")) { | ||
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.SUCCESS); | ||
} else if (getSubjectError().equals("failed")) { | ||
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.FAILURE); | ||
} |
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.
avoid NPE using "failed".equals(getSubjectError())
return endPointURL; | ||
} catch (MalformedURLException e) { | ||
throw new InvalidRequestException( | ||
"Unable to determine base URL from Microsoft Teams webhook URL.", e); |
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.
is this only related to Microsoft Teams?
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.
Apologies for the typo, corrected with valid message
@@ -0,0 +1,85 @@ | |||
/* | |||
* Copyright 2020 Cerner Corporation |
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.
2023 Nordix
?
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.
Yes Updated
@@ -0,0 +1,66 @@ | |||
/* | |||
* Copyright 2016 Google, Inc. |
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.
2023 Nordix
?
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.
yes, updated
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.
Overall better. Frankly the APIs are a TOUCh rough on cdevents side. Example:
- EVERY event right now looks like it duplicates that same call
cdEvent.setSource(URI.create(getSource()));
uniquely. There isn't a base object that could handle this and reduce the duplication. There appear to be about 3 or 4 fields which are duplicated across ALL cdevents stuff - The URI parsing has a few places of "hey this is probably non ideal" on things like the format of spinnaker URls & such, but... for the moment it'd work. Just a personal preference on UI urls vs. API urls and encoding. Just depends on how downstream handles it, not enough to "block" the PR at this time.
@jasonmcintosh The SDKs are very low level currently for CDEvents. So no abstractions have been added to make things much easier. Generally when you generating from openapi, smithy, and any other API language, they may not generate base classes especially for languages like golang. It's completely up to the generator. However, what most people do is use that to generate clients but are not meant to be used as the whole SDK. You see this in AWS SDKs where the clients are actually very low level, but there are higher level abstractions that make it easier. Due to CDEvents being still early in its life, these abstractions have no been made yet, unfortunately. This should get better with time though :). |
Exactly :) Let's get this merged, and see how it gets better! |
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.
LGTM! 🚢
Implementing Produce CDEvents as per the RFC design
https://github.com/spinnaker/governance/blob/master/rfc/cdevents-spinnaker.md#produce-cdevents-1
This PR has the changes to implement a new CDEventsNotificationAgent, to handle the CDEvent type of Notifications and send them to
address
configured as events-broker URL using CDEvents-Java-SDKNote:
Need some clarifications on type of CDEvents that Spinnaker needs to send,
Current PR has the implementation to produce Core CDEvents for now, other events will be implemented after this PR review and discussion.
Dependent Spinnaker/deck PR - spinnaker/deck#9997