[Docs][Connector-V2] Improve alert and API connector docs#11383
[Docs][Connector-V2] Improve alert and API connector docs#11383DanielCarter-stack wants to merge 1 commit into
Conversation
SEZ9
left a comment
There was a problem hiding this comment.
Thanks for working on this. I re-reviewed the latest head a90854c26a88 from scratch.
What this PR fixes
- User pain: The alert/API connector docs (WeChat, Slack, Sentry, My Hours, OpenMLDB) drifted from the actual connector factories — wrong option names like
flushTimeoutMills, a brokenSlackSinkidentifier in examples, and missing retry/multi-table options that made jobs fail or behave unexpectedly when users copied examples verbatim. - Fix approach: Align option tables (names, types, required flags, defaults) with the current option rules, fix the Slack identifier and Sentry
flushTimeoutMillisspelling, correct the WeChat/Slack message-format descriptions to match what the sinks actually emit, and keep EN/CN docs in sync. - One-line summary: Docs-only PR that is accurate against the connector code and mergeable, though it should add config-encryption guidance for the plaintext credentials (
email/password,oauth_token) it documents and swap the WeChat example key for an unambiguous placeholder.
Findings
Issue 1: MyHours docs should warn that email/password are plaintext secrets in the job config and point users to SeaTunnel's config encryption
- Location:
docs/en/connectors/source/MyHours.md:40 - Why it matters: - Root cause: the rewritten options table documents
emailandpasswordas plain string options with no guidance on protecting them, and the description explicitly states the connector "logs in with the configuredemailandpassword". - Impact: job HOCON files are routinely committed to VCS, shipped to the Zeta/Flink/Spark cluster, and echoed in job submission logs or REST job-config endpoints, so plaintext My Hours account credentials in the config are a real leak vector; the docs are the only place most users learn how to avoid this.
- Fix: add a short note under the
email/passwordoption descriptions recommending SeaTunnel's built-in config encryption (shade.identifier, see docs/en/connectors/Config-Encryption-Decryption.md) or variable substitution (-ijob variables) instead of hard-coding credentials, matching the wording already used in other credential-bearing connector docs. Apply the same note to the Chinese doc for consistency. - Evidence:
docs/en/connectors/source/MyHours.md:36emailandpassword, obtains an access token, and then sends the configured HTTP request with that token. - Suggested fix: Since
email/passwordare real account credentials that end up in plaintext in the job HOCON (and therefore in VCS, cluster nodes, and potentially job-submission logs), please add a note here recommending SeaTunnel's config encryption (shade.identifier) or-ivariable substitution instead of hard-coding them. Other credential-bearing connector docs carry this warning, and this rewrite is the right place to add it. Please mirror the note indocs/zh/connectors/source/MyHours.mdas well. - Severity: Medium
Issue 2: Slack oauth_token should be documented as a sensitive credential; identifier fix from SlackSink to Slack is correct but needs the same treatment as other secrets
- Location:
docs/en/connectors/sink/Slack.md:34 - Why it matters: - Root cause: the updated description "Slack OAuth token used to list channels and post messages" accurately describes usage but doesn't flag that a
xoxp-/xoxb-token grants broad workspace API access well beyond posting to one channel. - Impact: users following the example will embed a user-scoped
xoxp-token in plaintext job configs; if that config leaks, the blast radius is the whole Slack workspace scope of the token, not just the alert channel. The example token in the diff is fully masked withxs, which is fine, but the option row is where users learn how to handle it. - Fix: add "Treat this as a secret; prefer a bot token (
xoxb-) with minimal scopes (chat:write,channels:read) and use config encryption or variable substitution instead of hard-coding it" to theoauth_tokendescription. Also good catch fixing the block name fromSlackSinktoSlack— that matches theSlackSinkFactoryidentifier and the old doc example would have failed plugin discovery; please make sure the zh doc example usesSlacktoo. - Evidence:
docs/en/connectors/sink/Slack.md:44 Slack { - Suggested fix: Since a Slack OAuth token (especially a user
xoxp-token) grants API access far beyond posting to one channel, please extend this description to say it must be treated as a secret — recommend a minimally-scoped bot token (xoxb-withchat:write+channels:read) and config encryption/variable substitution rather than hard-coding. Separately, theSlackSink→Slackidentifier fix in the example below is correct (matchesSlackSinkFactory); please double-check the zh example got the same fix. - Severity: Low
Issue 3: Sentry example enables enableExternalConfiguration = true without explaining the security implication of external SDK config loading
- Location:
docs/en/connectors/sink/Sentry.md:71 - Why it matters: - Root cause: the polished example now sets
enableExternalConfiguration = true, and the new option-table row only says "Whether the Sentry SDK can load external configuration" without stating what that means. - Impact: with this flag on, the Sentry SDK will pick up DSN, proxy, and other settings from
sentry.properties, system properties, and environment variables on the worker hosts; users copy-pasting this example onto shared Zeta/Flink/Spark clusters may unknowingly have their events redirected by environment-level configuration they don't control, which is both a surprise-behavior and a data-exfiltration-adjacent risk. - Fix: either drop
enableExternalConfiguration = truefrom the minimal example (it isn't needed to demonstrate the sink) or add one sentence to the option description clarifying that enabling it lets environment/sentry.propertiesvalues override job-level settings, so it should only be enabled on trusted hosts. Keep the zh doc consistent. - Evidence:
docs/en/connectors/sink/Sentry.md:11 | enableExternalConfiguration | boolean | no | - | Whether the Sentry SDK can load external configuration. | - Suggested fix: Enabling
enableExternalConfiguration = truein the canonical example means the Sentry SDK will also read DSN/settings fromsentry.properties, system properties, and environment variables on the worker hosts — which can silently override the job config on shared clusters. I'd either drop this line from the minimal example or add a sentence to the option description explaining that external config can override job settings and should only be enabled on trusted hosts. - Severity: Low
Issue 4: WeChat webhook example key looks like a partially-obfuscated real key; use an unambiguous placeholder
- Location:
docs/en/connectors/sink/Enterprise-WeChat.md:63 - Why it matters: - Root cause: both re-formatted examples keep the value
key=693axxx6-7aoc-4bc4-97a0-0ec2sifa5aaa, which follows the exact UUID shape of a real Enterprise WeChat robot key and is only partially masked withxxx, so it reads like a redacted real credential rather than a placeholder. - Impact: since the webhook key is the only secret protecting the robot (anyone holding the URL can post to the group), examples that look like lightly-masked real keys train users to paste their own real keys into shared configs, and secret scanners may repeatedly flag this string in the repo.
- Fix: while you're re-indenting these blocks, replace the value with an obviously fake placeholder such as
key=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxand add a one-line note that the webhook URL must be treated as a secret. Apply the same change todocs/zh/connectors/sink/Enterprise-WeChat.md. - Evidence:
docs/en/connectors/sink/Enterprise-WeChat.md:69 url = "https://qyapi.weixin.qq.com/cgi-bin/webhook/send?key=693axxx6-7aoc-4bc4-97a0-0ec2sifa5aaa" - Suggested fix: This example key (
693axxx6-7aoc-4bc4-97a0-0ec2sifa5aaa) has the exact shape of a real WeChat robot key with only a few characters masked, so it reads like a redacted real secret. Since anyone with the webhook URL can post to the group, please switch to an unambiguous placeholder likekey=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxand note that the webhook URL is a secret. Same for the second example below and the zh doc. - Severity: Low
Review conclusion
Conclusion: can merge; the notes below are non-blocking
2. Suggested (non-blocking) follow-ups
- Issue 1: MyHours docs should warn that
email/passwordare plaintext secrets in the job config and point users to SeaTunnel's config encryption - Issue 2: Slack
oauth_tokenshould be documented as a sensitive credential; identifier fix fromSlackSinktoSlackis correct but needs the same treatment as other secrets - Issue 3: Sentry example enables
enableExternalConfiguration = truewithout explaining the security implication of external SDK config loading - Issue 4: WeChat webhook example key looks like a partially-obfuscated real key; use an unambiguous placeholder
Nice work — thanks again for the contribution. Happy to discuss any of the above.
DanielLeens
left a comment
There was a problem hiding this comment.
Thanks for working on this. I re-reviewed the latest head a90854c26a88 from scratch and also read through @SEZ9's earlier review. I agree with the general direction of the doc cleanup and with most of the non-blocking polish points already raised there, but I do see one blocker on the MyHours example path that I don't think has been called out yet.
What this PR fixes
- User pain: several alert/API connector docs had drifted from the current factories and option rules, so users could hit wrong option names, wrong block identifiers, or behavior that did not match the docs.
- Fix approach: this PR rewrites the option tables and examples for Enterprise WeChat, Slack, Sentry, MyHours, and OpenMLDB so they line up better with the current connector implementations.
- One-line summary: the cleanup is useful overall, but the English MyHours mock examples were changed from runnable
Httpexamples intoMyHoursexamples that do not actually run as written.
Full call path I checked
Job config
-> MyHoursSource(ReadonlyConfig) [MyHoursSource.java:45-49]
-> super(pluginConfig)
-> getAccessToken(pluginConfig)
Login phase
-> MyHoursSource.getAccessToken() [MyHoursSource.java:68-105]
-> MyHoursSourceParameter.buildWithLoginConfig(pluginConfig) [MyHoursSourceParameter.java:40-60]
-> setUrl(MyHoursSourceOptions.AUTHORIZATION_URL) [MyHoursSourceParameter.java:42]
-> setMethod(POST) [MyHoursSourceParameter.java:44]
-> put email/password into the login body [MyHoursSourceParameter.java:47-53]
-> execute login request
Business request phase
-> buildWithConfig(pluginConfig, accessToken) [MyHoursSourceParameter.java:30-38]
-> add Authorization header
-> createReader()
-> HttpSourceReader finally uses the configured business URL
Blocking issue
Issue 1: the English MyHours mock examples are no longer runnable after changing them from Http to MyHours
- Location:
docs/en/connectors/source/MyHours.md:217,docs/en/connectors/source/MyHours.md:272,.../MyHoursSource.java:45,.../MyHoursSourceParameter.java:40 - Why this is a real problem:
- Before this PR, those examples were
Httpconnector examples pointing athttp://mockserver:1080/..., which matched the old E2E references and were fine as pure JSON extraction examples. - In this PR they were mechanically rewritten to
MyHours { ... }, butMyHoursSourcealways performs a login first. The first request is not the documented mockserver URL; it is the hard-coded MyHours authorization URL set inbuildWithLoginConfig(). - So a user who copies the doc as written will fail in the login phase before they ever reach the documented
content_field/json_fieldpath.
- Before this PR, those examples were
- Risk:
- This breaks the main reader path for those examples.
- It also sends users in the wrong debugging direction, because it looks like a JSON-path/config problem while the real failure is the hidden login precondition.
- Best fix:
- Option A (recommended): revert those two examples back to
Httpand restore the old mock/E2E references, since they are really demonstrating JSON extraction semantics rather than the full MyHours auth flow. - Option B: keep
MyHours, but then the docs need to explicitly say that a real MyHours login environment is required and that mockserver only covers the second business request, not the initial auth hop.
- Option A (recommended): revert those two examples back to
- Severity: High
- Already raised by others: No
Review conclusion
Conclusion: can merge after fixes
- Blocking item
- Issue 1: the English MyHours examples currently describe a runnable mock flow that the actual
MyHoursconnector cannot execute as written, because login to the real authorization endpoint happens first.
- Non-blocking suggestions
- I broadly agree with @SEZ9's earlier non-blocking suggestions around secret-handling guidance, the Sentry external-config note, and using a more obviously fake Enterprise WeChat webhook placeholder, so I am not repeating them as separate blockers here.
Overall, the doc cleanup is moving in the right direction, but I would fix the MyHours example path before merging so the docs stay truthful to the real connector lifecycle. Happy to re-review once that is updated.
|
Rechecked the latest head: the source-level blocker from my previous Daniel round is still closed for me, and the current At this point I do not see a new code-side blocker from Daniel on this revision. For queue bookkeeping, this branch is still If the repository still needs a write-capable maintainer to refresh the formal review state or dismiss an older visible |
|
Rechecked the same unchanged head Runtime path recheckedMerge conclusionConclusion: source blocker cleared from Daniel's side
I do not see a reopened code-side blocker from Daniel on the current head. |
Summary
This PR improves documentation for five connector docs that had not been modified by recent connector-doc PRs:
Changes
flushTimeoutMillisoption spelling and example usage.Verification
git diff --check.