fix(s3toss): reject invalid config entries#35404
Conversation
Return parse errors for malformed ssAccessString options, out-of-range dataDir levels, and invalid legacy s3endpoint values instead of allowing index or slice panics. Fixes taosdata#35403 Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request improves input validation and error handling in tools/s3toss/main.go when parsing configuration options such as data directory levels, S3 endpoints, and S3 access strings. It also introduces a comprehensive suite of unit tests in tools/s3toss/main_test.go to verify these changes. The review feedback suggests refactoring the resetConfig() helper in the test file to reset the global config fields individually rather than duplicating the anonymous struct definition, which avoids future compilation failures if the struct definition changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func resetConfig() { | ||
| config = struct { | ||
| BlockSize int64 | ||
| DNode uint | ||
| DataDirs [3][]string | ||
| Endpoint string | ||
| Secure bool | ||
| AccessKey string | ||
| SecretKey string | ||
| Bucket string | ||
| Region string | ||
| }{} | ||
| } |
There was a problem hiding this comment.
The resetConfig() function duplicates the entire anonymous struct definition of config from main.go. This duplication is a maintainability hazard: if the config struct in main.go is modified in the future (e.g., adding or removing a field), this test file will fail to compile because the two anonymous struct types will no longer be identical and assignable.
To improve maintainability, we should reset the fields of the global config variable individually. This avoids duplicating the struct definition and ensures the tests remain compatible even if the struct fields change.
func resetConfig() {
config.BlockSize = 0
config.DNode = 0
config.DataDirs = [3][]string{}
config.Endpoint = ""
config.Secure = false
config.AccessKey = ""
config.SecretKey = ""
config.Bucket = ""
config.Region = ""
}There was a problem hiding this comment.
Addressed in 37afe2f by resetting each config field directly instead of assigning a duplicated anonymous struct literal. Re-ran cd tools/s3toss && go test ./... successfully (ok github.com/taosdata/s3toss 0.003s).
Avoid duplicating the anonymous config struct type in tests when resetting global parser state. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
This reverts commit 3de45f2.
This reverts commit e388c64.
Description
s3tossnow fails selected malformedtaos.cfgentries with explicit parse errors instead of allowing malformed local configuration to reach unchecked parser operations.What Problem This Solves
s3tossis a local shared-storage maintenance tool. Its user-visible entry point is ataos.cfgpath supplied with-taoscfg;parseTaosCfg()runs before MinIO client initialization and before any migration I/O.On the existing CLI path, malformed legacy configuration was reachable for
dataDirands3endpoint:For this shape,
parseTaosCfg()used the parsed level directly as an index intoconfig.DataDirs, whose supported tiers are0..2. That boundary matches TDengine's storage contract: the multi-tier storage docs definedataDirlevel values as0,1, and2,include/util/tdef.hsetsTFS_MAX_TIERSto3, and core TFS validation rejects levels below0or greater than or equal toTFS_MAX_TIERSinsource/libs/tfs/src/tfs.c.For the legacy endpoint form, the parser treated any non-
http://value as HTTPS and sliced the string as if that prefix existed. Longer malformed values could be truncated incorrectly, and shorter values could panic.The
ssAccessStringpath is narrower:The unchecked
parseAccessString()helper could readpart[1]afterstrings.SplitN(item, "=", 2), but the old CLI path did not actually enter thessAccessStringbranch becauseparseTaosCfg()lowercased the key and then matched it against mixed-casecase "ssAccessString". This PR connects that intended branch asssaccessstringand returns an explicit parse error before malformedkey=valueoptions can be consumed.Changes
The production change is limited to
tools/s3tossconfiguration parsing:Focused unit tests cover valid S3 access strings, malformed access-string options, out-of-range
dataDirlevels, invalid legacy endpoints, and the realtaos.cfgparsing path.The PR does not change migration behavior, MinIO client setup, credential handling, object naming, existing
protocolhandling, or which unknown well-formed S3 optionss3tossignores. Non-S3ssAccessStringvalues remain ignored as before.Evidence
The focused tests exercise both the helper-level parser and the user-visible config-file path:
Expected error fragments are covered by tests:
Local validation was run with Go 1.24.2:
GitHub checks: no checks are currently reported for this branch.
Possible call chain / impact
This affects only invalid local configuration inputs in the
s3tosstool. It is not a network request path, and it does not change core TDengine TFS parsing.Issue(s)
Checklist
Please check the items in the checklist if applicable.