objstore: support use native OSS sdk to access#65610
objstore: support use native OSS sdk to access#65610ti-chi-bot[bot] merged 15 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. |
D3Hunter
changed the title
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
This PR adds support for using the native Alibaba Cloud OSS SDK to access OSS storage, as an alternative to the generic S3-compatible approach.
Changes:
- Implements native OSS storage client with credential management and region detection
- Updates storage routing logic to direct OSS URIs to the new native implementation
- Adds comprehensive test coverage for OSS-specific functionality
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/objstore/storage.go | Routes OSS provider to new native OSS storage implementation |
| pkg/objstore/s3like/store.go | Refactors CopyFrom to support OSS wrapper through interface checking |
| pkg/objstore/parse.go | Adds OSS scheme parsing to backend configuration |
| pkg/objstore/ossstore/store_test.go | Comprehensive test suite for OSS storage operations |
| pkg/objstore/ossstore/store.go | Core OSS storage implementation with credential management |
| pkg/objstore/ossstore/BUILD.bazel | Build configuration updates for new OSS storage module |
| pkg/objstore/BUILD.bazel | Adds ossstore dependency to objstore package |
| qs.Bucket, qs.Region, detectedRegion)) | ||
| } | ||
|
|
||
| log.Info("succeed to get bucket region", zap.String("region", detectedRegion)) |
There was a problem hiding this comment.
The error message "succeeded" should be "succeeded" in the log message. While grammatically "succeed" is correct as an infinitive, in this context it should be "succeeded" to maintain consistency with past tense logging conventions for completed actions.
| log.Info("succeed to get bucket region", zap.String("region", detectedRegion)) | |
| log.Info("succeeded to get bucket region", zap.String("region", detectedRegion)) |
| // TODO changing the input backend is a side effect, it shouldn't be part of | ||
| // the NewXXX, but we have to do it here to keep compatibility now. | ||
| // | ||
| // OSS credential through assume role need refresh periodically, if we do | ||
| // send them out to TiKV, they also need to be refreshed, not sure how this | ||
| // works for BR now, we can add it later. |
There was a problem hiding this comment.
The TODO comment on lines 69-74 suggests that sending OSS credentials to TiKV is not currently supported but may need to be added later. However, the code on line 76 immediately returns an error when SendCredentials is true, making this a hard restriction. Consider adding more context to the TODO about when/why this might be needed, or if this is a permanent limitation that should be documented differently. This will help future maintainers understand whether this is a temporary workaround or a design decision.
| // TODO changing the input backend is a side effect, it shouldn't be part of | |
| // the NewXXX, but we have to do it here to keep compatibility now. | |
| // | |
| // OSS credential through assume role need refresh periodically, if we do | |
| // send them out to TiKV, they also need to be refreshed, not sure how this | |
| // works for BR now, we can add it later. | |
| // NOTE: changing the input backend is a side effect, it shouldn't be part of | |
| // the NewXXX, but we have to do it here to keep compatibility with existing | |
| // callers that expect backend.AccessKey / SecretAccessKey to be cleared. | |
| // | |
| // OSS credentials obtained via AssumeRole need to be refreshed periodically. | |
| // If we were to send credentials to TiKV, TiKV would also need a mechanism | |
| // to refresh or re-fetch them before expiry. BR / TiKV do not currently have | |
| // a protocol for propagating refreshed OSS credentials, so sending them | |
| // would either be unsafe (risk of using expired credentials) or require a | |
| // larger design change. | |
| // | |
| // As a result, sending OSS credentials to TiKV is intentionally disabled | |
| // here. If we ever add support for this, it must come with a well-defined | |
| // credential refresh / rotation mechanism and should be documented as such. |
| GetBucketPrefix() storeapi.BucketPrefix | ||
| }) | ||
| if !ok { | ||
| return errors.Annotatef(berrors.ErrStorageInvalidConfig, "CopyFrom is only supported by S3 storage, get %T", inStore) |
There was a problem hiding this comment.
The CopyFrom method now uses a duck-typed interface check instead of a concrete type assertion. While this works, the error message still says "CopyFrom is only supported by S3 storage" which is now misleading since it also supports OSS storage. The error message should be updated to reflect that it supports S3-like storage implementations.
| return errors.Annotatef(berrors.ErrStorageInvalidConfig, "CopyFrom is only supported by S3 storage, get %T", inStore) | |
| return errors.Annotatef(berrors.ErrStorageInvalidConfig, "CopyFrom is only supported by S3-like storage implementations, got %T", inStore) |
| credRefresher = newCredentialRefresher(provider, log.L().With( | ||
| zap.String("bucket", qs.GetBucket()), | ||
| zap.String("prefix", qs.GetPrefix()), | ||
| )) | ||
| if err := credRefresher.refreshOnce(); err != nil { | ||
| return nil, errors.Annotatef(err, "failed to get initial OSS credentials") | ||
| } | ||
| ossCfg = ossCfg.WithCredentialsProvider(credRefresher) | ||
| } | ||
|
|
||
| if opts.AccessRecording != nil { | ||
| ossOptFns = append(ossOptFns, func(o *oss.Options) { | ||
| o.ResponseHandlers = append(o.ResponseHandlers, func(resp *http.Response) error { | ||
| // nolint:bodyclose | ||
| opts.AccessRecording.RecRequest(resp.Request) | ||
| return nil | ||
| }) | ||
| }) | ||
| } | ||
|
|
||
| // get bucket location or check the specified region is correct | ||
| getLocCfg := &(*ossCfg) | ||
| if qs.Region == "" { | ||
| getLocCfg = getLocCfg.WithRegion(defaultRegion) | ||
| } else { | ||
| getLocCfg = getLocCfg.WithRegion(qs.Region) | ||
| } | ||
| ossCli := oss.NewClient(getLocCfg, ossOptFns...) | ||
| resp, err := ossCli.GetBucketLocation(ctx, &oss.GetBucketLocationRequest{Bucket: oss.Ptr(qs.Bucket)}) | ||
| if err != nil { | ||
| return nil, errors.Annotatef(err, "failed to get location of bucket %s", qs.Bucket) | ||
| } | ||
|
|
||
| detectedRegion := trimOSSRegionID(tea.StringValue(resp.LocationConstraint)) | ||
| if qs.Region != "" && detectedRegion != qs.Region { | ||
| return nil, errors.Trace(fmt.Errorf("bucket and region are not matched, bucket=%s, input region=%s, real region=%s", | ||
| qs.Bucket, qs.Region, detectedRegion)) | ||
| } | ||
|
|
||
| log.Info("succeed to get bucket region", zap.String("region", detectedRegion)) | ||
|
|
||
| qs.Prefix = storeapi.NewPrefix(qs.Prefix).String() | ||
| bucketPrefix := storeapi.NewBucketPrefix(qs.Bucket, qs.Prefix) | ||
| ossCfg = ossCfg.WithRegion(detectedRegion) | ||
|
|
||
| cli := &client{ | ||
| svc: oss.NewClient(ossCfg, ossOptFns...), | ||
| BucketPrefix: bucketPrefix, | ||
| options: &qs, | ||
| } | ||
| if err := s3like.CheckPermissions(ctx, cli, opts.CheckPermissions); err != nil { | ||
| return nil, errors.Annotatef(berrors.ErrStorageInvalidPermission, "check permission failed due to %v", err) | ||
| } | ||
|
|
||
| if credRefresher != nil { | ||
| if err = credRefresher.startRefresh(); err != nil { | ||
| return nil, errors.Annotatef(err, "failed to start OSS credential refresher") | ||
| } | ||
| } |
There was a problem hiding this comment.
The credential refresher may leak resources if the function returns an error after line 128 but before line 178. The refresher has already called refreshOnce() and initialized credentials, but if an error occurs in GetBucketLocation (line 152), region mismatch check (line 158-160), or CheckPermissions (line 174-175), the function returns without calling credRefresher.close(). Consider adding a defer statement after credRefresher creation to clean up on error, or ensure proper cleanup in all error paths.
There was a problem hiding this comment.
it's ok to skip close without starting the routine
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65610 +/- ##
================================================
- Coverage 77.8228% 77.6972% -0.1256%
================================================
Files 1989 1916 -73
Lines 543383 532702 -10681
================================================
- Hits 422876 413895 -8981
+ Misses 118848 118798 -50
+ Partials 1659 9 -1650
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
/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. |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/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 |
|
/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. |
|
/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. |
What problem does this PR solve?
Issue Number: close #65461
Problem Summary:
What changed and how does it work?
as title
Changes:
Check List
Tests
run the tests in
store_test.gomanually on real OSS, all passrun a small table import with OSS through role-arn
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.