Skip to content

fix(s3toss): reject invalid config entries#35404

Open
VectorPeak wants to merge 6 commits into
taosdata:mainfrom
VectorPeak:fix-s3toss-config-parse
Open

fix(s3toss): reject invalid config entries#35404
VectorPeak wants to merge 6 commits into
taosdata:mainfrom
VectorPeak:fix-s3toss-config-parse

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jul 16, 2026

Copy link
Copy Markdown

Description

s3toss now fails selected malformed taos.cfg entries with explicit parse errors instead of allowing malformed local configuration to reach unchecked parser operations.

What Problem This Solves

s3toss is a local shared-storage maintenance tool. Its user-visible entry point is a taos.cfg path 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 dataDir and s3endpoint:

dataDir /tmp/taos 3

For this shape, parseTaosCfg() used the parsed level directly as an index into config.DataDirs, whose supported tiers are 0..2. That boundary matches TDengine's storage contract: the multi-tier storage docs define dataDir level values as 0, 1, and 2, include/util/tdef.h sets TFS_MAX_TIERS to 3, and core TFS validation rejects levels below 0 or greater than or equal to TFS_MAX_TIERS in source/libs/tfs/src/tfs.c.

s3endpoint localhost:9000

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 ssAccessString path is narrower:

ssAccessString s3:endpoint

The unchecked parseAccessString() helper could read part[1] after strings.SplitN(item, "=", 2), but the old CLI path did not actually enter the ssAccessString branch because parseTaosCfg() lowercased the key and then matched it against mixed-case case "ssAccessString". This PR connects that intended branch as ssaccessstring and returns an explicit parse error before malformed key=value options can be consumed.

Changes

The production change is limited to tools/s3toss configuration parsing:

+if item == "" {
+    continue
+}
 part := strings.SplitN(item, "=", 2)
+if len(part) != 2 {
+    return fmt.Errorf("invalid ssAccessString option %q: expected key=value", item)
+}
@@
+if lvl < 0 || lvl >= len(config.DataDirs) {
+    return fmt.Errorf("invalid dataDir level %d: must be between 0 and %d", lvl, len(config.DataDirs)-1)
+}
 config.DataDirs[lvl] = append(config.DataDirs[lvl], parts[1])
@@
-case "ssAccessString":
+case "ssaccessstring":
@@
+} else if strings.HasPrefix(endpoint, "https://") {
+    config.Secure = true
+    config.Endpoint = parts[1][8:]
 } else {
+    return fmt.Errorf("invalid s3endpoint %q: expected http:// or https:// prefix", parts[1])
 }

Focused unit tests cover valid S3 access strings, malformed access-string options, out-of-range dataDir levels, invalid legacy endpoints, and the real taos.cfg parsing path.

The PR does not change migration behavior, MinIO client setup, credential handling, object naming, existing protocol handling, or which unknown well-formed S3 options s3toss ignores. Non-S3 ssAccessString values remain ignored as before.

Evidence

The focused tests exercise both the helper-level parser and the user-visible config-file path:

ssAccessString s3:endpoint=s3.amazonaws.com;bucket=mybucket;protocol=http;accessKeyId=AK;secretAccessKey=SK;region=us-east-2
ssAccessString s3:endpoint
dataDir /tmp/taos 3
s3endpoint localhost:9000

Expected error fragments are covered by tests:

expected key=value
invalid dataDir level 3
expected http:// or https:// prefix

Local validation was run with Go 1.24.2:

cd tools/s3toss && go test -count=1 ./...
ok   github.com/taosdata/s3toss 0.005s

GitHub checks: no checks are currently reported for this branch.

Possible call chain / impact

administrator or automation runs s3toss with -taoscfg
  -> main()
  -> parseTaosCfg()
  -> dataDir / s3endpoint / intended ssAccessString branch
  -> previous unchecked index/slice/helper parsing, now explicit parse error
  -> createMinIOClient() and migration I/O only run after parsing succeeds

This affects only invalid local configuration inputs in the s3toss tool. 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.

  • Is the user manual updated?
  • Are the test cases passed and automated?
  • Is there no significant decrease in test coverage?

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>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread tools/s3toss/main_test.go
Comment on lines +115 to +127
func resetConfig() {
config = struct {
BlockSize int64
DNode uint
DataDirs [3][]string
Endpoint string
Secure bool
AccessKey string
SecretKey string
Bucket string
Region string
}{}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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 = ""
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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).

VectorPeak and others added 5 commits July 16, 2026 11:55
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

s3toss can panic on malformed configuration entries

1 participant