mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2025-08-19 17:01:12 +00:00
[v12.0/forgejo] fix: minio initialization can freeze indefinitely if misconfigured (#8914)
Some checks failed
testing-integration / test-sqlite (push) Has been skipped
testing-integration / test-unit (push) Has been skipped
testing / frontend-checks (push) Has been skipped
testing / backend-checks (push) Has been skipped
testing / test-e2e (push) Has been skipped
testing / test-unit (push) Has been skipped
testing / test-pgsql (push) Has been skipped
testing / test-mysql (push) Has been skipped
testing / test-remote-cacher (redis) (push) Has been skipped
testing / test-sqlite (push) Has been skipped
testing / test-remote-cacher (garnet) (push) Has been skipped
testing / test-remote-cacher (valkey) (push) Has been skipped
testing / test-remote-cacher (redict) (push) Has been skipped
testing / security-check (push) Has been skipped
/ release (push) Has been cancelled
Some checks failed
testing-integration / test-sqlite (push) Has been skipped
testing-integration / test-unit (push) Has been skipped
testing / frontend-checks (push) Has been skipped
testing / backend-checks (push) Has been skipped
testing / test-e2e (push) Has been skipped
testing / test-unit (push) Has been skipped
testing / test-pgsql (push) Has been skipped
testing / test-mysql (push) Has been skipped
testing / test-remote-cacher (redis) (push) Has been skipped
testing / test-sqlite (push) Has been skipped
testing / test-remote-cacher (garnet) (push) Has been skipped
testing / test-remote-cacher (valkey) (push) Has been skipped
testing / test-remote-cacher (redict) (push) Has been skipped
testing / security-check (push) Has been skipped
/ release (push) Has been cancelled
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/8897 Fixes #8893 by using a 30 second initialization timeout on the minio storage. Manually tested by configuring `MINIO_ENDPOINT=100.64.123.123`... ``` 2025/08/14 11:29:29 ...s/storage/storage.go:157:initAttachments() [I] Initialising Attachment storage with type: minio 2025/08/14 11:29:29 ...les/storage/minio.go💯NewMinioStorage() [I] Creating Minio storage at 100.64.123.123:mfenniak-forgejo with base path attachments/ 2025/08/14 11:29:59 routers/init.go:63:mustInit() [F] forgejo.org/modules/storage.Init failed: Get "http://100.64.123.123/mfenniak-forgejo/?versioning=": context deadline exceeded ``` ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/<pull request number>.md` to be be used for the release notes instead of the title. Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/8914 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org> Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
This commit is contained in:
parent
14a7e6a5ad
commit
2941adfd11
2 changed files with 48 additions and 3 deletions
|
@ -76,8 +76,13 @@ var getBucketVersioning = func(ctx context.Context, minioClient *minio.Client, b
|
|||
return err
|
||||
}
|
||||
|
||||
var initializationTimeout = 30 * time.Second
|
||||
|
||||
// NewMinioStorage returns a minio storage
|
||||
func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage, error) {
|
||||
initCtx, cancel := context.WithTimeout(ctx, initializationTimeout)
|
||||
defer cancel()
|
||||
|
||||
config := cfg.MinioConfig
|
||||
if config.ChecksumAlgorithm != "" && config.ChecksumAlgorithm != "default" && config.ChecksumAlgorithm != "md5" {
|
||||
return nil, fmt.Errorf("invalid minio checksum algorithm: %s", config.ChecksumAlgorithm)
|
||||
|
@ -112,7 +117,7 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage,
|
|||
// Otherwise even if the request itself fails (403, 404, etc), the code should still continue because the parameters seem "good" enough.
|
||||
// Keep in mind that GetBucketVersioning requires "owner" to really succeed, so it can't be used to check the existence.
|
||||
// Not using "BucketExists (HeadBucket)" because it doesn't include detailed failure reasons.
|
||||
err = getBucketVersioning(ctx, minioClient, config.Bucket)
|
||||
err = getBucketVersioning(initCtx, minioClient, config.Bucket)
|
||||
if err != nil {
|
||||
errResp, ok := err.(minio.ErrorResponse)
|
||||
if !ok {
|
||||
|
@ -125,13 +130,13 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage,
|
|||
}
|
||||
|
||||
// Check to see if we already own this bucket
|
||||
exists, err := minioClient.BucketExists(ctx, config.Bucket)
|
||||
exists, err := minioClient.BucketExists(initCtx, config.Bucket)
|
||||
if err != nil {
|
||||
return nil, convertMinioErr(err)
|
||||
}
|
||||
|
||||
if !exists {
|
||||
if err := minioClient.MakeBucket(ctx, config.Bucket, minio.MakeBucketOptions{
|
||||
if err := minioClient.MakeBucket(initCtx, config.Bucket, minio.MakeBucketOptions{
|
||||
Region: config.Location,
|
||||
}); err != nil {
|
||||
return nil, convertMinioErr(err)
|
||||
|
|
|
@ -9,8 +9,10 @@ import (
|
|||
"net/http/httptest"
|
||||
"os"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"forgejo.org/modules/setting"
|
||||
"forgejo.org/modules/test"
|
||||
|
||||
"github.com/minio/minio-go/v7"
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
@ -217,3 +219,41 @@ func TestMinioCredentials(t *testing.T) {
|
|||
})
|
||||
})
|
||||
}
|
||||
|
||||
func TestNewMinioStorageInitializationTimeout(t *testing.T) {
|
||||
defer test.MockVariableValue(&getBucketVersioning, func(ctx context.Context, minioClient *minio.Client, bucket string) error {
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return ctx.Err()
|
||||
case <-time.After(1 * time.Millisecond):
|
||||
return minio.ErrorResponse{
|
||||
StatusCode: http.StatusBadRequest,
|
||||
Code: "TestError",
|
||||
Message: "Mocked error for testing",
|
||||
}
|
||||
}
|
||||
})()
|
||||
|
||||
settings := &setting.Storage{
|
||||
MinioConfig: setting.MinioStorageConfig{
|
||||
Endpoint: "localhost",
|
||||
AccessKeyID: "123456",
|
||||
SecretAccessKey: "12345678",
|
||||
Bucket: "bucket",
|
||||
Location: "us-east-1",
|
||||
},
|
||||
}
|
||||
|
||||
// Verify that we reach `getBucketVersioning` and return the error from our mock.
|
||||
storage, err := NewMinioStorage(t.Context(), settings)
|
||||
require.ErrorContains(t, err, "Mocked error for testing")
|
||||
assert.Nil(t, storage)
|
||||
|
||||
defer test.MockVariableValue(&initializationTimeout, 1*time.Nanosecond)()
|
||||
|
||||
// Now that the timeout is super low, verify that we get a context deadline exceeded error from our mock.
|
||||
storage, err = NewMinioStorage(t.Context(), settings)
|
||||
require.Error(t, err)
|
||||
require.ErrorIs(t, err, context.DeadlineExceeded, "err must be a context deadline exceeded error, but was %v", err)
|
||||
assert.Nil(t, storage)
|
||||
}
|
||||
|
|
Loading…
Add table
Add a link
Reference in a new issue