fix: treat JSON parsing errors as fatal errors

[coryflucas]

Causes them to fail fast and provide better error messages. Fixes #552

Description

Why is this change being made?

1. Corrects an issue with error handling that leads to error messages that are misleading and hard to diagnose.

What is changing?

1. Treat errors handling JSON parsing/processing of secrets as fatal so they bubble up to the pod's status rather than the generic error.

Related Links

• JMES Path results in "Failed to fetch secrets" #552

---

Testing

How was this tested?

1. Ran updated automated tests
2. I spun up an EKS cluster, deployed the changes and created a SecretProviderClass with a jmesPath pointing to a nonexistent key (NONEXISTENT_KEY) and verified the pod's status showed this error:

3. Using the same pod, changed the jmesPath have path: "[username", verified the pods status showed this error:

MountVolume.SetUp failed for volume "secrets" : rpc error: code = Unknown desc = failed to mount secrets store objects for pod default/dev-bad-jmes-pod, err: rpc error: code = Unknown desc = Invalid JMESPath "[username" for object alias "bad-value" in secret "dev-json-secret": SyntaxError: Expected tComma, received: tEOF

When testing locally, provide testing artifact(s):

---

Reviewee Checklist

Update the checklist after submitting the PR

• I have reviewed, tested and understand all changes
  If not, why:
• I have filled out the Description and Testing sections above
  If not, why:
• Build and Unit tests are passing
  If not, why:
• Unit test coverage check is passing
  If not, why:
• Integration tests pass locally
  If not, why:
• I have updated integration tests (if needed)
  If not, why:
• I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
  If not, why:
• I have added explanatory comments for complex logic, new classes/methods and new tests
  If not, why:
• I have updated README/documentation (if needed)
  If not, why:
• I have clearly called out breaking changes (if any)
  If not, why:

---

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

• Reviewee checklist has been accurately filled out
• Code changes align with stated purpose in description
• Test coverage adequately validates the changes

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[secrets-manager-ai-code-review]

Claude finished @simonmarty's task in 3m 58s —— View job

---

Code Review — PR #629: treat JSON parsing errors as fatal

• Read PR diff
• Review error-handling flow (IsFatalError usage)
• Review secret_value.go changes
• Review test coverage
• Post review feedback

Overall this is a clean, well-targeted fix. Introducing a typed JSONProcessingError and teaching IsFatalError to treat it as fatal is the right approach — JSON/JMESPath failures are deterministic config errors, so failing fast and surfacing a precise message to the pod status is a real improvement over the generic "Failed to fetch from all regions." The error messages are much more actionable, and test coverage (unit + server-level for both Secrets Manager and Parameter Store) is solid. 👍

Below are a few things worth considering.

---

1. Behavioral change in the failover path (main consideration)

Previously, a JSON/JMESPath error inside fetchSecretManagerValueWithClient / fetchParameterStoreBatch was non-fatal, so the region loop logged a klog.Warning and continued to the next client. Now it returns immediately:

provider/secrets_manager_provider.go:82 and provider/parameter_store_provider.go:86

if utils.IsFatalError(err) {
    return nil, err   // now short-circuits on the FIRST region's JMES error
}

For a genuinely invalid JMESPath expression this is exactly what we want — every region would fail identically. The one edge case: if the secret content differs across regions (e.g. primary holds malformed/legacy JSON but the failover region has valid JSON), the old code would have failed over and succeeded, whereas the new code fails fast on the primary.

This is a very unlikely scenario, and failing fast with a clear message is arguably the better behavior anyway — but since it's a behavioral change to failover, it's worth a conscious decision. If you agree it's acceptable, a one-line note in the PR description or a code comment would help future readers.

2. JSONProcessingError doesn't wrap the underlying error

utils/error_handling_helper.go:11-15

type JSONProcessingError struct {
    Message string
}
func (e *JSONProcessingError) Error() string { return e.Message }

The json.Unmarshal / jmespath.Search errors are formatted into the string via %v but not retained as a wrapped error. Consider adding an Err error field and an Unwrap() method so callers can still errors.Is/As the root cause:

type JSONProcessingError struct {
    Message string
    Err     error
}
func (e *JSONProcessingError) Error() string { return e.Message }
func (e *JSONProcessingError) Unwrap() error  { return e.Err }

Not required for correctness (the message already interpolates the detail), just a robustness nicety. Fix this →

3. Minor: error-string capitalization

The new messages start with capital letters ("Failed to parse...", "Invalid JMESPath..."). Go's ST1005 convention discourages capitalized error strings, but the surrounding codebase already capitalizes (e.g. "Failed to fetch secret from all regions..."), so this is consistent with the repo — no change needed, just noting it.

---

Nits

• provider/parameter_store_provider.go:174 — dropping the client.Region prefix from the returned error is correct now that it's fatal (the error is region-independent). Good catch.
• The test refactor from exact-match to strings.Contains (secret_value_test.go) is reasonable given the messages now embed the underlying error text.

Nothing blocking here — the change is correct and well-tested. Items 1 and 2 are worth a quick decision but I'd be comfortable merging as-is.

[codecov]

Codecov Report

❌ Patch coverage is 92.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.72%. Comparing base (3b0dd00) to head (07ba764).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
provider/parameter_store_provider.go	0.00%	1 Missing ⚠️
utils/error_handling_helper.go	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #629      +/-   ##
==========================================
+ Coverage   60.84%   61.72%   +0.88%     
==========================================
  Files          11       11              
  Lines         710      729      +19     
==========================================
+ Hits          432      450      +18     
- Misses        261      262       +1     
  Partials       17       17              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coryflucas]

@simonmarty let me know if you'd like to see any tweaks based on review from Claude.

It's 100% right on item 1 that some times trying multiple regions might work, but it did not really seem like a real use case as it would mean someone has a secret in multiple regions where either 1 of the regions is JSON and the other is not, or they are both JSON but have different structure. Both scenarios seem very unlikely.

[simonmarty]

Yeah I agree with you that the secret value being different between the primary and the failover region is not something that we should worry about. I think can assume they are either both valid JSON or both invalid but I'm double checking.