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

Fix Datasource testing connection don't validate endpoints with path #5663

Merged

Conversation

xinruiba
Copy link
Member

@xinruiba xinruiba commented Jan 5, 2024

Task List

Description

Test connection should only success when opensearch-client response status code is 200 and

  1. response body not empty (with serverless collection)
  2. response body have cluster_name field (with server cluster)
    Please check this ticket for more details [BUG] Cluster endpoint test connection passed unexpectly #5707

01/19/2024 Update:

Create an initial fix based on discussion, and more details can be found under this issue: #5707

01/09/2024 Update:

Discussed with team (checkout: #5663 (comment))

We are going to take a further investigation and we put the first proposal under the issue description:
#5656

CC @Flyingliuhub @seraphjiang @bandinib-amzn please feel free to leave comments~ Thanks

Issues Resolved

fixes #5656

Fixes #5702

Screenshot

Before:
Test connection with endpoint: "https://google.com" will pass which is unexpected

Before-TestGoogle

After:
Test connection with endpoint: "https://google.com" will be failed to connect with expected response message:

After-TestGoogle

Also verified endpoint with path name will fail test connection:

After-WithPath

Validate cluster endpoint still able to pass connection testing:
After-NoPath

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e83b7ee) 67.03% compared to head (77b9ce6) 67.03%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5663   +/-   ##
=======================================
  Coverage   67.03%   67.03%           
=======================================
  Files        3295     3296    +1     
  Lines       63329    63339   +10     
  Branches    10084    10087    +3     
=======================================
+ Hits        42451    42459    +8     
- Misses      18429    18430    +1     
- Partials     2449     2450    +1     
Flag Coverage Δ
Linux_1 35.23% <ø> (ø)
Linux_2 55.18% <ø> (ø)
Linux_3 43.90% <100.00%> (+<0.01%) ⬆️
Linux_4 35.33% <ø> (ø)
Windows_1 35.26% <ø> (ø)
Windows_2 55.15% <ø> (ø)
Windows_3 43.92% <100.00%> (+0.01%) ⬆️
Windows_4 35.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Flyingliuhub
Flyingliuhub previously approved these changes Jan 5, 2024
Signed-off-by: Xinrui Bai <[email protected]>
Flyingliuhub
Flyingliuhub previously approved these changes Jan 8, 2024
@bandinib-amzn
Copy link
Member

Also I think it would be great if we can run this fix by UX team to see if it is ok from users perspective.

@xinruiba
Copy link
Member Author

xinruiba commented Jan 8, 2024

Also I think it would be great if we can run this fix by UX team to see if it is ok from users perspective.

Thanks for the comment. I'm reaching out to our UX team for more inputs.

Signed-off-by: Xinrui Bai <[email protected]>
@seraphjiang
Copy link
Member

seraphjiang commented Jan 8, 2024

Also I think it would be great if we can run this fix by UX team to see if it is ok from users perspective.

@kgcreative who could help to work with us on this.

@kgcreative
Copy link
Member

kgcreative commented Jan 9, 2024

I'm happy to review async here. To that end, i have a few questions before i make a recommendation.

  1. When we say "OpenSearch Endpoint" we mean the cluster endpoint, and not an OSD endpoint, correct? If so, then "test connection" should handshake with the endpoint, and there should be some sort of response that validates that this is indeed the correct endpoint.

  2. when it comes to there being a path, is there a legitimate usecase for an endpoint having a path in it? for example: https://www.bank.com/opensearch and if so, how do we differentiate between that and say https://opensearch.bank.com

  3. I don't think we should disable the test connection button, instead, i think if there is an appended path, test connection should return an error, at which point the endpoint URL input should change to failed (red)

@Flyingliuhub
Copy link
Member

I'm happy to review async here. To that end, i have a few questions before i make a recommendation.

1. When we say "OpenSearch Endpoint" we mean the cluster endpoint, and not an OSD endpoint, correct? If so, then "test connection" should handshake with the endpoint, and there should be some sort of response that validates that this is indeed the correct endpoint.

2. when it comes to there being a path, is there a legitimate usecase for an endpoint having a path in it? for example: `https://www.bank.com/opensearch` and if so, how do we differentiate between that and say `https://opensearch.bank.com`

3. I don't think we should disable the `test connection` button, instead, i think if there is an appended path, `test connection` should return an error, at which point the endpoint URL input should change to failed (red)

Thanks, @kgcreative, for the great suggestions. I agree with you.
There are a couple of approaches which we can address this. @xinruiba and I will figure out more detail later on.

  1. Endpoint ULR pattern validation, which includes OpenSearch and Opensearch serverless, we need to have a good endpoint URL pattern validation pattern to validate user's input endpoint URL to quickly provide warning if doesn't match the endpoint pattern.
  2. Test connection validate the response, and see if the response match the OpenSearch/OpenSearch Serverless

@xinruiba
Copy link
Member Author

xinruiba commented Jan 9, 2024

I'm happy to review async here. To that end, i have a few questions before i make a recommendation.

  1. When we say "OpenSearch Endpoint" we mean the cluster endpoint, and not an OSD endpoint, correct? If so, then "test connection" should handshake with the endpoint, and there should be some sort of response that validates that this is indeed the correct endpoint.

Answer:
Yes, it's cluster endpoint.

  1. when it comes to there being a path, is there a legitimate usecase for an endpoint having a path in it? for example: https://www.bank.com/opensearch and if so, how do we differentiate between that and say https://opensearch.bank.com

Answer:
For now, we don't have a user case that supporting an endpoint having a path in it (@bandinib-amzn @Flyingliuhub rectify me if I'm wrong). In the future will we supported endpoint with path? I need to have more discussion with my team.

  1. I don't think we should disable the test connection button, instead, i think if there is an appended path, test connection should return an error, at which point the endpoint URL input should change to failed (red)

Answer:
Thanks for the comment. I think your proposed user experience is:

Before user clicking the "Test Connection" Button:

  1. User fill in the datasource creation form (with an invalidate cluster endpoint)
  2. Test connect button still available
  3. Endpoint URL input still green

After user clicking the "Test Connection" Button:

  1. Banner shows up to say connection failed
  2. Endpoint URL input becomes red

Is that correct?

For this proposal, me and @Flyingliuhub have a discussion and @Flyingliuhub already provide a response.
We will have a further discussion and keep you updated.

Thanks for the comment.

@kgcreative
Copy link
Member

I think the green underline is just a focus indicator, so when you tab out of the field it should return to it's unfocused state.

@bandinib-amzn
Copy link
Member

Failed cypress test is not related to this change, so we can ignore.

@Flyingliuhub Flyingliuhub merged commit b5d39b6 into opensearch-project:main Jan 23, 2024
70 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 23, 2024
…5663)

* fix Datasource testing connection don't validate endpoints with path #5656
Signed-off-by: Xinrui Bai <[email protected]>
(cherry picked from commit b5d39b6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
Flyingliuhub pushed a commit that referenced this pull request Jan 25, 2024
…5663) (#5727)

* fix Datasource testing connection don't validate endpoints with path #5656
Signed-off-by: Xinrui Bai <[email protected]>
(cherry picked from commit b5d39b6)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
yujin-emma pushed a commit to yujin-emma/OpenSearch-Dashboards that referenced this pull request Feb 5, 2024
…pensearch-project#5663)

* fix Datasource testing connection don't validate endpoints with path opensearch-project#5656
Signed-off-by: Xinrui Bai <[email protected]>
Signed-off-by: yujin-emma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants