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

[Remote clusters] Cloud deployment form when adding new cluster #93953

Closed
wants to merge 3 commits into from

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Mar 8, 2021

Summary

This PR adds a new flyout that helps configure a new remote cluster when on Cloud. The flyout has an input for the Elasticsearch endpoint url copied from the deployment page on Cloud. The url is then parsed and fields proxy address and server name are pre-filled in the main form.

Screenshots

Screenshot

Screenshot 2021-03-08 at 16 35 27

Gif

Mar-08-2021.16-39-57.mp4

How to test

When running Kibana locally add following values to kibana.yml file to simulate Cloud environment

xpack.cloud.id: 'remote-deployment:dXMtY2VudHJhbDEuZ2NwLmZvdW5kaXQubm8kODU0NDAwYjA4OTJhNGFhMzk2M2ZhZTg0MzRiOGRhODEkZDVmZTQzZTFmZWM3NDljM2JhNDgyYTNlNTY5YmRjMTc='
xpack.cloud.deploymentUrl: 'https://staging.found.no/deployments/d74e0a97f269463ca2ec05dcbe703662'

Release Note

When adding a remote cluster on Cloud, a new flyout with Cloud-specific instructions now helps pre-filling proxy connection values.

@yuliacech yuliacech added release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 v8.0.0 labels Mar 8, 2021
@yuliacech yuliacech marked this pull request as ready for review March 8, 2021 15:52
@yuliacech yuliacech requested a review from a team as a code owner March 8, 2021 15:52
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@yuliacech yuliacech requested review from a team and zanbel March 8, 2021 15:56
@myasonik
Copy link
Contributor

myasonik commented Mar 8, 2021

Any chance you can proactively add a11y tests to this? (Docs)

Will save you the trouble of having to go back and do this when QA goes through it later and makes a follow up ticket for you but you've forgotten all the context around your work. (Meta with many examples)

@yuliacech yuliacech requested a review from juliocamarero March 8, 2021 16:27
@ryankeairns
Copy link
Contributor

@yuliacech the Cloud team is actively working on adding several URLs to the kibana.yml. afaik, this does not specifically include the ES API URL, but you should soon be able to direct users to deployments.

@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 8, 2021

💔 Build Failed

Failed CI Steps


Test Failures

Kibana Pipeline / jest / Jest Tests.x-pack/plugins/remote_clusters/public/application/sections/components/remote_cluster_form.RemoteClusterForm renders untouched state

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `RemoteClusterForm renders untouched state 1`

- Snapshot  -  0
+ Received  + 18

@@ -89,10 +89,28 @@
          >
            <h2
              class="euiTitle euiTitle--small euiTitle euiTitle--xsmall euiDescribedFormGroup__title"
            >
              Connection mode
+             <button
+               class="euiButtonEmpty euiButtonEmpty--primary euiButtonEmpty--small"
+               type="button"
+             >
+               <span
+                 class="euiButtonContent euiButtonEmpty__content"
+               >
+                 <span
+                   class="euiButtonContent__icon"
+                   data-euiicon-type="logoCloud"
+                 />
+                 <span
+                   class="euiButtonEmpty__text"
+                 >
+                   Add Cloud deployment
+                 </span>
+               </span>
+             </button>
            </h2>
            <div
              class="euiText euiText--small euiDescribedFormGroup__description"
            >
              <div
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/remote_clusters/public/application/sections/components/remote_cluster_form/remote_cluster_form.test.js:21:23)
    at Promise.then.completed (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:276:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:216:10)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:40)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

Kibana Pipeline / jest / Jest Tests.x-pack/plugins/remote_clusters/public/application/sections/components/remote_cluster_form.RemoteClusterForm proxy mode renders correct connection settings when user enables proxy mode

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches


Stack Trace

Error: expect(received).toMatchSnapshot()

Snapshot name: `RemoteClusterForm proxy mode renders correct connection settings when user enables proxy mode 1`

- Snapshot  -  0
+ Received  + 61

@@ -386,10 +386,21 @@
                <FormattedMessage
                  defaultMessage="Connection mode"
                  id="xpack.remoteClusters.remoteClusterForm.sectionModeTitle"
                  values={Object {}}
                />
+               <EuiButtonEmpty
+                 iconType="logoCloud"
+                 onClick={[Function]}
+                 size="s"
+               >
+                 <FormattedMessage
+                   defaultMessage="Add Cloud deployment"
+                   id="xpack.remoteClusters.remoteClusterForm.cloudFormButton"
+                   values={Object {}}
+                 />
+               </EuiButtonEmpty>
              </h2>
            </EuiTitle>
          }
        >
          <div
@@ -422,10 +433,60 @@
                            id="xpack.remoteClusters.remoteClusterForm.sectionModeTitle"
                            values={Object {}}
                          >
                            Connection mode
                          </FormattedMessage>
+                         <EuiButtonEmpty
+                           iconType="logoCloud"
+                           onClick={[Function]}
+                           size="s"
+                         >
+                           <button
+                             className="euiButtonEmpty euiButtonEmpty--primary euiButtonEmpty--small"
+                             disabled={false}
+                             onClick={[Function]}
+                             type="button"
+                           >
+                             <EuiButtonContent
+                               className="euiButtonEmpty__content"
+                               iconSide="left"
+                               iconType="logoCloud"
+                               textProps={
+                                 Object {
+                                   "className": "euiButtonEmpty__text",
+                                 }
+                               }
+                             >
+                               <span
+                                 className="euiButtonContent euiButtonEmpty__content"
+                               >
+                                 <EuiIcon
+                                   className="euiButtonContent__icon"
+                                   size="m"
+                                   type="logoCloud"
+                                 >
+                                   <span
+                                     className="euiButtonContent__icon"
+                                     data-euiicon-type="logoCloud"
+                                     size="m"
+                                   />
+                                 </EuiIcon>
+                                 <span
+                                   className="euiButtonEmpty__text"
+                                 >
+                                   <FormattedMessage
+                                     defaultMessage="Add Cloud deployment"
+                                     id="xpack.remoteClusters.remoteClusterForm.cloudFormButton"
+                                     values={Object {}}
+                                   >
+                                     Add Cloud deployment
+                                   </FormattedMessage>
+                                 </span>
+                               </span>
+                             </EuiButtonContent>
+                           </button>
+                         </EuiButtonEmpty>
                        </h2>
                      </EuiTitle>
                    </EuiTitle>
                    <EuiText
                      className="euiDescribedFormGroup__description"
    at Object.<anonymous> (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/plugins/remote_clusters/public/application/sections/components/remote_cluster_form/remote_cluster_form.test.js:30:25)
    at Promise.then.completed (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:276:28)
    at new Promise (<anonymous>)
    at callAsyncCircusFn (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/utils.js:216:10)
    at _callCircusTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:212:40)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at _runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:149:3)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:63:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at _runTestsForDescribeBlock (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:57:9)
    at run (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/run.js:25:3)
    at runAndTransformResultsToJestFormat (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapterInit.js:176:21)
    at jestAdapter (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-circus/build/legacy-code-todo-rewrite/jestAdapter.js:109:19)
    at runTestInternal (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:380:16)
    at runTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/runTest.js:472:34)
    at Object.worker (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-runner/build/testWorker.js:133:12)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
remoteClusters 131 135 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
remoteClusters 176.0KB 185.0KB +9.0KB

Page load bundle

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

id before after diff
remoteClusters 15.6KB 16.0KB +337.0B
Unknown metric groups

miscellaneous assets size

id before after diff
remoteClusters 0.0B 40.2KB +40.2KB

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

@mdefazio
Copy link
Contributor

mdefazio commented Mar 8, 2021

@yuliacech Here is some quick feedback.

I'm wondering if we need a flyout for this since it is simply a single input? If we think this is the primary way users will enter this info, then let's default to this for the form. And then have a switch to Manually enter proxy which would then show the proxy form. (Side note, maybe it's a cloud specific thing, but the description of the connection mode seemed opposite of what was presented by default—see screenshot)

If we need the flyout (and use a smaller sized flyout), I would suggest then using a button that is placed above the proxy input . I'm not sure if the current position of the link is visible enough as an action for the user.
Perhaps:
'Add cloud deployment'
-or-
Proxy Form

General thoughts on the flyout:
image

Some thoughts on this form in general:
image

I am happy to put together a quick wireframe if this feedback isn't clear.

Copy link

@zanbel zanbel left a comment

Choose a reason for hiding this comment

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

Thanks for opening the ER!

A couple of comments:

  • You used the Elastic Cloud logo while this should be relevant for ECE as well. Not sure if you want to display a different log depending on the platform or just go with text, in which case Elastic Cloud can fit both

image

  • With the new flyout the information presented in the info message is confusing since we already guide users to copy-paste ES URL and show the note in case they use an alias

image

  • I think the screenshot can be refined, specifically the size and red triangle

image

Will add comments about the text in the relevant parts

Copy link

@zanbel zanbel left a comment

Choose a reason for hiding this comment

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

Added a few text suggestions.

<p>
<FormattedMessage
id="xpack.remoteClusters.cloudDeploymentForm.formDescription"
defaultMessage="If you're connecting to a Cloud deployment, you can copy and paste the Elasticsearch
Copy link

Choose a reason for hiding this comment

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

We know this is cloud deployment (we don't support any other option) so I think the text can be more instructive.

Text suggestion: "Copy and paste the Elasticsearch endpoint URL of the remote deployment to automatically configure the remote cluster. The URL can be found on the remote deployment overview page."

<EuiFlexItem>
<FormattedMessage
id="xpack.remoteClusters.cloudDeploymentForm.formTitle"
defaultMessage=" Add Cloud deployment as remote cluster"
Copy link

Choose a reason for hiding this comment

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

I think it should be "Add Cloud deployment as a remote cluster", no?

Copy link

Choose a reason for hiding this comment

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

Also, should it say "Elastic Cloud"?

</strong>
<FormattedMessage
id="xpack.remoteClusters.cloudDeploymentForm.aliasNoteDescription"
defaultMessage="If you configured a deployment alias in Elastic Cloud,
Copy link

Choose a reason for hiding this comment

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

"If you configured a deployment alias, either in Elastic Cloud, in your reverse proxy or load-balancer, you will need to copy-paste the Proxy address and Server name or the remote deployment in the remote cluster form directly. These values can be copied from the Remote parameters section on the remote deployment Security page. More information is available here".

As you mentioned, we can't point to the relevant security page so I think we need to only keep the link to our user guide.

@shubhaat are we going to call this feature deployment alias or configurable deployment endpoints?

Copy link

@shubhaat shubhaat Mar 9, 2021

Choose a reason for hiding this comment

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

On the UI it is called an "Custom endpoint alias", we did not use the "deployment alias" terminology since at the time this was designed we still had the CCS template that used the "deployment alias" term

Screen Shot 2021-03-09 at 7 58 41 AM

I suggest using "Custom endpoint alias" instead for consistency.

Copy link

Choose a reason for hiding this comment

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

Thanks @shubhaat !

Heads up @yuliacech @cjcenizal et al that when using the alias in the URL and changing the alias things will break. That's expected since changing the alias requires changing clients, integrations, etc. Just want to raise it in the context of a need to handle what happens when we can't connect to the cluster during and after creation.

One of the benefits of using a UUID is that it never changes.

@yuliacech
Copy link
Contributor Author

@yuliacech the Cloud team is actively working on adding several URLs to the kibana.yml. afaik, this does not specifically include the ES API URL, but you should soon be able to direct users to deployments.

Thanks for this info @ryankeairns! I was using cloudDeploymentUrl provided by the x-pack cloud plugin to link to the Cloud deployment Security page, but then realized that we can't know which other Cloud deployment the user want to configure as a remote cluster.

@yuliacech
Copy link
Contributor Author

Thanks a lot both for your great feedback, @mdefazio and @zanbel! I'd like to explore the idea of incorporating the cloud url input directly into the form.

I'm wondering if we can force 'proxy mode' on Cloud to avoid having too many toggles (seed nodes vs proxy mode and cloud url vs address + server name). @juliocamarero Could you please weigh in on that?

@juliocamarero
Copy link
Member

I'm wondering if we can force 'proxy mode' on Cloud to avoid having too many toggles (seed nodes vs proxy mode and cloud url vs address + server name). @juliocamarero Could you please weigh in on that?

I think this is a good idea. We don't support any other mode other than the Proxy Mode on Cloud, therefore I think hiding and enforcing this mode will be a UX improvement.

@mdefazio
Copy link
Contributor

mdefazio commented Mar 9, 2021

@yuliacech reposting the result of our convo on Slack here for visibility for the rest of the team.
image

@cjcenizal
Copy link
Contributor

@yuliacech What will the UX be for someone who's editing a remote cluster that was originally configured to connect to a Cloud deployment?

@yuliacech
Copy link
Contributor Author

@yuliacech What will the UX be for someone who's editing a remote cluster that was originally configured to connect to a Cloud deployment?

That's a great question, @cjcenizal! My assumption for editing a remote cluster on Cloud is that if proxyAddress contains Cloud default port 9400 and equals (without the port) to serverName, then we can default to input mode with 1 input for cloud url and to 'manual' mode with 2 inputs otherwise. When defaulting to cloud url form, we can populate the input with this value (without the port).

I think what can be confusing for the user, is that when creating a cluster they type in something similar to https://my-cloud-url.com:1234 and when editing they see my-cloud-url.com. For that I'm thinking of using prepend and append labels for the input:
Screenshot 2021-03-09 at 18 34 27

@cjcenizal
Copy link
Contributor

@yuliacech If I understand you correctly, it sounds like you're describing a rule like this: "If the proxyAddress's port is 9400 and its domain name is the same as serverName, then we can assume it's a Cloud deployment." Is that right?

If so, have you verified with the ES engineers that this rule is an invariant? It'd be confusing if someone were able to configure an on-prem remote cluster that incidentally satisfies these criteria, and we end up showing them the wrong UI. :)

@yuliacech
Copy link
Contributor Author

@cjcenizal, yes that is the rule, but we only apply it when on Cloud similar to the 'add new cluster' view. So that the form looks the same for the user for both when adding and editing the cluster. When adding a cluster we default to the cloud url form and when editing we try to guess if they used the simple form to begin with. Does it make sense? :)

@cjcenizal
Copy link
Contributor

cjcenizal commented Mar 9, 2021

@yuliacech What throws me off is that clusters on Cloud, ECE, and on-prem can all connect to one another as remote clusters. So when the user is logged into a Cloud deployment, there is no guarantee that the remote clusters they're configuring will be on Cloud as well. So even if we only apply this rule when the user is logged into a Cloud deployment, it seems like there will still be edge cases where this rule could be incorrectly applied.

when editing we try to guess if they used the simple form to begin with

Based on the above, this guess-based approach for editing seems prone to the undesirable UX I originally described.

@yuliacech
Copy link
Contributor Author

Hi @cjcenizal, I understand your concerns, but I didn't think clusters can connect with each other from different platforms. I discussed with @juliocamarero and he confirmed that only same environment clusters are supported right now for CCR/CCS (Cloud, ECE, on-prem). The default 9400 port is also not likely to change anytime soon.
I believe currently it's safe to assume that on Cloud only other Cloud deployments are being configured as remote clusters. WDYT?

@cjcenizal
Copy link
Contributor

cjcenizal commented Mar 10, 2021

Thanks for clarifying @yuliacech! I'm not sure how I got the mistaken impression that remote clusters from different platforms were supported. @zanbel Was this something we discussed?

Anyway, if only Cloud deployments are being connected to on Cloud, then my concerns aren't valid. Thanks for helping me clear that up. This leads me to another question though. Why do we need the rule you described? If all remote clusters will belong to the same platform that the user's Kibana instance is on, then we can we use the Cloud plugin to determine the platform the user is on, and therefore the platform that all remote clusters are on?

@yuliacech
Copy link
Contributor Author

@cjcenizal Thanks for asking such great questions! This definitely helped me learn a lot about Cloud :)
So the user still might have a custom endpoint alias, a load balancer or a reverse proxy, for that they would toggle the 'manual mode' and input a proxy address and a server name. Since those values can be arbitrary and can't be 'converted back' to a singular Cloud url value, when editing such a cluster, the form defaults back to 'manual mode'.

@cjcenizal
Copy link
Contributor

OK, thanks @yuliacech! Could you please update the PR description with the information we've covered? This will document the edge cases and intended behavior which will help us test the PR's UX, verify that the code communicates intentions clearly, and if we ever need to do some forensic analysis it will give us a record of our decision-making to help us spot mistakes. The info I'm looking for is something like:

  • Invariants, such as "Clusters on a platform can only connect to remote clusters on the same platform"
  • Description of various remote cluster types, such as those using a custom endpoint aliases
  • A truth table that maps each remote cluster type to the appropriate "Create" and "Edit" flows

@yuliacech
Copy link
Contributor Author

Closing this PR in favor of #94450 that implements the in-form solution.

@yuliacech yuliacech closed this Mar 11, 2021
@yuliacech yuliacech deleted the remote_clusters_cloud branch April 22, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants