feat(cdevents-webhooks) : Consume CDEvents webhook API implementation #1651

rjalander · 2023-04-11T13:32:41Z

Changes to implement a new CDEvents webhook API /webhooks/cdevents/{source} end point to Consume CDEvent

Note:
The implementation is as per the RFC design
https://github.com/spinnaker/governance/blob/master/rfc/cdevents-spinnaker.md#implementation-details

Dependent Spinnaker/deck PR - spinnaker/deck#9977
Dependent Spinnaker/echo PR - spinnaker/echo#1290

spinnakerbot · 2023-04-11T15:50:13Z

The following commits need their title changed:

6efb466: test for cdevents API

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

…pectations

…oks_cdevents

mattgogerly · 2023-06-16T07:32:06Z

@Mergifyio update

mergify · 2023-06-16T07:32:09Z

update

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@Nordix needs to authorize modification on its head branch.
err-code: 427B6

rjalander · 2023-06-16T08:38:46Z

Thank you for checking this PR @mattgogerly, I have updated the base branch now.

mattgogerly · 2023-06-16T08:44:13Z

Thanks, sorry it's taken so long to get this reviewed. I'm double checking with the TOC and if there's no objections I'll get these merged.

gate-web/src/main/groovy/com/netflix/spinnaker/gate/services/WebhookService.groovy

gate-web/src/test/groovy/com/netflix/spinnaker/gate/controllers/WebhookControllerSpec.groovy

mattgogerly · 2023-06-16T08:47:50Z

gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/GateConfig.groovy

@@ -340,6 +342,7 @@ class GateConfig extends RedisHttpSessionConfiguration {
      .enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL)
      .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
      .registerModule(new JavaTimeModule())
+      .configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);


Could you elaborate on why these changes are necessary?

yeah with out setting this, when sending CDEvent POST request to this webhook will result in below error in the response;

{"error":"Internal Server Error","exception":"retrofit.RetrofitError","message":"com.fasterxml.jackson.databind.exc.InvalidDefinitionException: No serializer found for class io.cloudevents.core.data.BytesCloudEventData and no properties discovered to create BeanSerializer (to avoid exception, disable SerializationFeature.FAIL_ON_EMPTY_BEANS) (through reference chain: io.cloudevents.core.v1.CloudEventV1[\"data\"])"}

Hm, I'd like to get @link108 or @dbyron-sf's thoughts on this. I'm hesitant to change this for the global ObjectMapper without thorough understanding of any side effects.

This setting is what I need it for echoservice.
If this is fine, I can only set it for echo as below also works for me

Suggested change

.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);

if(serviceName.equals("echo")){

objectMapper.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);

}

@link108 @dbyron-sf any suggestion on this change?

As per the master branch changes which is conflicting with this change, now objectMapper is configured using Jackson2ObjectMapperBuilder,
But still the property spring.jackson.serialization.fail-on-empty-beans=false needs to be added in gate-web/src/main/resources/application.properties as per the master branch change. Please share your thoughts on this.

I'm not super familiar with the consequences of setting fail-on-empty-beans to false. Maybe @jvz knows?

What would it take with the latest code on master to configure it that way only for echo?

I'm not really sure myself what that does. I learn something new about Jackson every time I use it, though! If the setting isn't safe to set globally, then what you can do is update the EchoService bean to use the objectMapperBuilder field to create a custom ObjectMapper as that bean is prototype-scoped.

Using the same way, setting this configuration only for echo as suggested above with in the buildService method

gate-web/gate-web.gradle

gate-core/gate-core.gradle

jvz · 2023-07-10T17:29:34Z

gate-core/src/main/java/com/netflix/spinnaker/gate/config/CloudEventHandlerConfiguration.java

+
+  @Override
+  public void configureMessageConverters(List<HttpMessageConverter<?>> converters) {
+    converters.add(0, new CloudEventHttpMessageConverter());


Doing this would set the default message converter for all of Gate's controllers, wouldn't it? Or is this a different MIME type?

This is actually a different type, this will help to write an endpoints with CloudEvent input or output,
More information here

What I mean is the content-type or accept HTTP headers. If this is just application/json, then it will override the converter used for every other request in gate.

I think this configuration will add to the list of existing converters but not override or replace the configuration.

It adds it to the front of the list, though! If you can add it without an index, then that should work, though then you could just define a bean of CloudEventHttpMessageConverter.

Adding this converter without index is causing an issue with InvalidDefinitionException,
So keeping index at 0 and defined a bean of CloudEventHttpMessageConverter.

Putting it at index 0 means making it top priority. That is causing it to be used instead of the JSON message converter you were updating before making the custom one. It might just be simpler to update the properties file like you did previously if this setting is required globally. Alternatively, I'd suggest looking at Jackson's annotations and mixins for a potentially localized solution here, but perhaps that's unnecessary. While Jackson provides numerous knobs for tuning things, it's kind of complicated in my experience.

Yes in this case its working only when keeping this converter at index 0.

Updating properties file is actually for objectMapper configuration(SerializationFeature.FAIL_ON_EMPTY_BEANS) and not for CloudEventHttpMessageConverter If I am not wrong.

Yeah I don't see any Jackson's annotations to set the localized configuration at a request level for adding converters like CloudEventHttpMessageConverter but it is possible when we use rest client for any requests something like below,
restTemplate.getMessageConverters().add(0, new CloudEventHttpMessageConverter());

Read this: https://www.baeldung.com/spring-httpmessageconverter-rest

Sorry for the delay in response.
I have re-tested this, it works perfectly fine when keeping the CloudEventHandlerConfiguration.java under gate-web project instead of gate-core with out indexing at top priority.(not sure why it's working other way when index set to 0)

jvz · 2023-07-10T17:29:57Z

gate-web/src/main/groovy/com/netflix/spinnaker/gate/config/CloudEventHandlerConfiguration.java

+
+  @Override
+  public void configureMessageConverters(List<HttpMessageConverter<?>> converters) {
+    converters.add(0, new CloudEventHttpMessageConverter());


Doing this twice seems redundant?

As we have different endpoints written in gate-core and gate-web separately, we need this configuration to handle the CloudEvent conversion by Spring framework.

I'm pretty sure that beans defined in core also show up in web regardless if it's an api or implementation dependency.

yes apologies, it works keeping this config file in either core or web. I will delete this config file from web

…oks_cdevents

Signed-off-by: Jalander Ramagiri <[email protected]>

…oks_cdevents

Signed-off-by: Jalander Ramagiri <[email protected]>

…oks_cdevents

rjalander · 2023-07-24T09:38:45Z

@mattgogerly @jvz @dbyron-sf Thank you all for reviewing this PR, please let me know If you have any other comments.
Thank you

jvz

Thanks for dealing with the Jackson madness! I'm alright with the changes now.

rjalander · 2023-08-09T08:38:14Z

Thanks @jvz.
@mattgogerly @dbyron-sf can you approve/merge this PR If no other comments.

rjalander · 2023-08-21T14:16:41Z

@mattgogerly in the same context I have created PRs for producing CDEvents using Notifications,
spinnaker/deck#9997
spinnaker/echo#1295
Please add reviewers to the above PRs and help in reviewing. Thank you.

feat(cdevents-webhooks) : CDEvents webhook API implementation

a691cc9

test(cdevents-webhooks): test for cdevents API

021bbc4

rjalander force-pushed the webhooks_cdevents branch from 6efb466 to 021bbc4 Compare April 11, 2023 16:13

Merge branch 'master' into webhooks_cdevents

812bdf2

This was referenced Apr 12, 2023

feat(cdevents-webhooks) : Consume CDEvents webhook API implementation spinnaker/echo#1290

Merged

feat(cdevents-webhooks) : CDEvents Webhook type in Automated Triggers type spinnaker/deck#9977

Merged

rjalander and others added 6 commits April 19, 2023 09:57

Merge branch 'master' into webhooks_cdevents

265d5be

Merge branch 'master' into webhooks_cdevents

17cb33e

Merge branch 'master' into webhooks_cdevents

3aba97e

fix(cdevents-webhooks) : Response type to match with events-broker ex…

cc63a33

…pectations

Merge branch 'webhooks_cdevents' of github.com:Nordix/gate into webho…

9e8c95e

…oks_cdevents

Merge branch 'master' into webhooks_cdevents

d466002

Merge branch 'master' into webhooks_cdevents

14fc1f4

mattgogerly reviewed Jun 16, 2023

View reviewed changes

rjalander and others added 4 commits June 16, 2023 14:00

Merge branch 'master' into webhooks_cdevents

5cbba1e

addressing review comments

776b00c

Merge branch 'master' into webhooks_cdevents

5065cf2

Merge branch 'master' into webhooks_cdevents

5659283

rjalander requested a review from mattgogerly July 3, 2023 08:40

resolve merge conflicts

eaf7f63

rjalander requested review from dbyron-sf and jvz July 10, 2023 15:39

jvz reviewed Jul 10, 2023

View reviewed changes

Merge branch 'master' into webhooks_cdevents

c122959

Jalander Ramagiri and others added 3 commits July 12, 2023 14:32

removing the redundant config file

0418061

Merge branch 'webhooks_cdevents' of github.com:Nordix/gate into webho…

0ecccfd

…oks_cdevents

Merge branch 'master' into webhooks_cdevents

d94e741

rjalander requested a review from jvz July 13, 2023 08:59

Jalander Ramagiri and others added 5 commits July 19, 2023 09:44

fix review comment on using CloudEventHttpMessageConverter

187ec98

Signed-off-by: Jalander Ramagiri <[email protected]>

Merge branch 'webhooks_cdevents' of github.com:Nordix/gate into webho…

7654eb8

…oks_cdevents

Merge branch 'master' into webhooks_cdevents

4ff96e1

adding index to cloudEvent HttpMessageConverter

574b78c

Signed-off-by: Jalander Ramagiri <[email protected]>

Merge branch 'webhooks_cdevents' of github.com:Nordix/gate into webho…

d8e9e90

…oks_cdevents

rjalander and others added 3 commits July 26, 2023 17:38

Merge branch 'master' into webhooks_cdevents

99854c0

fix: Placing the CloudEventHandlerConfiguration in gate-web project

091123e

fix: code violation

0189536

jvz approved these changes Aug 9, 2023

View reviewed changes

mattgogerly approved these changes Aug 9, 2023

View reviewed changes

Merge branch 'master' into webhooks_cdevents

3e2be52

mattgogerly added the ready to merge Approved and ready for merge label Aug 11, 2023

mergify bot added the auto merged label Aug 11, 2023

Merge branch 'master' into webhooks_cdevents

2d81501

mergify bot merged commit f57df15 into spinnaker:master Aug 14, 2023
4 checks passed

spinnakerbot added the target-release/1.32 label Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdevents-webhooks) : Consume CDEvents webhook API implementation #1651

feat(cdevents-webhooks) : Consume CDEvents webhook API implementation #1651

rjalander commented Apr 11, 2023 •

edited

Loading

spinnakerbot commented Apr 11, 2023

mattgogerly commented Jun 16, 2023

mergify bot commented Jun 16, 2023

rjalander commented Jun 16, 2023

mattgogerly commented Jun 16, 2023

mattgogerly Jun 16, 2023

rjalander Jun 21, 2023

mattgogerly Jun 21, 2023

rjalander Jun 22, 2023

rjalander Jul 4, 2023

rjalander Jul 5, 2023

dbyron-sf Jul 5, 2023

jvz Jul 6, 2023

rjalander Jul 10, 2023

jvz Jul 10, 2023

rjalander Jul 11, 2023

jvz Jul 14, 2023

rjalander Jul 17, 2023

jvz Jul 18, 2023

rjalander Jul 20, 2023

jvz Jul 24, 2023

rjalander Jul 26, 2023

jvz Jul 28, 2023

rjalander Aug 8, 2023

jvz Jul 10, 2023

rjalander Jul 11, 2023

jvz Jul 11, 2023

rjalander Jul 12, 2023

rjalander commented Jul 24, 2023

jvz left a comment

rjalander commented Aug 9, 2023

rjalander commented Aug 21, 2023

-      .configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);
+   if(serviceName.equals("echo")){
+      objectMapper.configure(SerializationFeature.FAIL_ON_EMPTY_BEANS, false);
+   }

feat(cdevents-webhooks) : Consume CDEvents webhook API implementation #1651

feat(cdevents-webhooks) : Consume CDEvents webhook API implementation #1651

Conversation

rjalander commented Apr 11, 2023 • edited Loading

spinnakerbot commented Apr 11, 2023

mattgogerly commented Jun 16, 2023

mergify bot commented Jun 16, 2023

❌ Pull request can't be updated with latest base branch changes

rjalander commented Jun 16, 2023

mattgogerly commented Jun 16, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjalander commented Jul 24, 2023

jvz left a comment

Choose a reason for hiding this comment

rjalander commented Aug 9, 2023

rjalander commented Aug 21, 2023

rjalander commented Apr 11, 2023 •

edited

Loading