Implement client secret for Databricks managed credential in AWS

[t-margheim]

This PR improves the team operator's support for Databricks managed credentials in Workbench. Prior to this, it was impossible to set the client secret for Databricks instances which were in AWS; only Azure based Databricks instances would have a client secret set. This was because the client secret is an optional parameter for Databricks instances housed in AWS, but is required for Azure hosted instances. See the Posit Workbench docs for more details.

[claude]

Claude finished @t-margheim's task —— View job

---

Reviewed PR #43

• Read review guidelines
• Get PR diff
• Review changed files
• Submit review

Review submitted with findings including:

• Critical: Bug in AWS secret error classification logic (condition is inverted)
• Behavioral change: Databricks errors now fail reconciliation (previously logged as non-fatal)
• Suggestions: Error type distinction for Azure, test determinism note

[claude]

Review Summary

This PR adds support for Databricks client secrets in AWS (previously only Azure was supported) with appropriate error handling that makes secrets optional for AWS but required for Azure.

---

Critical Issue

Bug in api/product/secret.go:263-268: The condition logic is inverted. Currently returns SecretNotFoundError when the error code is NOT ErrCodeResourceNotFoundException:

if errors.As(err, &awsErr) && awsErr.Code() != secretsmanager.ErrCodeResourceNotFoundException {
    return "", newSecretNotFoundError(secretType, vaultName, key, err)
}

Should be:

if errors.As(err, &awsErr) && awsErr.Code() == secretsmanager.ErrCodeResourceNotFoundException {
    return "", newSecretNotFoundError(secretType, vaultName, key, err)
}
return "", newSecretAccessError(secretType, vaultName, key, err)

This bug means AWS "not found" scenarios won't be correctly classified, which would break the optional-secret behavior for AWS Databricks.

---

Behavioral Change

internal/controller/core/workbench.go:162-165: Previously, Azure Databricks secret failures were logged as "Not fatal" and reconciliation continued. Now errors cause reconciliation to fail with return ctrl.Result{}, err. This is likely intentional but worth confirming.

---

Suggestions

1. internal/controller/core/workbench.go:64-69: For Azure Databricks, all errors return immediately. Consider distinguishing between SecretNotFoundError (configuration issue) vs SecretAccessError (transient failure) for clearer operational guidance.

2. internal/controller/core/workbench_test.go:84: Map iteration order in Go is non-deterministic. The test iterates over Databricks map entries and relies on assertions that pass regardless of order. Consider adding a comment noting this is order-independent, or splitting into separate subtests for clarity.

---

Positive Notes

• Clean introduction of typed errors (SecretNotFoundError, SecretAccessError) enables precise error handling
• Good separation of concerns with extracted fetchAWSSecret and fetchKubernetesSecret functions
• Test coverage includes both "secret found" and "secret not found" scenarios for AWS and Azure
• The StrictMode addition to TestSecretProvider is a nice pattern for testing

[t-margheim]

@claude, please review this again.

[t-margheim]

@claude, please review this PR again.

[t-margheim]

I'm not sure what we want to do with this PR. On the one hand, I think it might make sense for the team operator to support setting client secret for Databricks instances hosted on AWS. On the other hand, we don't have any need for this functionality today, so I'd rather introduce a change like this when we had more robust testing in place.

I'm open to any ideas here regarding whether we should merge or close this PR.

[stevenolen]

I'd rather introduce a change like this when we had more robust testing in place.

I think it makes sense since the behavior isn't actually required for all use-cases we have, we can close this and opt to implement at a future time if it becomes required.

[t-margheim]

Closing for now, since we don't actually need this capability yet.