oss: detect ECS region and use internal endpoint automaticaly#65687
oss: detect ECS region and use internal endpoint automaticaly#65687ti-chi-bot[bot] merged 8 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
moved to pkg/util/httputil/
| // return errors.Trace(err) | ||
| // } | ||
| // fmt.Println(resp.IP) | ||
| func GetJSON(ctx context.Context, client *http.Client, url string, v any) error { |
There was a problem hiding this comment.
there are 2 such method, they are merged into one in pkg/util/httputil
| require.False(t, common.IsDirExists("not-exists")) | ||
| } | ||
|
|
||
| func TestGetJSON(t *testing.T) { |
There was a problem hiding this comment.
moved to pkg/util/httputil
| // fmt.Println(resp.IP) | ||
| // | ||
| // nolint:unused | ||
| func GetJSON(client *http.Client, url string, v any) error { |
There was a problem hiding this comment.
Pull request overview
This PR implements automatic detection of ECS region and uses internal OSS endpoints to avoid traffic charges when running on Aliyun ECS instances. It also refactors HTTP utility code by moving it from br/pkg/httputil to pkg/util/httputil for better code organization and reusability.
Changes:
- Adds automatic ECS region detection using metadata service when using ECS RAM role credentials
- Implements logic to use internal OSS endpoints when ECS and bucket regions match
- Moves HTTP utility functions (NewClient, GetJSON) from br/pkg/httputil to pkg/util/httputil
- Adds GetText function to httputil package for fetching plain text responses
- Updates all import paths across the codebase to use the new httputil location
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/objstore/ossstore/store.go | Adds ECS region detection and internal endpoint logic; refactors logger initialization |
| pkg/objstore/ossstore/logger.go | Refactors newLogPrinter to accept logger directly with improved caller skip logic |
| pkg/objstore/ossstore/store_test.go | Adds tests for internal endpoint detection logic |
| pkg/objstore/ossstore/BUILD.bazel | Updates dependencies and shard count |
| pkg/util/httputil/http.go | New package with HTTP utility functions moved from br/pkg/httputil |
| pkg/util/httputil/http_test.go | Tests for HTTP utility functions |
| pkg/util/httputil/BUILD.bazel | Build configuration for new httputil package |
| pkg/util/util.go | Removes GetJSON function (moved to httputil) |
| pkg/lightning/common/util.go | Removes GetJSON function (moved to httputil) |
| pkg/lightning/common/util_test.go | Removes GetJSON test (moved to httputil) |
| pkg/lightning/common/security.go | Updates import path to use new httputil location |
| pkg/lightning/common/BUILD.bazel | Updates dependencies and shard count |
| br/pkg/httputil/http.go | Deleted - functionality moved to pkg/util/httputil |
| br/pkg/httputil/BUILD.bazel | Deleted - package moved |
| br/pkg/task/*.go | Updates import paths to use new httputil location |
| br/pkg/stream/*.go | Updates import paths to use new httputil location |
| br/pkg/pdutil/*.go | Updates import paths to use new httputil location |
| br/tests/br_key_locked/*.go | Updates import paths to use new httputil location |
Comments suppressed due to low confidence (1)
pkg/objstore/ossstore/store.go:112
- This TODO comment is now outdated since this PR implements the automatic internal endpoint detection. The TODO should be removed or updated to reflect that the feature has been implemented.
// TODO OSS charges for traffic, consider auto use internal endpoint when
// not specified explicitly and the bucket is in the same region with the
// client.
| func GetText(client *http.Client, url string) (string, error) { | ||
| body, err := doGet(context.Background(), client, url) |
There was a problem hiding this comment.
The GetText function doesn't accept a context parameter, unlike GetJSON. This is inconsistent and prevents callers from controlling timeouts or cancellation for GetText operations. Consider adding a context parameter for consistency and to allow proper timeout/cancellation control.
| func GetText(client *http.Client, url string) (string, error) { | |
| body, err := doGet(context.Background(), client, url) | |
| func GetText(ctx context.Context, client *http.Client, url string) (string, error) { | |
| body, err := doGet(ctx, client, url) |
| func TestGetText(t *testing.T) { | ||
| // Mock success response | ||
| handle := func(res http.ResponseWriter, _ *http.Request) { | ||
| res.WriteHeader(http.StatusOK) | ||
| _, err := res.Write([]byte("test-content")) | ||
| require.NoError(t, err) | ||
| } | ||
| testServer := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { | ||
| handle(res, req) | ||
| })) | ||
| defer testServer.Close() | ||
|
|
||
| client := &http.Client{} | ||
| text, err := GetText(client, testServer.URL+"/test") | ||
| require.NoError(t, err) | ||
| require.Equal(t, "test-content", text) | ||
| } |
There was a problem hiding this comment.
The TestGetText test only covers the success case. It should also test error cases similar to TestGetJSON, such as connection errors and non-200 status codes, to ensure comprehensive test coverage.
There was a problem hiding this comment.
that part of code is common, GetJSON can cover it
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/objstore/ossstore/store.go:112
- This TODO comment should be removed or updated since the feature it describes is now implemented in this PR. The code below already detects the ECS region and automatically uses the internal endpoint when the bucket is in the same region as the ECS instance.
// TODO OSS charges for traffic, consider auto use internal endpoint when
// not specified explicitly and the bucket is in the same region with the
// client.
| func GetText(client *http.Client, url string) (string, error) { | ||
| body, err := doGet(context.Background(), client, url) |
There was a problem hiding this comment.
The GetText function should accept a context parameter for proper cancellation support. The function currently uses context.Background() internally, which prevents callers from being able to cancel or timeout the HTTP request. This is inconsistent with GetJSON which accepts a context parameter. Consider changing the signature to accept a context parameter similar to GetJSON.
| func GetText(client *http.Client, url string) (string, error) { | |
| body, err := doGet(context.Background(), client, url) | |
| func GetText(ctx context.Context, client *http.Client, url string) (string, error) { | |
| body, err := doGet(ctx, client, url) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65687 +/- ##
================================================
+ Coverage 77.7971% 79.5799% +1.7827%
================================================
Files 1992 1942 -50
Lines 544075 531894 -12181
================================================
+ Hits 423275 423281 +6
+ Misses 119141 107153 -11988
+ Partials 1659 1460 -199
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, joechenrh, Leavrth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: ref #65461
Problem Summary:
What changed and how does it work?
unlike s3 which is free for traffic when enable gateway endpoint on the console, it's transparent to the client, OSS must explicitly use its internal endpoint on the client side to avoid charged of traffic
this pr detect the ECS region when possible, and use internal endpoint automatically
also move and refactor the API to make http requests inside br and pkg/util
Check List
Tests
when run
TestInternalEndpointlocally, public endpoint is usedrun on aliyun ECS which has bind a RAM Role, internal endpoint is used.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.