-
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] Fix handling of remote clusters with deprecated proxy setting #61126
[Remote clusters] Fix handling of remote clusters with deprecated proxy setting #61126
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
/cc @juliocamarero |
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.
Tested locally, code LGTM!
Just had a question about this:
The server_name field does not exist in the v1 proxy implementation. The UI attempts to resolve this by creating a server name using the proxy address, minus the port.
The UI marks this field as optional. Is this a mistake? Or, if it is optional, do we need to create one per the note above?
@cjcenizal Thanks for the review!
This was a suggestion from the cloud team. I followed-up with them again regarding it. It is technically optional via the ES cluster settings API, but it is required for the cloud proxy to work correctly. WDYT? Also, I realized that there will be the same bug when deleting a remote cluster with the v1 proxy implementation, so I pushed a commit that resolves this as well and updated the PR description. |
@alisonelizabeth As a short-term solution I think we should look at this part of the UI through the eyes of a Cloud user and remove the "Optional" note from the label. Otherwise what we're telling them won't reflect the underlying model. Ideally we can use the Cloud plugin to determine if we're on Cloud or not, and conditionally render the "Optional" note (and conditionally validate that the user has provided a name). If this is too much work for FF, then I think we can just remove it for now. I think we should also ask Cloud if there's a way to solve this on their side (either by deriving a name just-in-time the way we're doing here, or by using some other identifier). If they can do this then we can reduce complexity in this UI by removing the above conditions and just making the field optional. |
@cjcenizal makes sense 👍. Also, just to clarify, the derived name would only be used in the edit flow when the cluster was created pre-7.6 and used the deprecated proxy setting. When creating a new remote cluster, this field would be blank and it would be up to the user to define. |
This reverts commit ac7857a.
@cjcenizal I'm going to leave the implementation as is for now. I think this will be a less-traveled path, and it would be helpful to get user feedback before deciding to make any further changes. I've chatted with @juliocamarero about this and he is OK with the current direction for now. |
@alisonelizabeth Sounds good! Thanks. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This PR fixes two issues that came up while testing remote clusters with proxy mode on cloud (implemented via #59221).
proxy
setting must be set tonull
onPUT _cluster/settings
.server_name
field does not exist in the v1 proxy implementation. The UI attempts to resolve this by creating a server name using the proxy address, minus the port.How to test
Example:
Insert after L20:
Insert after L29:
Server name
input has a value.proxy
field is set tonull
.