docs: warn that connector configs are stored in plaintext, and document secrets provider behaviour - #204
Conversation
|
@david-streamlio:Thanks for your contribution. For this PR, do we need to update docs? |
|
@david-streamlio:Thanks for providing doc info! |
freeznet
left a comment
There was a problem hiding this comment.
Thanks for this — the table gaps were real and the secrets-over-plaintext-configs guidance is the right thing to document.
I verified the PR's central claim mechanically rather than by eye, and it holds:
- Completeness ✅ — extracted every
resourceSource*Key/resourceSink*Keyconstant frompulsar/resource_pulsar_source.goandpulsar/resource_pulsar_sink.goand diffed against the two README tables on this branch: zero missing, zero extra top-level arguments in either (after excluding the 5 nestedinput_specssub-keys). - Descriptions ✅ — all new rows match the schema
Descriptionstrings verbatim, so README and generateddocs/resources/*.mdagree. Agreed notfplugindocsrun needed. Requiredcolumn ✅ — every new row is correctlyFalse. The onlyRequired: trueattributes are tenant/namespace/name/archive/destination_topic_name (source) and tenant/namespace/name/cleanup_subscription/archive/auto_ack (sink).- Secret shape ✅ — the documented
{ "path": ..., "key": ... }object matches what the acceptance tests already exercise (resource_pulsar_source_test.go:254,resource_pulsar_sink_test.go:237). - No perpetual-diff risk ✅ — I ran the snippets through
terraform validate/apply:jsonencodeemits compact key-sorted JSON, which matches what the read path writes back viajson.Marshal(map[string]interface{}). Both the README (:) and examples (=) forms parse fine.
Two things I'd like changed before merge, both in the examples rather than the tables:
- The
gsaKeysecret in the source example doesn't correspond to anything the connector reads. Both source examples usepulsar-io-filewithconfigs = {"inputDirectory":"opt"}— a filesystem source with no credentials at all. There was no plaintext credential to move, and the file connector never reads agsaKey. Details inline. examples/should stay runnable against the repo's own local cluster.hack/pulsar-docker.shstarts a plain Pulsar standalone (defaultDefaultSecretsProviderConfigurator→ClearTextSecretsProvider, process/thread runtime). The{path, key}indirection isKubernetesSecretsProviderConfigurator-specific — as your README note says.terraform applystill succeeds (Pulsar stores the map without resolving it at submit time), so this fails silently: the JDBC sink just gets no password at runtime. Details inline.
The rest are nits — take or leave.
One follow-up worth a separate issue, since this PR raises the security framing: configs isn't marked Sensitive: true in either schema, so its value is echoed in plan output and stored readably in state regardless of the secrets guidance. Closing that is a code change, and I'd rather not widen a docs PR into it — but the note here would be strictly stronger once it lands.
| // Reference the database password through the secrets provider rather than | ||
| // embedding it in `configs` (stored in plaintext). The sink reads it under | ||
| // the `password` secret name. | ||
| secrets = jsonencode({ |
There was a problem hiding this comment.
Nit — style drift from the convention already in the tree. examples/functions/main.tf:61 writes this as:
secrets = jsonencode(
{
"SECRET1": {
"path": "sectest"
"key": "hello"
}
})i.e. the paren on its own line and : separators — which is what your README snippets use. The two new blocks in examples/sinks/main.tf and examples/sources/main.tf use inline jsonencode({ with = instead. Both parse (I checked with terraform validate), so purely cosmetic, but worth matching the functions example so all three read the same.
| | `custom_runtime_options` | A string that encodes options to customize the runtime, see docs for configured runtime for details | False | | ||
| | `secrets` | The map of secretName to an object that encapsulates how the secret is fetched by the underlying secrets provider | False | | ||
| | `max_pending_messages` | The maximum size of a queue holding pending messages | False | | ||
| | `max_pending_messages_across_partitions` | The maximum number of pending messages across partitions | False | |
There was a problem hiding this comment.
Nit, purely cosmetic: the long new keys (max_pending_messages_across_partitions, use_thread_local_producers, crypto_key_reader_classname, and negative_ack_redelivery_delay_ms in the sink table) overflow the padded column widths the rest of the table uses, so the raw source goes ragged. Renders identically on GitHub — only worth a reflow if you want future diffs to stay clean.
|
Thanks for the thorough review! Addressed in 068acd5: dropped the Agreed |
|
Filed #213 to track marking |
The `secrets` argument has been supported on pulsar_source and pulsar_sink
since v0.2.0, but nothing in the provider says why it exists. The generated
docs describe what it is - a map of secret name to a fetch reference - and
stop there, so a user has no way to learn that credentials placed in
`configs` are stored and returned in plaintext through the function
metadata topic, `pulsar-admin sinks get`, and Terraform state.
Put the guidance in the schema Description strings so it reaches the
generated docs and cannot drift from the schema, and document the
`ClearTextSecretsProvider` behaviour alongside it: `{path, key}` references
are resolved only by a runtime whose secrets provider understands them, so
under the process and standalone runtimes - including the cluster started
by `make run-pulsar-in-docker` - apply succeeds while the connector
silently receives no value for the secret. That failure mode is invisible
without being told about it.
Also add a worked example to examples/sinks/main.tf showing the credential
moved out of `configs` and into `secrets`.
This replaces the original contents of this pull request, which documented
these arguments as README property tables. Commit f8319dc has since
replaced those tables with links to the generated docs, and every argument
the tables covered is now published from the schema, so only the security
guidance remained unaddressed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
88d94a1 to
1747879
Compare
|
Rewritten and rebased onto current master — this is now a fresh diff on the same branch, so it is worth re-reading rather than skimming the previous review context. The original version documented What was never covered anywhere is the security guidance this PR was opened for — that credentials in So the guidance now lives in the schema Now MERGEABLE. |
Motivation
secretshas been supported onpulsar_sourceandpulsar_sinksince v0.2.0 (#96 / #97), but nothing in the provider explains why it exists.The generated docs say what it is — "The map of secretName to an object that encapsulates how the secret is fetched by the underlying secrets provider" — and stop. A user reading that has no way to learn the thing that actually matters: credentials placed in
configsare stored and returned in plaintext, through the function metadata topic,pulsar-admin sinks get/sources getoutput, and Terraform state. The natural thing to do — put the password inconfigs, because that is where the connector's other settings go — is the wrong thing, and nothing says so.There is a second, sharper problem.
{"path": ..., "key": ...}references are resolved only by a runtime whose secrets provider understands them, such as the Kubernetes runtime'sKubernetesSecretsProviderConfigurator. Under the defaultClearTextSecretsProvider— process and standalone runtimes, including the cluster this repository's own acceptance tests start viamake run-pulsar-in-docker—terraform applysucceeds and the connector silently receives no value for the secret. A user who follows the advice to move credentials intosecrets, and tests it locally, gets a green apply and a connector that cannot authenticate, with nothing anywhere to explain it.Neither fact is documented anywhere in the repository today:
grep -riE "plaintext|ClearTextSecretsProvider|secrets provider" docs/ README.mdreturns only the two auto-generated description lines above.Modifications
Descriptionstrings forconfigsandsecretson bothpulsar_sourceandpulsar_sinkto carry the plaintext warning and theClearTextSecretsProvidercaveat. Putting the guidance in the schema rather than in prose means it reaches the generated docs automatically and cannot drift away from the code.docs/resources/sink.mdanddocs/resources/source.mdwithgo generate ./....examples/sinks/main.tfshowing the password moved out ofconfigsand intosecrets, with the runtime caveat noted inline.What changed and why
The original diff added README property-table rows for 21 arguments across the two resources. All 21 are now published from the schema in
docs/resources/{sink,source}.md— I checked each one — so re-targeting those rows would have duplicated content that already exists, in a file that no longer holds tables.What was not covered anywhere was the security guidance, which was the reason this PR was opened. That is all this now contains.
Verifying this change
This change is documentation only and carries no test coverage of its own. Verified as follows:
go generate ./...reproduces the committeddocs/output; the only diff is the four description strings.make testandgolangci-lintpass.terraform fmt -check -recursive examples/is clean.Documentation
Check the box below.
Need to update docs?
doc-requiredno-need-docdocSchema descriptions updated and
docs/resources/{sink,source}.mdregenerated.