feat(cdevents-notification): CDEvents notification to produce CDEvents #9997

rjalander · 2023-05-19T11:54:47Z

Changes to introduce new Notification type CDEvents, to produce CDEvent using CDEvents-Java-SDK from Spinnaker Pipeline configuration.

Pipeline configuration after saving the Pipeline with CDEvents notification;

  "notifications": [
    {
     "address": "http://10.101.1.48:8010/default/events-broker",
     "cdEventsType": "dev.cdevents.pipelinerun.finished",
     "level": "stage",
     "type": "cdevents",
     "when": [
      "stage.complete"
     ]
    }
   ],

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

Dependent Spinnaker/echo PR - spinnaker/echo#1295

…otification_cdevents

jcavanagh · 2023-12-11T22:08:10Z

packages/core/src/presentation/forms/validation/validators.ts

@@ -9,13 +9,31 @@ const VALID_EMAIL_REGEX = new RegExp(
  '^(([^<>()\\[\\]\\\\.,;:\\s@"]+(\\.[^<>()\\[\\]\\\\.,;:\\s@"]+)*)|(".+"))@((\\[[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}])|(([a-zA-Z\\-0-9]+\\.)+[a-zA-Z]{2,}))$',
 );

+const VALID_URL = new RegExp('^https?://.+$');


It would be good to make this configurable from settings.js - operators may wish to only allow just one or a few URLs here.

@jcavanagh Thank you for reviewing this PR
can you please point some examples how exactly these placeholders/regex patterns can be configurable in settings.js,
and I think these are fixed values do you see any need of making these configurable.

The URL here is a single Message Broker URL where the CDEvents should be delivered.

The need is as I described in the first comment - operators may wish to only allow just one or a few URLs here, or limit that to a subset of domains.

Exfiltration of CDEvents can be a serious security problem, and it would be useful to have an additional guard on the frontend to have that experience align with any guards on the backend.

Or, we could just delegate validation to the backend, and skip the frontend URL validation entirely.

As an example, the code in here would first check for a value from the Spinnaker SETTINGS object, and fall back to a reasonable default if not set.

import { SETTINGS } from '../config'; ... const patternStr = SETTINGS?.cdevents?.validUrlPattern ?? '^https?://.+$'; const VALID_URL = new RegExp(patternStr); ...

Or something along those lines. The import path will vary based on where this file is relative to that in the Deck source tree.

jcavanagh · 2023-12-11T22:08:47Z

packages/core/src/presentation/forms/validation/validators.ts

@@ -9,13 +9,31 @@ const VALID_EMAIL_REGEX = new RegExp(
  '^(([^<>()\\[\\]\\\\.,;:\\s@"]+(\\.[^<>()\\[\\]\\\\.,;:\\s@"]+)*)|(".+"))@((\\[[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}])|(([a-zA-Z\\-0-9]+\\.)+[a-zA-Z]{2,}))$',
 );

+const VALID_URL = new RegExp('^https?://.+$');
+
+const VALID_CDEVENT_REGEX = new RegExp('^dev\\.cdevents\\.[^.]+\\.[^.]+$');


It would also be good to make this configurable from settings.js - some enterprises might choose a different type schema.

jcavanagh · 2023-12-11T22:14:33Z

packages/core/src/notification/selector/types/cdevents/CDEventsNotificationType.tsx

+            <TextInput
+              inputClassName={'form-control input-sm'}
+              {...props}
+              placeholder="URL starts with https://events-broker-address/default/events-broker/"


Placeholder text is best when it is an actual possible value, not a description (e.g. placeholder="https://...").

The screenshot in the PR already has it like that, but this is different here in the code.

More descriptive help can be added via a help field on the FormikFormField component, e.g.:

<FormikFormField ... help={<HelpField id="..." />} ... input={...} />

We may also need this placeholder value configurable from settings.js, as it would need to match the custom validation.

I followed the same format how it is used today for MS Teams or GoogleChat notifications, not sure If we change for this alone, will it be a different user experience?

Configurable from settings.js, not sure If I understood this correctly is that keeping a key:value pair in settings.js something like this, but this will actually keep the default value right, rather this value should be just an example URL

I don't mind that much either way on the content of the placeholder text. However, if the URL validation is configurable, the placeholder value should not conflict with it.

I would probably suggest removing it in this case, as the path on the event broker is not in the CDEvents spec, and so the /default/events-broker/ path is probably more confusing than helpful.

Ok, changing the placeholder for Events Broker URL to a message "Enter an events message broker URL" and make configurable through settings

jcavanagh · 2023-12-11T22:17:33Z

packages/core/src/notification/selector/types/cdevents/CDEventsNotificationType.tsx

+            <TextInput
+              inputClassName={'form-control input-sm'}
+              {...props}
+              placeholder="CDEvents Type starts with dev.cdevents.<subject>.<predicate>"


Similar to the above comment, placeholder text is best as an actual value, and would match the PR screenshot. More descriptive help can also be added if needed, as above.

We may also need this placeholder value configurable from settings.js, as it would need to match the custom validation.

when looking at other notifications I think placeholders are showing an example/description how actual value should be, in this case the input for CDEvents Type can have different type of events from CDEvents spec and starts with dev.cdevents.

The CDEvents spec details that event types "should" start with dev.cdevents, not "must". Since the spec allows this to be configurable, we should also allow that to be configurable.

That's the typical interpretation of that language in other RFC processes.

As for the placeholder value itself, I don't have a strong opinion on the text of it, so long as it doesn't conflict with event type validation.

Agree, changing the placeholder for CDEvents Type to a message "Enter a CDEvents type" and make configurable through settings

jcavanagh

Looks awesome, thank you!

rjalander · 2023-12-15T09:42:00Z

@jcavanagh @xibz Can you please merge this if there are no other comments. Thank you.

jasonmcintosh · 2023-12-22T17:54:01Z

FYI Soon as echo is updated plan on merging most of these ;)

feat(cdevents-notification): CDEvents notification to produce CDEvents

c6c2f18

rjalander mentioned this pull request May 19, 2023

feat(cdevents-notification): Implementing produce CDEvents using Notification spinnaker/echo#1295

Merged

rjalander and others added 9 commits May 19, 2023 13:00

Merge branch 'master' into notification_cdevents

9b46d39

Merge branch 'master' into notification_cdevents

129c59d

Merge branch 'master' into notification_cdevents

3ac861a

Merge branch 'master' into notification_cdevents

bf27653

Merge branch 'master' into notification_cdevents

f1f5934

fix: linter issue Sort the import statements

aaa1a7f

Merge branch 'notification_cdevents' of github.com:Nordix/deck into n…

e27f287

…otification_cdevents

fix: cdevents files formatting

1f702ec

Merge branch 'master' into notification_cdevents

a2aa0e9

rjalander mentioned this pull request Aug 21, 2023

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

Merged

rjalander and others added 9 commits October 17, 2023 14:07

Merge branch 'master' into notification_cdevents

26f6792

adding validations for URL and cdevents type

16bf5e8

fix prettier formatting issue

af60d16

Merge branch 'master' into notification_cdevents

5bffdf9

update regex for URL validation

b27eaff

Merge branch 'notification_cdevents' of github.com:Nordix/deck into n…

0e5d44f

…otification_cdevents

Merge branch 'master' into notification_cdevents

de0e35f

Merge branch 'master' into notification_cdevents

591cc9c

Merge branch 'master' into notification_cdevents

09ed874

jcavanagh suggested changes Dec 11, 2023

View reviewed changes

rjalander and others added 4 commits December 12, 2023 09:48

Merge branch 'master' into notification_cdevents

096330b

review comments on placeholder/regex configurable

9778374

lint issue with order import

9099a58

lint issue Prettier

491e216

rjalander requested a review from jcavanagh December 13, 2023 16:27

jcavanagh approved these changes Dec 13, 2023

View reviewed changes

xibz approved these changes Dec 13, 2023

View reviewed changes

jasonmcintosh approved these changes Dec 22, 2023

View reviewed changes

jasonmcintosh added the ready to merge Reviewed and ready for merge label Dec 22, 2023

mergify bot added the auto merged Merged automatically by a bot label Dec 22, 2023

mergify bot merged commit fc96adb into spinnaker:master Dec 22, 2023
4 checks passed

spinnakerbot added the target-release/1.34 label Dec 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cdevents-notification): CDEvents notification to produce CDEvents #9997

feat(cdevents-notification): CDEvents notification to produce CDEvents #9997

rjalander commented May 19, 2023

jcavanagh Dec 11, 2023

rjalander Dec 12, 2023 •

edited

Loading

jcavanagh Dec 12, 2023 •

edited

Loading

jcavanagh Dec 12, 2023 •

edited

Loading

jcavanagh Dec 11, 2023

jcavanagh Dec 11, 2023

rjalander Dec 12, 2023

jcavanagh Dec 12, 2023

rjalander Dec 13, 2023 •

edited

Loading

jcavanagh Dec 11, 2023

rjalander Dec 12, 2023

jcavanagh Dec 12, 2023

rjalander Dec 13, 2023

jcavanagh left a comment

rjalander commented Dec 15, 2023

jasonmcintosh commented Dec 22, 2023

feat(cdevents-notification): CDEvents notification to produce CDEvents #9997

feat(cdevents-notification): CDEvents notification to produce CDEvents #9997

Conversation

rjalander commented May 19, 2023

Choose a reason for hiding this comment

rjalander Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

jcavanagh Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

jcavanagh Dec 12, 2023 • 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

rjalander Dec 13, 2023 • 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

jcavanagh left a comment

Choose a reason for hiding this comment

rjalander commented Dec 15, 2023

jasonmcintosh commented Dec 22, 2023

rjalander Dec 12, 2023 •

edited

Loading

jcavanagh Dec 12, 2023 •

edited

Loading

jcavanagh Dec 12, 2023 •

edited

Loading

rjalander Dec 13, 2023 •

edited

Loading