Skip to content

[CELEBORN-1553] Using the request base url as swagger server#2674

Closed
turboFei wants to merge 3 commits intoapache:mainfrom
turboFei:swagger_server
Closed

[CELEBORN-1553] Using the request base url as swagger server#2674
turboFei wants to merge 3 commits intoapache:mainfrom
turboFei:swagger_server

Conversation

@turboFei
Copy link
Member

@turboFei turboFei commented Aug 9, 2024

What changes were proposed in this pull request?

Using the request base url as swagger server, to prevent the swagger server not reachable and CORS error if the swagger server urls do not match.

Currently, if the http host is bound to local, the swagger server is not reachable.
For example:

celeborn.master.http.host=0.0.0.0
celeborn.worker.http.host=0.0.0.0

Why are the changes needed?

image

Does this PR introduce any user-facing change?

No, just use the request base url as swagger server.

How was this patch tested?

Integration testing:
image

image

@turboFei turboFei changed the title Expose external connection url for swagger servers [CELEBORN-1553] Expose external connection url for swagger servers if local host bound Aug 9, 2024
@codecov
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 33.35%. Comparing base (ea6617c) to head (e4eddab).
Report is 17 commits behind head on main.

Files Patch % Lines
...rver/common/http/api/CelebornOpenApiResource.scala 0.00% 3 Missing ⚠️
...rg/apache/celeborn/server/common/HttpService.scala 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2674      +/-   ##
==========================================
- Coverage   39.83%   33.35%   -6.47%     
==========================================
  Files         239      310      +71     
  Lines       15026    18193    +3167     
  Branches     1362     1672     +310     
==========================================
+ Hits         5984     6067      +83     
- Misses       8711    11786    +3075     
- Partials      331      340       +9     

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

@turboFei turboFei changed the title [CELEBORN-1553] Expose external connection url for swagger servers if local host bound [CELEBORN-1553] Using the request base url as swagger server Aug 10, 2024
@turboFei
Copy link
Member Author

cc @pan3793

@turboFei turboFei marked this pull request as draft August 10, 2024 03:13
@turboFei turboFei marked this pull request as ready for review August 12, 2024 18:31
@turboFei turboFei requested a review from pan3793 August 12, 2024 18:31
@turboFei
Copy link
Member Author

also cc @cfmcgrady @SteNicholas

Copy link
Contributor

@RexXiong RexXiong left a comment

Choose a reason for hiding this comment

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

LGTM

@turboFei
Copy link
Member Author

Hi @RexXiong

If no more comments, can we merge it? thanks

@RexXiong
Copy link
Contributor

RexXiong commented Sep 3, 2024

Hi @RexXiong

If no more comments, can we merge it? thanks

Thanks for the reminder. Merge to main(v0.6.0)

@RexXiong RexXiong closed this in 48910bb Sep 3, 2024
s0nskar pushed a commit to s0nskar/celeborn that referenced this pull request Sep 16, 2024
### What changes were proposed in this pull request?

Using the request base url as swagger server, to prevent the swagger server not reachable and `CORS` error if the swagger server urls do not match.

Currently, if the http host is bound to local, the swagger server is not reachable.
For example:
```
celeborn.master.http.host=0.0.0.0
celeborn.worker.http.host=0.0.0.0
```

### Why are the changes needed?
![image](https://github.com/user-attachments/assets/3553fb94-c937-4877-bd55-3df0c121bd01)

### Does this PR introduce _any_ user-facing change?

No, just use the request base url as swagger server.

### How was this patch tested?
Integration testing:
<img width="1483" alt="image" src="https://github.com/user-attachments/assets/f8465bcb-a266-4532-9f11-52e9374c56a5">

<img width="1423" alt="image" src="https://github.com/user-attachments/assets/84eacd86-7ba3-4f14-907b-6d529c6e0e28">

Closes apache#2674 from turboFei/swagger_server.

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
wankunde pushed a commit to wankunde/celeborn that referenced this pull request Oct 11, 2024
### What changes were proposed in this pull request?

Using the request base url as swagger server, to prevent the swagger server not reachable and `CORS` error if the swagger server urls do not match.

Currently, if the http host is bound to local, the swagger server is not reachable.
For example:
```
celeborn.master.http.host=0.0.0.0
celeborn.worker.http.host=0.0.0.0
```

### Why are the changes needed?
![image](https://github.com/user-attachments/assets/3553fb94-c937-4877-bd55-3df0c121bd01)

### Does this PR introduce _any_ user-facing change?

No, just use the request base url as swagger server.

### How was this patch tested?
Integration testing:
<img width="1483" alt="image" src="https://github.com/user-attachments/assets/f8465bcb-a266-4532-9f11-52e9374c56a5">

<img width="1423" alt="image" src="https://github.com/user-attachments/assets/84eacd86-7ba3-4f14-907b-6d529c6e0e28">

Closes apache#2674 from turboFei/swagger_server.

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants