-
Notifications
You must be signed in to change notification settings - Fork 903
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
Conversation
…otification_cdevents
…otification_cdevents
@@ -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?://.+$'); |
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.
It would be good to make this configurable from settings.js
- operators may wish to only allow just one or a few URLs 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.
@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.
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.
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.
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 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.
@@ -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\\.[^.]+\\.[^.]+$'); |
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.
It would also be good to make this configurable from settings.js
- some enterprises might choose a different type schema.
<TextInput | ||
inputClassName={'form-control input-sm'} | ||
{...props} | ||
placeholder="URL starts with https://events-broker-address/default/events-broker/" |
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.
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.
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 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
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 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.
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.
Ok, changing the placeholder for Events Broker URL to a message "Enter an events message broker URL" and make configurable through settings
<TextInput | ||
inputClassName={'form-control input-sm'} | ||
{...props} | ||
placeholder="CDEvents Type starts with dev.cdevents.<subject>.<predicate>" |
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.
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.
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.
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.
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.
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.
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.
Agree, changing the placeholder for CDEvents Type to a message "Enter a CDEvents type" and make configurable through settings
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.
Looks awesome, thank you!
@jcavanagh @xibz Can you please merge this if there are no other comments. Thank you. |
FYI Soon as echo is updated plan on merging most of these ;) |
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;
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