-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Add support for proxy mode #59221
[Remote clusters] Add support for proxy mode #59221
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
…e_clusters_proxy_mode
…e_clusters_proxy_mode
…e_clusters_proxy_mode
…e_clusters_proxy_mode
…e_clusters_proxy_mode
@gchaps do you think you could review the copy in the screenshots I included in the PR description? I also will need to work with you to update the remote clusters documentation. At a minimum, the screenshot should be updated. |
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.
This looks fantastic, Alison! I tested locally and everything works as expected. Code looks impeccable. Great job!
export function deserializeCluster(name: string, esClusterObject: any): any { | ||
import { PROXY_MODE } from '../constants'; | ||
|
||
export interface ClusterEs { |
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.
Thanks for adding these types! 😬
} = this.state; | ||
|
||
let modeSettings; |
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.
Great decision here, to allow for caching of the user's input if they flip between modes.
<FormattedMessage | ||
id="xpack.remoteClusters.remoteClusterForm.sectionSeedsDescription1" | ||
defaultMessage="A list of remote cluster nodes to query for the cluster state. | ||
Specify multiple seed nodes so discovery doesn't fail if a node is unavailable." |
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 feel like we lose a bit of helpful information here. Can we move some of this content into the help text below the "Seed nodes" input, changing it to: "An IP address or host name, followed by the transport port of the remote cluster. Specify multiple seed nodes so discovery doesn't fail if a node is unavailable."
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.
Good point!
<h2> | ||
<FormattedMessage | ||
id="xpack.remoteClusters.remoteClusterForm.sectionModeTitle" | ||
defaultMessage="Connection mode 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.
I'll defer to @gchaps if she disagrees, but I think this can be simplified to just "Connection mode", since technically everything in this form is a setting.
// If the cluster hasn't been stored in the cluster state, then it's defined by the | ||
// node's config file. | ||
const isConfiguredByNode = !isTransient && !isPersistent; | ||
|
||
// Pre-7.6, ES supported an undocumented "proxy" field | ||
// ES does not handle migrating this to the new implementation, so we need to surface it in the UI | ||
// This value is not available via the GET /_remote/info API, so we get it from the cluster 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.
Thanks for these helpful comments.
skipUnavailable: schema.boolean(), | ||
mode: schema.oneOf([schema.literal(PROXY_MODE), schema.literal(SNIFF_MODE)]), | ||
seeds: schema.nullable(schema.arrayOf(schema.string())), | ||
nodeConnections: schema.nullable(schema.number()), |
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.
Did we miss this in the original NP migration? I'm going to make a mental note to pay extra attention to this when I'm doing my own migration work because it seems super-easy to overlook a missed validation during testing. It seems the only way for us to discover a missed validation is with thorough functional or manual testing, right?
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 actually added support for defining node connections as part of this PR. It was always possible via the ES API, but we were previously only allowing users to define seeds
in the UI (screenshot).
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.
Thanks, I missed that!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
@gchaps I'm going to go ahead and merge this PR, but I can address any copy feedback in a follow-up PR. |
Summary
Fixes #58927
Items to note:
server_name
and deprecatedproxy
field are not available via the GET /_remote/info API, so we need to access them via cluster settings. I only checkpersistent
settings, based on a previous conversation here: [CCR] Remote clusters settings sources #26067 (comment). I tried to document this in the code, but please let me know if there are any questions or if it is unclear.sniff
(default) andproxy
modes have different settings. To simplify the table view, I generalized the column names, e.g., "Addresses" maps toproxy_address
for "proxy" mode andseeds
for "sniff" mode. On the details panel, however, I use the more specific description, e.g., "Proxy address" and "Seeds".Release note
The Remote Clusters UI added support for enabling "proxy" mode when creating or editing a remote cluster. This feature aligns with Elasticsearch's support for remote cluster connection modes that was added in
7.6
.How to test
You will need to set up two clusters in order to test remote clusters. For example:
Test cases:
Create a remote cluster using the default mode
Create a remote cluster with the "proxy" mode enabled
Verify detail panel for both clusters
Verify edit and delete actions
Modify the responses in get_route.ts to simulate a cluster with deprecated settings, and verify icon displays in table and callout appears on the detail panel and edit route.
Screenshots
Proxy mode enabled
Deprecation warnings