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.

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

Merged
merged 29 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
483a461
feat(cdevents-notification): Implementing produce CDEvents using Noti…
rjalander May 15, 2023
3b759ee
feat(cdevents-notification): unit tests to produce CDEvents using Not…
rjalander May 16, 2023
576e7d1
feat(cdevents-notification): update variable name to cdEventsType
rjalander May 16, 2023
b943650
Merge branch 'master' into notification_cdevents
rjalander May 22, 2023
68ff04e
Merge branch 'master' into notification_cdevents
rjalander May 26, 2023
174e51b
Merge branch 'master' into notification_cdevents
rjalander Jun 16, 2023
25e6b7a
Merge branch 'master' into notification_cdevents
rjalander Jul 13, 2023
09237b9
Merge branch 'master' into notification_cdevents
rjalander Jul 24, 2023
de76a8c
Updating cdevents-sdk-java dependency version
rjalander Jul 26, 2023
02a6610
fix: unit test failure with sdk
rjalander Jul 26, 2023
f687deb
Merge branch 'master' into notification_cdevents
rjalander Aug 9, 2023
34c40c9
Merge branch 'master' into notification_cdevents
rjalander Aug 11, 2023
6ff8064
Merge branch 'master' into notification_cdevents
rjalander Aug 14, 2023
71f5c8f
Merge branch 'master' into notification_cdevents
rjalander Aug 28, 2023
e7cf714
Merge branch 'master' into notification_cdevents
rjalander Sep 29, 2023
cb9e3a7
Merge branch 'master' into notification_cdevents
rjalander Oct 12, 2023
6470546
using retrofit to send cdevent
rjalander Oct 12, 2023
ac7262a
Merge branch 'notification_cdevents' of github.com:Nordix/echo into n…
rjalander Oct 12, 2023
65e8b33
fixing format violations
rjalander Oct 12, 2023
f573844
addressing review comments
rjalander Oct 13, 2023
a24cabd
Interface to create CDEvents
rjalander Oct 16, 2023
f9fa051
update Copyright information
rjalander Oct 16, 2023
2eecfe0
Making CDEventCreator as an abstract
rjalander Oct 18, 2023
af917bb
Merge branch 'master' into notification_cdevents
rjalander Oct 18, 2023
7b832ee
Merge branch 'master' into notification_cdevents
rjalander Oct 23, 2023
fc31963
Class name changed to BaseCDEvent and uing lombok.Getter
rjalander Oct 24, 2023
0ebb3ce
Merge branch 'notification_cdevents' of github.com:Nordix/echo into n…
rjalander Oct 24, 2023
1f783f6
addressing review comments
rjalander Oct 24, 2023
621da6b
addressing minor review comments
rjalander Oct 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions echo-notifications/echo-notifications.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ dependencies {
implementation "org.jsoup:jsoup:1.8.3"
implementation "com.atlassian.commonmark:commonmark:0.9.0"
implementation "org.codehaus.groovy:groovy-json"
implementation "io.cloudevents:cloudevents-http-basic:2.3.0"
implementation ("dev.cdevents:cdevents-sdk-java:0.1.2")
testImplementation("com.icegreen:greenmail:1.5.14") {
exclude group: "com.sun.mail", module: "javax.mail"
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
/*
Copyright (C) 2023 Nordix Foundation.
For a full list of individual contributors, please see the commit history.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

SPDX-License-Identifier: Apache-2.0
*/

package com.netflix.spinnaker.echo.cdevents;

import com.netflix.spinnaker.echo.api.events.Event;
import dev.cdevents.CDEvents;
import dev.cdevents.constants.CDEventConstants;
import dev.cdevents.events.*;
jasonmcintosh marked this conversation as resolved.
Show resolved Hide resolved
import dev.cdevents.exception.CDEventsException;
import io.cloudevents.CloudEvent;
import java.net.URI;
import java.util.Map;
import java.util.Optional;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Component;

@Slf4j
@Component
public class CDEventsBuilderService {

public CloudEvent createCDEvent(
Map<String, Object> preference,
String application,
Event event,
Map<String, String> config,
String status,
String spinnakerUrl) {

String configType = Optional.ofNullable(config).map(c -> (String) c.get("type")).orElse(null);
Copy link

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?

Copy link
Contributor Author

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 configLink = Optional.ofNullable(config).map(c -> (String) c.get("link")).orElse(null);
Copy link

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

Copy link
Contributor Author

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 executionId =
Optional.ofNullable(event.content)
.map(e -> (Map) e.get("execution"))
.map(e -> (String) e.get("id"))
.orElse(null);

String executionUrl =
String.format(
"%s/#/applications/%s/%s/%s",
spinnakerUrl,
application,
configType == "stage" ? "executions/details" : configLink,
executionId);

String executionName =
Optional.ofNullable(event.content)
.map(e -> (Map) e.get("execution"))
.map(e -> (String) e.get("name"))
.orElse(null);

String cdEventsType =
Optional.ofNullable(preference).map(p -> (String) p.get("cdEventsType")).orElse(null);

CloudEvent ceToSend =
buildCloudEventWithCDEventType(
cdEventsType, executionId, executionUrl, executionName, spinnakerUrl, status);
if (ceToSend == null) {
log.error("Failed to created CDEvent with type {} as CloudEvent", cdEventsType);
throw new CDEventsException("Failed to created CDEvent as CloudEvent");
}
return ceToSend;
}

private CloudEvent buildCloudEventWithCDEventType(
String cdEventsType,
String executionId,
String executionUrl,
String executionName,
String spinnakerUrl,
String status) {
CloudEvent ceToSend = null;
switch (cdEventsType) {
case "dev.cdevents.pipelinerun.queued":
Copy link

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

Copy link
Contributor Author

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

Copy link

Choose a reason for hiding this comment

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

Yes, please do that :)

ceToSend =
createPipelineRunQueuedEvent(executionId, executionUrl, executionName, spinnakerUrl);
break;
case "dev.cdevents.pipelinerun.started":
ceToSend =
createPipelineRunStartedEvent(executionId, executionUrl, executionName, spinnakerUrl);
break;
case "dev.cdevents.pipelinerun.finished":
ceToSend =
createPipelineRunFinishedEvent(
executionId, executionUrl, executionName, spinnakerUrl, status);
break;
case "dev.cdevents.taskrun.started":
ceToSend =
createTaskRunStartedEvent(executionId, executionUrl, executionName, spinnakerUrl);
break;
case "dev.cdevents.taskrun.finished":
ceToSend =
createTaskRunFinishedEvent(
executionId, executionUrl, executionName, spinnakerUrl, status);
break;
default:
throw new CDEventsException(
Copy link

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

Copy link
Contributor Author

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.

Copy link

@xibz xibz Oct 14, 2023

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());
}

Copy link
Contributor Author

@rjalander rjalander Oct 16, 2023

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)
    );

"Invalid CDEvents Type " + cdEventsType + " provided to create CDEvent");
}
return ceToSend;
}

private CloudEvent createTaskRunFinishedEvent(
String executionId,
String executionUrl,
String executionName,
String spinnakerUrl,
String status) {
TaskRunFinishedCDEvent cdEvent = new TaskRunFinishedCDEvent();
Copy link

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

cdEvent.setSource(URI.create(spinnakerUrl));

cdEvent.setSubjectId(executionId);
cdEvent.setSubjectSource(URI.create(spinnakerUrl));
cdEvent.setSubjectTaskName(executionName);
cdEvent.setSubjectUrl(URI.create(executionUrl));
cdEvent.setSubjectErrors(status);
cdEvent.setSubjectPipelineRunId(executionId);
if (status.equals("complete")) {
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.SUCCESS);
} else if (status.equals("failed")) {
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.FAILURE);
}
return CDEvents.cdEventAsCloudEvent(cdEvent);
}

private CloudEvent createTaskRunStartedEvent(
String executionId, String executionUrl, String executionName, String spinnakerUrl) {
TaskRunStartedCDEvent cdEvent = new TaskRunStartedCDEvent();
cdEvent.setSource(URI.create(spinnakerUrl));

cdEvent.setSubjectId(executionId);
cdEvent.setSubjectSource(URI.create(spinnakerUrl));
cdEvent.setSubjectTaskName(executionName);
cdEvent.setSubjectUrl(URI.create(executionUrl));
cdEvent.setSubjectPipelineRunId(executionId);

return CDEvents.cdEventAsCloudEvent(cdEvent);
}

private CloudEvent createPipelineRunFinishedEvent(
String executionId,
String executionUrl,
String executionName,
String spinnakerUrl,
String status) {
PipelineRunFinishedCDEvent cdEvent = new PipelineRunFinishedCDEvent();
cdEvent.setSource(URI.create(spinnakerUrl));
cdEvent.setSubjectId(executionId);
cdEvent.setSubjectSource(URI.create(spinnakerUrl));
cdEvent.setSubjectPipelineName(executionName);
cdEvent.setSubjectUrl(URI.create(executionUrl));
cdEvent.setSubjectErrors(status);

if (status.equals("complete")) {
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.SUCCESS);
} else if (status.equals("failed")) {
cdEvent.setSubjectOutcome(CDEventConstants.Outcome.FAILURE);
}

return CDEvents.cdEventAsCloudEvent(cdEvent);
}

private CloudEvent createPipelineRunStartedEvent(
String executionId, String executionUrl, String executionName, String spinnakerUrl) {
PipelineRunStartedCDEvent cdEvent = new PipelineRunStartedCDEvent();
cdEvent.setSource(URI.create(spinnakerUrl));
cdEvent.setSubjectId(executionId);
cdEvent.setSubjectSource(URI.create(spinnakerUrl));
cdEvent.setSubjectPipelineName(executionName);
cdEvent.setSubjectUrl(URI.create(executionUrl));

return CDEvents.cdEventAsCloudEvent(cdEvent);
}

private CloudEvent createPipelineRunQueuedEvent(
String executionId, String executionUrl, String executionName, String spinnakerUrl) {
PipelineRunQueuedCDEvent cdEvent = new PipelineRunQueuedCDEvent();
cdEvent.setSource(URI.create(spinnakerUrl));
cdEvent.setSubjectId(executionId);
cdEvent.setSubjectSource(URI.create(spinnakerUrl));
cdEvent.setSubjectPipelineName(executionName);
cdEvent.setSubjectUrl(URI.create(executionUrl));

return CDEvents.cdEventAsCloudEvent(cdEvent);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
Copyright (C) 2023 Nordix Foundation.
For a full list of individual contributors, please see the commit history.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

SPDX-License-Identifier: Apache-2.0
*/
package com.netflix.spinnaker.echo.cdevents;

import io.cloudevents.CloudEvent;
import io.cloudevents.core.message.MessageWriter;
import io.cloudevents.http.HttpMessageFactory;
import java.io.IOException;
import java.io.OutputStream;
import java.io.UncheckedIOException;
import java.net.HttpURLConnection;
import java.net.URL;
import org.springframework.stereotype.Component;

@Component
public class CDEventsSenderService {

/**
* Sends CDEvent to the Configured ClouEvent broker URL.
*
* @param ceToSend
* @param url
* @return HttpURLConnection
* @throws IOException
*/
public HttpURLConnection sendCDEvent(CloudEvent ceToSend, URL url) throws IOException {
HttpURLConnection httpUrlConnection = createConnection(url);
MessageWriter messageWriter = createMessageWriter(httpUrlConnection);
Copy link
Member

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.

Copy link

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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

messageWriter.writeBinary(ceToSend);

return httpUrlConnection;
}

/**
* @param url
* @return httpUrlConnection
* @throws IOException
*/
private HttpURLConnection createConnection(URL url) throws IOException {
HttpURLConnection httpUrlConnection = (HttpURLConnection) url.openConnection();
httpUrlConnection.setRequestMethod("POST");
httpUrlConnection.setDoOutput(true);
httpUrlConnection.setDoInput(true);
return httpUrlConnection;
}

/**
* @param httpUrlConnection
* @return messageWriter
*/
private MessageWriter createMessageWriter(HttpURLConnection httpUrlConnection) {
return HttpMessageFactory.createWriter(
httpUrlConnection::setRequestProperty,
body -> {
try {
if (body != null) {
httpUrlConnection.setRequestProperty("content-length", String.valueOf(body.length));
try (OutputStream outputStream = httpUrlConnection.getOutputStream()) {
outputStream.write(body);
}
} else {
httpUrlConnection.setRequestProperty("content-length", "0");
}
} catch (IOException t) {
throw new UncheckedIOException(t);
}
});
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright 2020 Cerner Corporation
Copy link
Member

Choose a reason for hiding this comment

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

2023 Nordix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Updated

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.netflix.spinnaker.echo.notification;

import com.netflix.spinnaker.echo.api.events.Event;
import com.netflix.spinnaker.echo.cdevents.CDEventsBuilderService;
import com.netflix.spinnaker.echo.cdevents.CDEventsSenderService;
import dev.cdevents.exception.CDEventsException;
import io.cloudevents.CloudEvent;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.Map;
import java.util.Optional;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.stereotype.Service;
import retrofit.mime.TypedByteArray;

@Slf4j
@ConditionalOnProperty("cdevents.enabled")
@Service
public class CDEventsNotificationAgent extends AbstractEventNotificationAgent {
@Autowired CDEventsBuilderService cdEventsBuilderService;
@Autowired CDEventsSenderService cdEventsSenderService;

@Override
public String getNotificationType() {
return "cdevents";
}

@Override
public void sendNotifications(
Map<String, Object> preference,
String application,
Event event,
Map<String, String> config,
String status) {
log.info("Sending CDEvents notification..");

String executionId =
Optional.ofNullable(event.content)
.map(e -> (Map) e.get("execution"))
.map(e -> (String) e.get("id"))
.orElse(null);
String cdEventsType =
Optional.ofNullable(preference).map(p -> (String) p.get("cdEventsType")).orElse(null);
String eventsBrokerUrl =
Optional.ofNullable(preference).map(p -> (String) p.get("address")).orElse(null);

try {
CloudEvent ceToSend =
cdEventsBuilderService.createCDEvent(
preference, application, event, config, status, getSpinnakerUrl());
log.info(
"Sending CDEvent {} notification to events broker url {}", cdEventsType, eventsBrokerUrl);
HttpURLConnection response =
cdEventsSenderService.sendCDEvent(ceToSend, new URL(eventsBrokerUrl));
if (response != null) {
log.info(
"Received response from events broker : {} {} for execution id {}. {}",
response.getResponseCode(),
response.getResponseMessage(),
executionId,
new String(((TypedByteArray) response.getContent()).getBytes()));
}

} catch (Exception e) {
log.error("Exception occurred while sending CDEvent {}", e);
throw new CDEventsException("Exception occurred while sending CDEvent", e);
Copy link
Member

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.

Copy link

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

Copy link
Contributor Author

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

}
}
}