Accept Opaque secrets as TLS credentials for backward compatibility

[AryanP123]

Fixes #2518

Summary by CodeRabbit

• New Features

  • TLS credentials are now recognized from compatible secrets even when they aren’t explicitly labeled as TLS.
  • Syncing can now satisfy TLS profiles backed by opaque secrets (with TLS material present).

• Bug Fixes

  • Improved TLS secret classification to be more permissive for backward compatibility while excluding irrelevant types (e.g., basic-auth).
  • Updated cache and sync logic to consistently use the new TLS predicate.

• Tests

  • Added unit tests covering TLS/opaque/basic-auth/nil secret scenarios and an integration-style test for opaque TLS sync.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 47532b36-68c3-424d-b4c0-126ceba9adf7

📥 Commits

Reviewing files that changed from the base of the PR and between 72dff74 and 291715b.

📒 Files selected for processing (2)

• internal/kube/secrets/context.go
• internal/kube/secrets/context_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/kube/secrets/context_test.go

---

📝 Walkthrough

Walkthrough

Adds an IsTlsCredentialSecret helper that identifies TLS-capable Kubernetes Secrets by type and data keys, allowing Opaque-typed secrets with TLS data to be treated as credentials. Updates secret manager and sync handlers to use this helper, plus adds corresponding tests.

Changes

TLS Credential Detection

Layer / File(s)	Summary
IsTlsCredentialSecret helper and tests internal/kube/secrets/context.go, internal/kube/secrets/context_test.go	Adds exported IsTlsCredentialSecret function handling nil secrets, SecretTypeTLS, and backward-compatible SecretTypeOpaque/empty-type secrets with required data checks; adds unit tests covering multiple type/data combinations.
Secret manager dispatch update internal/kube/secrets/manager.go	handleSecret and TlsCredentialSecretPresent now use IsTlsCredentialSecret instead of direct secret.Type comparison to select the TLS branch and scan cached secrets.
Sync handler dispatch and integration test internal/kube/secrets/sync.go, internal/kube/secrets/sync_test.go	(*Sync).handle restructures its branch logic to use IsTlsCredentialSecret for TLS matching while retaining the basic-auth check; adds TestSyncHandlerOpaqueTlsSecret and fixtureOpaqueTlsSecret verifying an Opaque secret resolves an SSL profile.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

• skupperproject/skupper#2523: Both PRs change how TLS credentials are identified or created for Kubernetes Secrets, with one aligning secret creation to kubernetes.io/tls and this PR broadening TLS detection to accept eligible Opaque secrets.

Suggested reviewers: nluaces, c-kruse, fgiorgetti

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: accepting Opaque secrets as TLS credentials for backward compatibility.
Linked Issues check	✅ Passed	The code updates TLS secret detection to accept Opaque secrets and adds tests, satisfying issue #2518.
Out of Scope Changes check	✅ Passed	The diff stays focused on TLS secret classification and its tests, with no obvious unrelated changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/kube/secrets/manager.go (1)

75-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding direct test coverage for the Opaque-secret dispatch path here.

sync.go got TestSyncHandlerOpaqueTlsSecret verifying the Opaque-secret branch end-to-end, but no equivalent test is included for handleSecret/TlsCredentialSecretPresent in this file. Since both files independently branch on IsTlsCredentialSecret, a regression in one wouldn't be caught by the other's tests.

Also applies to: 252-252

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c320e23-6097-411c-9153-a49cae40d1d7

📥 Commits

Reviewing files that changed from the base of the PR and between 172317c and 72dff74.

📒 Files selected for processing (5)

• internal/kube/secrets/context.go
• internal/kube/secrets/context_test.go
• internal/kube/secrets/manager.go
• internal/kube/secrets/sync.go
• internal/kube/secrets/sync_test.go

[c-kruse]

This last assertion would break at least one (probably untested) thing. Until this regression and the subsequent changes to tls credentials dependencies the controller was designed to mostly turn a blind eye to tlsCredentials secret contents. It mostly tried to sort the contents out in the router/kube-adaptor.

It's probably more work than just this to iron out all of the corner cases, but to tell whether or not a secret is suitable to be used with an sslProfile configuration we need more context. Example: a Connector with spec.useClientCert=false (default) only configures a sslProfile with a CA - no cert or key file.

All that to say - let's either properly solve for this with a path with concrete "reasons" a secret is unacceptable for a particular tlsCredential or return to ye olden days with something wide open like 'secret.Type == "" || opaque || tls'

[c-kruse]

Not to complicate this more, but just ran into another edge case we've never handled well: Listeners and MultiKeyListeners with requireClientCert=false have no real purpose for a CA certificate. We currently configure the sslProfile with one unconditionally and the router crashes when it is empty/absent. Looks like the kubernetes.io/tls type only requires tls.crt and tls.key (not ca.crt.)

Maybe the least controversial, most backwards compatible IsTlsCredentialSecret would be something like this? Would really count on your judgement here: A cryptic "Secret not found" error message is a big improvement over a crashing router deployment, but a major downgrade from a working link/service generated by regular skupper tooling.

func IsTlsCredentialSecret(secret *corev1.Secret) bool {
	if secret == nil || secret.Data == nil || secret.Type == corev1.SecretTypeBasicAuth {
		return false
	}
	return len(secret.Data["ca.crt"]) > 0
}

[AryanP123]

Went with your approach. Opaque/empty type + ca.crt present, basic-auth excluded. I also kept kubernetes.io/tls unconditionally true so listener certs without ca.crt don't regress. Still treating this as presence-only, happy to follow up with per-use-case validation separately if useful.

[c-kruse]

Thanks @AryanP123!