cockroachdb / cockroachdb/cockroach

changefeedccl: external:// sink silently drops the URI path

Open
#174,687 2 comments 0 reactions 1 assignee Claimed by @sjain022 View on GitHub
A-cdc C-bug P-2 T-cdc
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

A changefeed sink URI of the form `external:///` silently discards the
`` component. The feed is created successfully and runs, but writes to the
external connection's base URI instead of the requested subdirectory. There is no
error and no warning.

The same URI form works as expected for external *storage* (`BACKUP`, `IMPORT`), which
makes the divergence easy to trip over: the two paths look identical to a user but
behave differently.

**To Reproduce**

```sql
CREATE EXTERNAL CONNECTION mybucket AS 'gs://my-bucket/base?AUTH=implicit';

-- Writes to gs://my-bucket/base/subdir/ -- path honored.
BACKUP INTO 'external://mybucket/subdir';

-- Writes to gs://my-bucket/base/ -- path silently dropped.
CREATE CHANGEFEED FOR TABLE foo INTO 'external://mybucket/subdir';
```

Two changefeeds pointed at `external://mybucket/a` and `external://mybucket/b` will
therefore write into the same directory.

**Expected behavior**

Either the requested path is joined onto the external connection's URI, matching the
external-storage behavior, or the statement is rejected. Accepting the URI and then
ignoring part of it is the problematic case.

**Root cause**

`makeExternalConnectionSink` in `pkg/ccl/changefeedccl/sink_external_connection.go`
reads only the host component:

```go
externalConnectionName := u.Host

uri, err := p.lookup(externalConnectionName)
if err != nil {
return nil, err
}
// Replace the external connection URI in the `feedCfg` with the URI of the
// underlying resource.
feedCfg.SinkURI = uri
return getSink(ctx, serverCfg, feedCfg, timestampOracle, user, jobID, m, targets, false /* initialValidation */)
```

`u.Path` is never consulted, and `isqlExternalConnectionProvider.lookup` returns
`d.SimpleURI.URI` verbatim. The stored URI is passed to `getSink` unmodified.

Query parameters on the `external://` URI are dropped by the same code for the same
reason — `u.Query()` is not read either. (Whether they *should* be honored is the
separate UX question raised in #156532; this issue is about them being accepted and
then ignored rather than about the desired policy.)

**The path-aware implementations that already exist**

`pkg/cloud/externalconn/connection_storage.go` handles this correctly in two steps.
`makeExternalConnectionConfig` records the requested path:

```go
externalConnCfg.Path = uri.Path
```

and `makeExternalConnectionStorage` applies it when resolving the underlying resource:

```go
// Append the subdirectory that was passed in with the `external` URI to the
// underlying storage URI.
uri, err := url.Parse(d.SimpleURI.URI)
if err != nil {
return nil, errors.Wrap(err, "failed to parse underlying storage URI")
}
uri.Path = path.Join(uri.Path, cfg.Path)
```

`pkg/cloud/externalconn/connection.go` also provides `externalconn.Materialize`, which
does both halves properly — validates the scheme and that the host matches the
connection name, merges query parameters via `url.Values.Add`, and joins the path with
`url.URL.JoinPath` rather than string concatenation:

```go
func Materialize(ec ExternalConnection, uri *url.URL) (*url.URL, error) {
```

**Notes on a fix**

The changefeed path cannot call `Materialize` as currently structured, because the
provider interface is typed to a bare string and discards the `ExternalConnection`
object that `Materialize` needs:

```go
type externalConnectionProvider interface {
lookup(name string) (string, error)
}
```

`lookup` already loads that object and then throws it away. Widening the interface so
that resolution takes the full `*url.URL` and returns the materialized URI would let it
call `Materialize` internally, and would pick up the host/connection-name validation for
free.

One design question worth settling first: path semantics differ by sink type. For
`cloudstorage` a subdirectory is meaningful and joining it matches the external-storage
behavior exactly. For `kafka`, `webhook`, and `pubsub` a path component may be
meaningless or already load-bearing, so joining unconditionally could change behavior for
existing feeds. Rejecting a non-empty path for the sink types that do not use one may be
the safer choice there. Either way is an improvement on dropping it silently.

**Environment**

- Present on `master`, and on `release-26.1` and `release-25.4` — the `u.Host`-only
resolution is unchanged across all three.

Jira issue: CRDB-67903

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.