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

feat(webex-core): hostmap interceptor #3789

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

Coread
Copy link
Collaborator

@Coread Coread commented Aug 21, 2024

COMPLETES SPARK-555022

This pull request addresses

The hostmap from u2c maps the logical name of the service (these are actually FQDNs) to the hosts. Until recently, the first host listed for each logical name was the same as the logical name. Going forward this will not be the case. Services will continue to use logical names i.e. conversations/locus and the client needs to replace these with the correct host.

Existing methods in webex-core/services do not handle this (e.g. getServiceFromUrl)

by making the following changes

I've created a new method replaceHostFromHostmap which looks up the host in the hostmap and replaces it in a supplied URI. I've used this new method in an interceptor, so it will automatically happen for each request.

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios where tested

I've tested this in integration and production. Currently, the hostnames are the same so there is no effect, but I verified that the code was running through the new interceptor and selecting the host.

I certified that

  • I have read and followed contributing guidelines

  • I discussed changes with code owners prior to submitting this pull request

  • I have not skipped any automated checks

  • All existing and new tests passed

  • I have updated the documentation accordingly


Make sure to have followed the contributing guidelines before submitting.

@Coread Coread requested review from a team as code owners August 21, 2024 13:24
@Coread Coread added the validated If the pull request is validated for automation. label Aug 21, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-3789.d3m3l2kee0btzx.amplifyapp.com

Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

@Coread can you please fill in the manual tests you've done in the PR description test section? It would be good to see the real-world implication of this interceptor.

I see that we're replacing the hostname with the first instance that we find in the hostmap. Just for my understanding, what happens when the request fails to the first hostname and we need to use the second entry that's found in the hostmap? Do we plan on adding any logic for this at a later time?

@Coread
Copy link
Collaborator Author

Coread commented Aug 29, 2024

@Coread can you please fill in the manual tests you've done in the PR description test section? It would be good to see the real-world implication of this interceptor.

I see that we're replacing the hostname with the first instance that we find in the hostmap. Just for my understanding, what happens when the request fails to the first hostname and we need to use the second entry that's found in the hostmap? Do we plan on adding any logic for this at a later time?

This is logic that already exists in the desktop apps and has done for many years, the original intention of which was to provide high availability. The U2C hostmap will always contain at least one entry in the hosts section. All the entries will have the same host, but with different ids. See the below screenshot. i.e. the high availability feature of the catalog was never completed, so there is no point in using it like that, no point in checking other hosts.

Screenshot 2024-08-29 185036

@Coread Coread merged commit 6a34e63 into webex:next Sep 10, 2024
10 of 11 checks passed
pagour98 pushed a commit to pagour98/webex-js-sdk that referenced this pull request Sep 27, 2024
pagour98 pushed a commit to pagour98/webex-js-sdk that referenced this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
validated If the pull request is validated for automation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants