[Uptime] Refactor cert alerts from batched to individual #102138

dominiqueclarke · 2021-06-14T21:07:25Z

This PR transitions batched certificate alerts into individual certificate alerts. Previously, 1 alert was generated for all certs that listed a summary of the number of expiring and aging certs. Now there is one alert for each expired or aging cert along with context on it's status. An example of the alert content can be found below.

Here are some highlights form the implementation

Note
This PR will break the current batched cert alert experience. The new alert executor function will no longer provides the action variables (for example, expiring/aging count and batched summary strings) necessary for the batched alert message, and the new individual alert instances will not work with the old default action message, as it would create duplicate alert strings which the alerting framework rejects as duplicate alert instances. This creates a need for a migration to transition the default action message to the updated individual message. Some consideration may need to happen for users who have created custom action messages that expect the previous action variables.

This PR creates a new TLS rule type tlsIndividual, while maintaining support for the old rule type. All of the old rule type server side code is maintained in a separate code path in order to keep compatibility with old batched certificate rules.

Author Checklist

Accessibility has been considered, relevant aria tags and other accessibility features implemented
note: This feature is purely technical in nature, and does not directly touch UI elements that need accessibility considerations.
Telemetry has been added where relevant @justinkambic @shahzad31 Do we currently have telemetry in place for alerts?
Docs have been added to this PR covering any new, changed, or removed features
Testing for expected behavior is in place
- Automated tests exist to cover expected and edge case conditions
- User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
- ~~Any special/edge cases that need to be manually tested must be documented~~
- Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)
  note: This feature is purely technical in nature, and does not directly touch UI elements that need accessibility considerations.

Reviewer Checklist

Accessibility has been considered, relevant aria tags and other accessibility features implemented
Telemetry has been added where relevant
Docs have been added to this PR covering any new, changed, or removed features
Testing for expected behavior is in place
- Automated tests exist to cover expected and edge case conditions
- User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
- Any special/edge cases that need to be manually tested must be documented
- Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-06-14T21:07:29Z

Pinging @elastic/uptime (Team:uptime)

…ype to legacy

…0-individual-cert-alerts

dominiqueclarke · 2021-06-16T11:16:09Z

@elasticmachine merge upstream

…b.com/dominiqueclarke/kibana into feature/101150-individual-cert-alerts

dominiqueclarke · 2021-06-18T13:23:15Z

@elasticmachine merge upstream

x-pack/plugins/uptime/common/constants/alerts.ts

shahzad31 · 2021-06-18T15:04:35Z

x-pack/plugins/uptime/public/lib/alert_types/tls_legacy.tsx

+  id: CLIENT_ALERT_TYPES.TLS_LEGACY,
+  iconClass: 'uptimeApp',
+  documentationUrl(docLinks) {
+    return `${docLinks.ELASTIC_WEBSITE_URL}guide/en/uptime/${docLinks.DOC_LINK_VERSION}/uptime-alerting.html#_tls_alerts`;


Please update relevant docs as well especially for message , otherwise create a follow up issue for docs team.

How should we handle the deprecation of the other rule type in the docs?

shahzad31 · 2021-06-18T15:05:11Z

x-pack/plugins/uptime/public/lib/alert_types/translations.ts

@@ -8,14 +8,31 @@
 import { i18n } from '@kbn/i18n';

 export const TlsTranslations = {
+  defaultActionMessage: i18n.translate('xpack.uptime.alerts.tls.legacy.defaultActionMessage', {
+    defaultMessage: `Detected TLS certificate {commonName} from issuer {issuer} is expiring or becoming too old. Certificate {summary}


Message should be more accurate like we discussed offline

shahzad31 · 2021-06-18T15:09:18Z

x-pack/plugins/uptime/server/lib/alerts/tls.ts

+            )
+            .valueOf();
+          const alertInstance = alertInstanceFactory(
+            `${TLS.id}-${cert.common_name}-${cert.issuer}`


i don't think we need tls.id common_name and issue should be enough. tls.id will pollute ui where we are displaying list of instances in alert management ui

shahzad31 · 2021-06-18T15:13:24Z

x-pack/plugins/uptime/server/lib/alerts/tls.ts

+      const uniqueCerts = certs.reduce<Cert[]>((uniqueCertArray, currentCert) => {
+        if (!uniqueCertArray.find((cert) => cert.common_name === currentCert.common_name)) {
+          uniqueCertArray.push(currentCert);
+        }
+
+        return uniqueCertArray;
+      }, []);


i don't think we need this reduce, if you check getCerts function, es query is a collapse on tls.server.hash.sha256 which means it will only return unique certificates. common_name actually can be same, tls.server.hash.sha256 will always be different.

Thanks. I will use sha256 for the id then, as using common_name for the individual alert instance id was causing duplicate scheduling issues.

i would say keep common name at start of instance id and sha256 after that. sha256 isn't very helpful in identification.

maybe append common name + index or something?

As discussed offline the id will be common name + issuer + sha256

are we keeping this reduce? i think we don't need this.

@shahzad31 , you're right, my mistake.

…0-individual-cert-alerts

…b.com/dominiqueclarke/kibana into feature/101150-individual-cert-alerts

shahzad31 · 2021-06-22T12:12:14Z

@dominiqueclarke can we change the alert flyout , notify default for this alert, i think , by default we should set it to Every time alert is active, On Status change doesn't make sense for this Alert

On status change means, user will be only notified once, i think for this it make sense to remind them after interval.

dominiqueclarke · 2021-06-22T12:17:03Z

@dominiqueclarke can we change the alert flyout , notify default for this alert, i think , by default we should set it to Every time alert is active, On Status change doesn't make sense for this Alert

On status change means, user will be only notified once, i think for this it make sense to remind them after interval.

That means that you would get an action connector message each time the executor runs for the set interval, the default of which is 1 minute. That seems a bit excessive, if our users aren't updating the interval. Perhaps we should also update the interval.

shahzad31

Don't have any blocking concerns besides the extra reduce method, otherwise looks good.

Great Job !!

kibanamachine · 2021-06-22T21:15:46Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`uptime`	561	562	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`uptime`	20.3KB	22.0KB	+1.7KB

History

💚 Build #132812 succeeded 9a4615a
💚 Build #132398 succeeded 38fd1c0
💚 Build #132192 succeeded 52a0399
💔 Build #132138 failed f5e9d3f
💚 Build #131641 succeeded 96a6cc3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

) * refactor cert alerts from batched to individual * remove old translations * create new certificate alert rule type and transition old cert rule type to legacy * update translations * maintain legacy tls rule UI to support legacy rule editing * update translations * update TLS alert content, rule type id, and alert instance id schema * remove extraneous logic and format date content Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-23T00:59:33Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…103035) * refactor cert alerts from batched to individual * remove old translations * create new certificate alert rule type and transition old cert rule type to legacy * update translations * maintain legacy tls rule UI to support legacy rule editing * update translations * update TLS alert content, rule type id, and alert instance id schema * remove extraneous logic and format date content Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Dominique Clarke <doclarke71@gmail.com>

refactor cert alerts from batched to individual

243d238

dominiqueclarke requested a review from a team as a code owner June 14, 2021 21:07

botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jun 14, 2021

dominiqueclarke changed the title ~~refactor cert alerts from batched to individual~~ [Uptime] Refactor cert alerts from batched to individual Jun 14, 2021

dominiqueclarke added 4 commits June 14, 2021 17:21

remove old translations

068dded

create new certificate alert rule type and transition old cert rule t…

6220ef0

…ype to legacy

update translations

627e981

Merge branch 'master' of github.com:elastic/kibana into feature/10115…

081ac20

…0-individual-cert-alerts

dominiqueclarke added release_note:deprecation and removed release_note:breaking labels Jun 16, 2021

kibanamachine and others added 4 commits June 16, 2021 07:16

Merge branch 'master' into feature/101150-individual-cert-alerts

96a6cc3

maintain legacy tls rule UI to support legacy rule editing

8ac044c

Merge branch 'feature/101150-individual-cert-alerts' of https://githu…

f5e9d3f

…b.com/dominiqueclarke/kibana into feature/101150-individual-cert-alerts

update translations

52a0399

dominiqueclarke added release_note:enhancement and removed release_note:deprecation labels Jun 17, 2021

Merge branch 'master' into feature/101150-individual-cert-alerts

38fd1c0

shahzad31 reviewed Jun 18, 2021

View reviewed changes

x-pack/plugins/uptime/common/constants/alerts.ts Outdated Show resolved Hide resolved

shahzad31 reviewed Jun 18, 2021

View reviewed changes

dominiqueclarke added 3 commits June 21, 2021 13:40

update TLS alert content, rule type id, and alert instance id schema

ed635f1

Merge branch 'master' of github.com:elastic/kibana into feature/10115…

1567cd9

…0-individual-cert-alerts

Merge branch 'feature/101150-individual-cert-alerts' of https://githu…

9a4615a

…b.com/dominiqueclarke/kibana into feature/101150-individual-cert-alerts

shahzad31 approved these changes Jun 22, 2021

View reviewed changes

remove extraneous logic and format date content

cdc3cc8

dominiqueclarke merged commit 450abab into elastic:master Jun 23, 2021

dominiqueclarke deleted the feature/101150-individual-cert-alerts branch June 23, 2021 00:56

kibanamachine mentioned this pull request Jun 23, 2021

[7.x] [Uptime] Refactor cert alerts from batched to individual (#102138) #103035

Merged

dominiqueclarke mentioned this pull request Jun 28, 2021

[Uptime] Update TLS alert documentation to reflect current action variable image elastic/observability-docs#816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Refactor cert alerts from batched to individual #102138

[Uptime] Refactor cert alerts from batched to individual #102138

dominiqueclarke commented Jun 14, 2021 •

edited

Loading

elasticmachine commented Jun 14, 2021

dominiqueclarke commented Jun 16, 2021

dominiqueclarke commented Jun 18, 2021

shahzad31 Jun 18, 2021

dominiqueclarke Jun 21, 2021

shahzad31 Jun 18, 2021

shahzad31 Jun 18, 2021

shahzad31 Jun 18, 2021

dominiqueclarke Jun 18, 2021

shahzad31 Jun 18, 2021

shahzad31 Jun 18, 2021

dominiqueclarke Jun 21, 2021

shahzad31 Jun 22, 2021

dominiqueclarke Jun 22, 2021

shahzad31 commented Jun 22, 2021

dominiqueclarke commented Jun 22, 2021

shahzad31 left a comment

kibanamachine commented Jun 22, 2021

kibanamachine commented Jun 23, 2021

[Uptime] Refactor cert alerts from batched to individual #102138

[Uptime] Refactor cert alerts from batched to individual #102138

Conversation

dominiqueclarke commented Jun 14, 2021 • edited Loading

Author Checklist

Reviewer Checklist

For maintainers

elasticmachine commented Jun 14, 2021

dominiqueclarke commented Jun 16, 2021

dominiqueclarke commented Jun 18, 2021

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

shahzad31 commented Jun 22, 2021

dominiqueclarke commented Jun 22, 2021

shahzad31 left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 22, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Page load bundle

History

kibanamachine commented Jun 23, 2021

💚 Backport successful

dominiqueclarke commented Jun 14, 2021 •

edited

Loading