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): CDEvents notification to produce CDEvents #9997

Merged
merged 23 commits into from
Dec 22, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c6c2f18
feat(cdevents-notification): CDEvents notification to produce CDEvents
May 18, 2023
9b46d39
Merge branch 'master' into notification_cdevents
rjalander May 19, 2023
129c59d
Merge branch 'master' into notification_cdevents
rjalander Jun 16, 2023
3ac861a
Merge branch 'master' into notification_cdevents
rjalander Jul 13, 2023
bf27653
Merge branch 'master' into notification_cdevents
rjalander Jul 24, 2023
f1f5934
Merge branch 'master' into notification_cdevents
rjalander Jul 26, 2023
aaa1a7f
fix: linter issue Sort the import statements
Jul 26, 2023
e27f287
Merge branch 'notification_cdevents' of github.com:Nordix/deck into n…
Jul 26, 2023
1f702ec
fix: cdevents files formatting
Jul 27, 2023
a2aa0e9
Merge branch 'master' into notification_cdevents
rjalander Aug 9, 2023
26f6792
Merge branch 'master' into notification_cdevents
rjalander Oct 17, 2023
16bf5e8
adding validations for URL and cdevents type
Oct 19, 2023
af60d16
fix prettier formatting issue
Oct 19, 2023
5bffdf9
Merge branch 'master' into notification_cdevents
rjalander Oct 24, 2023
b27eaff
update regex for URL validation
Oct 24, 2023
0e5d44f
Merge branch 'notification_cdevents' of github.com:Nordix/deck into n…
Oct 24, 2023
de0e35f
Merge branch 'master' into notification_cdevents
rjalander Nov 2, 2023
591cc9c
Merge branch 'master' into notification_cdevents
rjalander Nov 27, 2023
09ed874
Merge branch 'master' into notification_cdevents
rjalander Dec 6, 2023
096330b
Merge branch 'master' into notification_cdevents
rjalander Dec 12, 2023
9778374
review comments on placeholder/regex configurable
Dec 13, 2023
9099a58
lint issue with order import
Dec 13, 2023
491e216
lint issue Prettier
Dec 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions halconfig/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ window.spinnakerSettings = {
pubsub: {
enabled: true,
},
cdevents: {
enabled: true,
},
slack: slack,
sms: sms,
},
Expand Down
3 changes: 3 additions & 0 deletions packages/app/src/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@ window.spinnakerSettings = {
sms: {
enabled: true,
},
cdevents: {
enabled: true,
},
},
onDemandClusterThreshold: Number(onDemandClusterThreshold),
pollSchedule: 30000,
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/config/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface INotificationSettings {
pubsub: { enabled: boolean };
slack: { botName: string; enabled: boolean };
sms: { enabled: boolean };
cdevents: { enabled: boolean };
}

export interface IFeatures {
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/notification/modal/NotificationDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ export class NotificationDetails extends React.Component<INotificationDetailsPro
}

private renderCustomMessage = (type: string, whenOption: string): React.ReactNode => {
if (whenOption !== 'manualJudgment' && ['email', 'slack', 'googlechat', 'microsoftteams'].includes(type)) {
if (
whenOption !== 'manualJudgment' &&
['email', 'slack', 'googlechat', 'microsoftteams', 'cdevents'].includes(type)
) {
return (
<FormikFormField
name={`message["${whenOption}"].text`}
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/notification/notification.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { INotificationTypeConfig } from '../domain';
import { Registry } from '../registry';

import { bearyChatNotification } from './selector/types/bearychat/beary.notification';
import { cdEventsNotification } from './selector/types/cdevents/cdevents.notification';
import { emailNotification } from './selector/types/email/email.notification';
import { githubstatusNotification } from './selector/types/githubstatus/githubstatus.notification';
import { googlechatNotification } from './selector/types/googlechat/googlechat.notification';
Expand All @@ -23,6 +24,7 @@ import { smsNotification } from './selector/types/sms/sms.notification';
pubsubNotification,
slackNotification,
smsNotification,
cdEventsNotification,
].forEach((config: INotificationTypeConfig) => {
if (SETTINGS.notifications) {
const notificationSetting: { enabled: boolean; botName?: string } =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React from 'react';

import type { INotificationTypeCustomConfig } from '../../../../domain';
import { FormikFormField, TextInput, Validators } from '../../../../presentation';

export class CDEventsNotificationType extends React.Component<INotificationTypeCustomConfig> {
public render() {
const { fieldName } = this.props;
return (
<>
<FormikFormField
label="Events Broker URL"
name={fieldName ? `${fieldName}.address` : 'address'}
validate={Validators.skipIfSpel(Validators.urlValue('Please enter a valid URL'))}
input={(props) => (
<TextInput
inputClassName={'form-control input-sm'}
{...props}
placeholder="URL starts with https://events-broker-address/default/events-broker/"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rjalander rjalander Dec 13, 2023

Choose a reason for hiding this comment

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

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

/>
)}
required={true}
/>
<FormikFormField
label="CDEvents Type"
name={fieldName ? `${fieldName}.cdEventsType` : 'cdEventsType'}
validate={Validators.skipIfSpel(Validators.cdeventsTypeValue('Please enter a valid CDEvents Type'))}
input={(props) => (
<TextInput
inputClassName={'form-control input-sm'}
{...props}
placeholder="CDEvents Type starts with dev.cdevents.<subject>.<predicate>"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/>
)}
required={true}
/>
</>
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { CDEventsNotificationType } from './CDEventsNotificationType';
import type { INotificationTypeConfig } from '../../../../domain';

export const cdEventsNotification: INotificationTypeConfig = {
component: CDEventsNotificationType,
key: 'cdevents',
label: 'CDEvents',
};
1 change: 1 addition & 0 deletions packages/core/src/notification/selector/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from './microsoftteams/MicrosoftTeamsNotificationType';
export * from './pubsub/PubsubNotificationType';
export * from './slack/SlackNotificationType';
export * from './sms/SmsNotificationType';
export * from './cdevents/CDEventsNotificationType';
20 changes: 20 additions & 0 deletions packages/core/src/presentation/forms/validation/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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?://.+$');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rjalander rjalander Dec 12, 2023

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@jcavanagh jcavanagh Dec 12, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jcavanagh jcavanagh Dec 12, 2023

Choose a reason for hiding this comment

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

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.


const VALID_CDEVENT_REGEX = new RegExp('^dev\\.cdevents\\.[^.]+\\.[^.]+$');
Copy link
Contributor

Choose a reason for hiding this comment

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

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


const emailValue = (message?: string): IValidator => {
return function emailValue(val: string, label = THIS_FIELD) {
message = message || `${label} is not a valid email address.`;
return val && !VALID_EMAIL_REGEX.test(val) && message;
};
};

const urlValue = (message?: string): IValidator => {
return function urlValue(val: string, label = THIS_FIELD) {
message = message || `${label} is not a valid URL.`;
return val && !VALID_URL.test(val) && message;
};
};

const cdeventsTypeValue = (message?: string): IValidator => {
return function cdeventsTypeValue(val: string, label = THIS_FIELD) {
message = message || `${label} is not a valid CDEvents Type.`;
return val && !VALID_CDEVENT_REGEX.test(val) && message;
};
};

const isRequired = (message?: string): IValidator => {
return function isRequired(val: any, label = THIS_FIELD) {
message = message || `${label} is required.`;
Expand Down Expand Up @@ -138,6 +156,8 @@ export const Validators = {
arrayNotEmpty,
checkBetween,
emailValue,
cdeventsTypeValue,
urlValue,
isNum,
isRequired,
isValidJson,
Expand Down