[Uptime] Update TLS settings #64111

justinkambic · 2020-04-21T20:20:29Z

Summary

We had previously made changes that added some fields to the Settings page in the Uptime solution. The needs for these fields have changed, so this patch renames them. Additional refactoring went into the way the form components receive input and interact with our state store.

Testing

Navigate to the Uptime settings page and modify the values for each field. Make sure that the values you provide are persisted.

Also, log in as a user who has readonly access in the current space, and ensure they're unable to edit the values on the page. No changes were made to the API side of this feature, so you shouldn't need to check access restrictions, but you can test that as well to ensure it's working if you choose.

Refactoring summary

Constant values

Previously, we had some default values that were hosted in our runtime_types directory. I don't think this is the place to put those values, because we're not doing that anywhere else in the app, and we're referencing those values from many places. I moved them to a new file in the constants folder, and I think that will be a good place to centrally locate all our default settings values as that feature grows.

State management

The settings form had an optional field for its state in the store. Given that the server will return the default values discussed above, we should feel comfortable referencing the default values in our store's bootstrap code as well. I made that field required and referenced the default values. Doing this means we'll never not have values for the settings in practice, unless something strange is done to the saved objects outside the scope of this code.

Because we're guaranteeing settings values now, we can remove a lot of branching code in the settings page.

Additionally, I adopted the translation pattern we've discussed previously offline and extracted all the i18n references from the settings page and added them to a dedicated file that the other pages can eventually reference as well.

We're spending a lot of energy thinking and talking about shortening value names like dateRangeStart. Given that we're already nesting cert settings values in a dedicated object, I gave the values themselves short names.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-04-21T20:20:31Z

Pinging @elastic/uptime (Team:uptime)

andrewvc

Left a comment on the move from the optional settings? value in the store to settings. I haven't reviewed the rest yet. This is a meaty topic worthy of discussion.

andrewvc · 2020-04-22T00:44:05Z

x-pack/legacy/plugins/uptime/public/state/reducers/dynamic_settings.ts

  loadError?: Error;
  saveError?: Error;
  loading: boolean;
 }

 const initialState: DynamicSettingsState = {
+  settings: DYNAMIC_SETTINGS_DEFAULTS,


This feels cleaner for the settings page in some ways because it reduces LoC, but unless I'm missing something carries hidden complexity that will be paid during the construction of other features.

Before this change one could simply check !!settings to see if they had a reasonably recent value to use. Now, however, one must reason about the state of loadError, saveError and loading, otherwise you might display to the user the default value, which could be terribly incorrect. The settings page itself must reason about all these variables but there's no escaping the fact that the settings page's concern is the current value of these items and their state transitions. Paying that cost in the code of the settings component is expected. Other consumers of settings don't care about anything other than that a fairly recent value of settings was loaded. What they don't want, however, is a fake default value. That could do some damage.

Imagine we use settings on another page, the alert flyout for instance. Before this change we would only need to check !!settings. One might think that now one needs to check !loading instead, however, is that the right thing? What if a reload was triggered but the values haven't changed. Well, we don't know, now we have to subscribe to an update. What if the load fails? Now we also need to check loadError, so now we need to check !loading && !loadError. What if the load failed, but the settings values are still current enough? Do we now do !loading && !loadError && settings != DEFAULT_SETTINGS, which will tell us if it loaded at least once successfully, but only in the case where the user changed the settings? This would be an easy bug to miss in a code review, and it's more thinking. If later we change any of these variables for any reason then we need to hunt down this logic all over the app, or use a helper function. It'd be much easier to just use the optional settings?.

TL;DR Reasoning about the lifecycle of settings carefully on the settings page makes sense, that's that page's job. Everywhere else it should be as simple as possible. The settings object is truly an optional, we can't hide that.

Point taken about the risk of displaying incorrect values and the associated awfulness. I had initially thought about compromising by making all the settings values Partial, but we discussed this a bit offline and decided that someone could add a Type value in the future, which would open us up to the same problem. It's better to make this optional at the root and add branching code where it's necessary.

I think the true representation is an optional Partial type, where all the fields are also optional. We could conceivably have some valid fields and others invalid.

I'll make an edit and ping you.

I've made the settings field optional again.

shahzad31

Thank you for taking care of it, just few suggestion and it will be good to go.

shahzad31 · 2020-04-23T08:54:48Z

x-pack/legacy/plugins/uptime/common/constants/settings_defaults.ts

+export const DYNAMIC_SETTINGS_DEFAULTS: DynamicSettings = {
+  heartbeatIndices: 'heartbeat-8*',
+  certificatesThresholds: {
+    expiration: 7,


i think this should be 30 days be default.

Fixed in d0246a7.

shahzad31 · 2020-04-23T08:56:13Z

x-pack/legacy/plugins/uptime/common/runtime_types/dynamic_settings.ts

-export const CertificatesStatesThresholdType = t.interface({
-  warningState: t.number,
-  errorState: t.number,
+export const CertificatesStatesThresholdType = t.partial({


i think this shouldn't be partial.

would it make sense just to rename it CertStateThresholds WDYT?

shahzad31 · 2020-04-23T09:00:51Z

x-pack/legacy/plugins/uptime/public/components/settings/certificate_form.tsx

-              id="xpack.uptime.sourceConfiguration.stateThresholds"
-              defaultMessage="Expiration State Thresholds"
+              id="xpack.uptime.sourceConfiguration.expirationThreshold"
+              defaultMessage="Expiration/Age Thresholds"


we should also add that text here which got updated in ACs "Change the threshold for displaying and alerting on certificate errors. Note, this will affect any configured alerts"

I added this copy in 0ddd97b, modified the punctuation a bit in a1fdf46.

shahzad31 · 2020-04-23T09:03:35Z

x-pack/legacy/plugins/uptime/public/components/settings/indices_form.tsx

@@ -63,7 +62,7 @@ export const IndicesForm: React.FC<SettingsFormProps> = ({
              id="xpack.uptime.sourceConfiguration.heartbeatIndicesDefaultValue"
              defaultMessage="The default value is {defaultValue}"
              values={{
-                defaultValue: <EuiCode>{defaultDynamicSettings.heartbeatIndices}</EuiCode>,
+                defaultValue: <EuiCode>{dss.settings.heartbeatIndices}</EuiCode>,


This change will display the current value? default value should come from constant?

Right you are, we don't want that to change!

This is using defaults again now.

shahzad31 · 2020-04-23T09:05:02Z

x-pack/legacy/plugins/uptime/public/pages/settings.tsx

-    const errorStateErr = certificatesThresholds?.errorState ? null : blankStr;
-    const warningStateErr = certificatesThresholds?.warningState ? null : blankStr;
+    const errorStateErr = certificatesThresholds?.expiration ? null : blankStr;
+    const warningStateErr = certificatesThresholds?.age ? null : blankStr;


can you also rename var name?

I realized we needed to improve this, so I added a type and renamed the var usage in 59e60e8.

shahzad31 · 2020-04-23T09:06:42Z

x-pack/legacy/plugins/uptime/public/pages/settings.tsx

-    }
-  };
+  const onChangeFormField: OnFieldChangeType = changedField =>
+    setFormFields(Object.assign({ ...merge(formFields, changedField) }));


We are using merge bcoz Object.assign doesn't take of nested merging, right?

I've gotten rid of merge altogether because it isn't pure and it was editing the state var. I'm wondering if settings shouldn't just be a flat object to avoid recursive concerns when editing slices of it.

yes i think having a right data structure is important, i do agree, having nested have made a lot of complexity. if you think, it's worth an effort, go ahead.

I'll make a follow-up issue.

We have other more important stuff we need to review and it's not worth holding that up for more work in this patch. We don't have any further settings changes planned for the time being, so this complexity shouldn't increase any more than what it is right now until we can get to it. It should be a light patch.

Issue: elastic/uptime#175

shahzad31 · 2020-04-23T09:09:37Z

x-pack/legacy/plugins/uptime/public/state/reducers/dynamic_settings.ts

    [String(setDynamicSettings)]: state => ({
      ...state,
      loading: true,
    }),
    [String(setDynamicSettingsSuccess)]: (state, action: Action<DynamicSettings>) => ({
+      ...state,


it's not necessary to destructure prev state to return new state, if you know what new state can be in absolute terms.

++ I removed these additions in 8412951

I think initially I thought some state-related issue was happening because of the lack of spread on unreferenced state params, and then never removed them after the fact.

shahzad31 · 2020-04-23T09:10:47Z

x-pack/test/functional/services/uptime/settings.ts

-      const errorInput = await testSubjects.find('error-state-threshold-input-loaded', 5000);
-      const warningInput = await testSubjects.find('warning-state-threshold-input-loaded', 5000);
+      const errorInput = await testSubjects.find('expiration-threshold-input-loaded', 5000);
+      const warningInput = await testSubjects.find('age-threshold-input-loaded', 5000);


var name rename plz :)

Renamed in f7678b6

justinkambic · 2020-04-23T17:53:14Z

@shahzad31 I think I've addressed your feedback, should be ok for a second look when you have time.

shahzad31

LGTM !!

kibanamachine · 2020-04-24T15:48:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: a6b014b

History

💚 Build #42837 succeeded a1fdf46
💚 Build #42828 succeeded 8412951
💚 Build #42822 succeeded ef21286
💔 Build #42782 failed f7678b6
💚 Build #42267 succeeded db90149c27d54f9adba0cab8228f8245d982ae92

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

* Refactor settings form event handling and modify certs fields. * Fix/improve broken types/unit/integration/api tests. * Modify default expiration threshold. * Rename test vars. * Implement PR feedback. * Refresh snapshots, fix broken tests/types. * Remove unnecessary state spreading. * Add type for settings field errors. * Refresh test snapshots. * Improve punctuation.

…bana into ingest-node-pipeline/open-flyout-create-edit * 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (116 commits) [Ingest Node Pipelines] More lenient treatment of on-failure value (elastic#64411) Report Deletion via UI- functional test (elastic#64031) Bump handlebars dependency from 4.5.3 to 4.7.6 (elastic#64402) [Uptime] Update TLS settings (elastic#64111) [alerting] removes usage of any throughout Alerting Services code (elastic#64161) [CANVAS] Moves notify to a canvas service (elastic#63268) [Canvas] Misc NP Stuff (elastic#63703) update apm index pattern (elastic#64232) Task/hostlist pagination (elastic#63722) [NP] Vega migration (elastic#63849) Move ensureDefaultIndexPattern into data plugin (elastic#63100) [Fleet] Fix agent status count to not include unenrolled agents (elastic#64106) Migrate graph_workspace saved object registration to Kibana platform (elastic#64157) Index pattern management UI -> TypeScript and New Platform Ready (edit_index_pattern) (elastic#64184) [ML] EuiDataGrid ml/transform components. (elastic#63447) [ML] Moving to kibana capabilities (elastic#64057) Move input_control_vis into NP (elastic#63333) remove reference to local application service in graph (elastic#64288) KQL removes leading zero and breaks query (elastic#62748) [FieldFormats] Cleanup: rename IFieldFormatType -> FieldFormatInstanceType (elastic#64193) ...

* [Uptime] Update TLS settings (#64111) * Refactor settings form event handling and modify certs fields. * Fix/improve broken types/unit/integration/api tests. * Modify default expiration threshold. * Rename test vars. * Implement PR feedback. * Refresh snapshots, fix broken tests/types. * Remove unnecessary state spreading. * Add type for settings field errors. * Refresh test snapshots. * Improve punctuation. * Change default index pattern from 8.x to 7.x value. Co-authored-by: Elastic Machine <[email protected]>

justinkambic · 2020-04-27T18:12:41Z

Backported to:

7.x/7.8.0 0214129
#64469

justinkambic added release_note:enhancement v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.8.0 labels Apr 21, 2020

justinkambic requested review from andrewvc and shahzad31 April 21, 2020 20:20

justinkambic self-assigned this Apr 21, 2020

justinkambic added 2 commits April 21, 2020 18:10

Refactor settings form event handling and modify certs fields.

674857e

Fix/improve broken types/unit/integration/api tests.

7d2bf4c

justinkambic force-pushed the uptime_tls-settings branch from db90149 to 7d2bf4c Compare April 21, 2020 22:11

andrewvc reviewed Apr 22, 2020

View reviewed changes

shahzad31 requested changes Apr 23, 2020

View reviewed changes

justinkambic added 9 commits April 23, 2020 10:23

Merge branch 'master' into uptime_tls-settings

caf3ef2

Modify default expiration threshold.

d0246a7

Rename test vars.

f7678b6

Implement PR feedback.

0ddd97b

Refresh snapshots, fix broken tests/types.

ef21286

Remove unnecessary state spreading.

8412951

Add type for settings field errors.

59e60e8

Refresh test snapshots.

d47d429

Improve punctuation.

a1fdf46

Merge branch 'master' into uptime_tls-settings

a6b014b

shahzad31 approved these changes Apr 24, 2020

View reviewed changes

justinkambic merged commit e918c43 into elastic:master Apr 24, 2020

justinkambic mentioned this pull request Apr 24, 2020

[7.x] [Uptime] Update TLS settings (#64111) #64469

Merged

justinkambic deleted the uptime_tls-settings branch April 27, 2020 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Update TLS settings #64111

[Uptime] Update TLS settings #64111

justinkambic commented Apr 21, 2020 •

edited

Loading

elasticmachine commented Apr 21, 2020

andrewvc left a comment

andrewvc Apr 22, 2020 •

edited

Loading

justinkambic Apr 22, 2020

justinkambic Apr 23, 2020

shahzad31 left a comment

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

shahzad31 Apr 23, 2020

justinkambic Apr 23, 2020

justinkambic commented Apr 23, 2020

shahzad31 left a comment

kibanamachine commented Apr 24, 2020

justinkambic commented Apr 27, 2020

[Uptime] Update TLS settings #64111

[Uptime] Update TLS settings #64111

Conversation

justinkambic commented Apr 21, 2020 • edited Loading

Summary

Testing

Refactoring summary

Constant values

State management

Checklist

For maintainers

elasticmachine commented Apr 21, 2020

andrewvc left a comment

Choose a reason for hiding this comment

andrewvc Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzad31 left a 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

justinkambic commented Apr 23, 2020

shahzad31 left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 24, 2020

💚 Build Succeeded

History

justinkambic commented Apr 27, 2020

justinkambic commented Apr 21, 2020 •

edited

Loading

andrewvc Apr 22, 2020 •

edited

Loading