Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,11 @@ linters:
- name: var-naming

# G104: Audit errors not checked (duplicates errcheck).
# G115: integer overflow conversions are too noisy for this codebase.
gosec:
excludes:
- G104
- G115
Comment on lines +87 to +91

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n .golangci.yml | sed -n '80,130p'

Repository: pingcap/ticdc

Length of output: 1886


Avoid disabling G115 repo-wide.

_test.go files and the tests/ directory are already fully excluded from gosec (Lines 114–129), so this change specifically removes integer overflow checks (G115) from production code. This weakens security posture without justification, as the "noise" is likely confined to test files which are already ignored.

Scope this exclusion to specific paths or use //nolint:gosec annotations at the specific call sites instead of disabling the rule globally.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.golangci.yml around lines 87 - 91, The repo-wide gosec exclusion is
disabling G115 for production code, which should be narrowed instead of applied
globally. Remove G115 from the top-level gosec excludes in the golangci config
and keep integer-overflow suppression limited to the already excluded test paths
or specific call sites. If a few production conversions are intentional,
annotate those exact spots with nolint rather than weakening the entire
codebase. Use the existing gosec and tests exclusions in the config to keep the
scope constrained.


# ST1000: don't require package comments.
# ST1003: don't enforce naming conventions on legacy identifiers.
Expand Down
3 changes: 2 additions & 1 deletion api/v2/changefeed.go
Original file line number Diff line number Diff line change
Expand Up @@ -1770,7 +1770,8 @@ func verifyTable4MQ(
return nil
}

eventRouter, err := eventrouter.NewEventRouter(replicaConfig.Sink, topic, config.IsPulsarScheme(scheme), protocol == config.ProtocolAvro)
isAvroLike := protocol == config.ProtocolAvro || protocol == config.ProtocolDebeziumAvro
eventRouter, err := eventrouter.NewEventRouter(replicaConfig.Sink, topic, config.IsPulsarScheme(scheme), isAvroLike)
if err != nil {
return err
}
Expand Down
54 changes: 37 additions & 17 deletions cmd/kafka-consumer/consumer.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,28 +42,48 @@ func getPartitionNum(o *option) (int32, error) {
}
defer admin.Close()

topics := strings.Split(o.topic, ",")
maxPartitionNum := int32(0)
timeout := 3000
for i := 0; i <= 30; i++ {
resp, err := admin.GetMetadata(&o.topic, false, timeout)
if err != nil {
if err.(kafka.Error).Code() == kafka.ErrTransport {
log.Info("retry get partition number", zap.Int("retryTime", i), zap.Int("timeout", timeout))
timeout += 100
continue
for _, topic := range topics {
topic = strings.TrimSpace(topic)
if topic == "" {
continue
}
found := false
for i := 0; i <= 30; i++ {
resp, err := admin.GetMetadata(&topic, false, timeout)
if err != nil {
var kafkaErr kafka.Error
if errors.As(err, &kafkaErr) && kafkaErr.Code() == kafka.ErrTransport {
log.Info("retry get partition number", zap.String("topic", topic), zap.Int("retryTime", i), zap.Int("timeout", timeout))
timeout += 100
continue
}
return 0, errors.Trace(err)
}
if topicDetail, ok := resp.Topics[topic]; ok {
numPartitions := int32(len(topicDetail.Partitions))
log.Info("get partition number of topic",
zap.String("topic", topic),
zap.Int32("partitionNum", numPartitions))
if numPartitions > maxPartitionNum {
maxPartitionNum = numPartitions
}
found = true
break
}
Comment on lines +65 to 75

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify existing metadata error handling and the Kafka client package in use.
rg -n 'confluent-kafka-go|IBM/sarama' go.mod go.sum
rg -n 'GetMetadata\(|topicDetail\.Error|TopicMetadata' --type go .

Repository: pingcap/ticdc

Length of output: 1002


🏁 Script executed:

#!/bin/bash
# Inspect the admin.go file to understand the GetMetadata implementation and return type.
cat -n pkg/sink/kafka/admin.go | head -80

Repository: pingcap/ticdc

Length of output: 3079


🏁 Script executed:

#!/bin/bash
# Check the sarama TopicMetadata struct definition in vendor or go mod cache (simulated by checking if vendor exists or just looking at the struct usage pattern in the codebase).
# Since we can't access go mod cache directly in a simple script, we will search for the struct usage in the codebase.
rg -n 'type TopicMetadata struct' --type go .

Repository: pingcap/ticdc

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Check the usage of GetMetadata response in consumer.go around line 55 to see the type of 'resp'.
cat -n cmd/kafka-consumer/consumer.go | sed -n '50,80p'

Repository: pingcap/ticdc

Length of output: 1371


🏁 Script executed:

#!/bin/bash
# Check the import in consumer.go and search for TopicMetadata struct details in the codebase or vendor logic.
rg -A 5 'type.*TopicMetadata' --type go .
rg -A 10 'type.*MetadataResponse' --type go .

Repository: pingcap/ticdc

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Check if there is a local vendor or if we can find the sarama package definition via grep in the mod cache simulation or just assume standard sarama if not found locally.
# Let's try to find the definition of the error field in the TopicMetadata struct by searching for its usage with .Err or .Error.
rg -n 'topicDetail\.Err|TopicMetadata.*Err' --type go .

Repository: pingcap/ticdc

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Check imports and admin variable declaration in consumer.go
cat -n cmd/kafka-consumer/consumer.go | head -40

Repository: pingcap/ticdc

Length of output: 1632


🏁 Script executed:

#!/bin/bash
# Find where GetMetadata is defined or used to determine the return type.
rg -n 'func.*GetMetadata' --type go .
rg -n 'MetadataResponse' --type go .

Repository: pingcap/ticdc

Length of output: 151


🏁 Script executed:

#!/bin/bash
# Since local src code for sarama/confuent structs might not be present, search for any existing usage of .Err on metadata in the codebase to confirm the pattern.
rg -n '\.Err' --type go cmd/kafka-consumer/

Repository: pingcap/ticdc

Length of output: 3089


🏁 Script executed:

#!/bin/bash
# Check the go.mod to confirm the exact version and search for any existing error handling patterns in the consumer file.
grep -A 5 'confluent-kafka-go' go.mod
rg -n 'kafka.Error' cmd/kafka-consumer/consumer.go

Repository: pingcap/ticdc

Length of output: 442


Check per-topic metadata errors before treating the topic as found.

resp.Topics[topic] can exist even when the broker reports a topic-level error (e.g., authorization denied, deleted topic). The current code treats the presence of the key as success, potentially masking errors where topicDetail.Err is set. This can lead to successful startup with an unusable topic.

The code uses confluent-kafka-go, where the error field is topicDetail.Err (not Error).

💡 Suggested fix

Check topicDetail.Err and retry if it is not kafka.ErrNoError.

 			if topicDetail, ok := resp.Topics[topic]; ok {
+				if topicDetail.Err != kafka.ErrNoError {
+					log.Info("retry get partition number due to topic error",
+						zap.String("topic", topic),
+						zap.Error(topicDetail.Err))
+					time.Sleep(1 * time.Second)
+					continue
+				}
 				numPartitions := int32(len(topicDetail.Partitions))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if topicDetail, ok := resp.Topics[topic]; ok {
numPartitions := int32(len(topicDetail.Partitions))
log.Info("get partition number of topic",
zap.String("topic", topic),
zap.Int32("partitionNum", numPartitions))
if numPartitions > maxPartitionNum {
maxPartitionNum = numPartitions
}
found = true
break
}
if topicDetail, ok := resp.Topics[topic]; ok {
if topicDetail.Err != kafka.ErrNoError {
log.Info("retry get partition number due to topic error",
zap.String("topic", topic),
zap.Error(topicDetail.Err))
time.Sleep(1 * time.Second)
continue
}
numPartitions := int32(len(topicDetail.Partitions))
log.Info("get partition number of topic",
zap.String("topic", topic),
zap.Int32("partitionNum", numPartitions))
if numPartitions > maxPartitionNum {
maxPartitionNum = numPartitions
}
found = true
break
}
🧰 Tools
🪛 ast-grep (0.44.0)

[warning] 65-65: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(len(topicDetail.Partitions))
Note: [CWE-190] Integer Overflow or Wraparound.

(integer-overflow-narrowing-conversion-go)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/kafka-consumer/consumer.go` around lines 65 - 75, The topic lookup in
consumer metadata handling is treating any present entry in resp.Topics as a
valid success, which can hide broker-reported topic-level failures. Update the
logic around topicDetail in consumer.go to inspect topicDetail.Err before
marking found=true or using the partition count, and only proceed when it equals
kafka.ErrNoError; otherwise continue retrying and do not treat the topic as
usable.

return 0, errors.Trace(err)
log.Info("retry get partition number", zap.String("topic", topic))
time.Sleep(1 * time.Second)
}
if topicDetail, ok := resp.Topics[o.topic]; ok {
numPartitions := int32(len(topicDetail.Partitions))
log.Info("get partition number of topic",
zap.String("topic", o.topic),
zap.Int32("partitionNum", numPartitions))
return numPartitions, nil
if !found {
return 0, errors.Errorf("get partition number(%s) timeout", topic)
}
log.Info("retry get partition number", zap.String("topic", o.topic))
time.Sleep(1 * time.Second)
}
return 0, errors.Errorf("get partition number(%s) timeout", o.topic)
if maxPartitionNum == 0 {
return 0, errors.Errorf("get partition number(%s) timeout", o.topic)
}
return maxPartitionNum, nil
}

type consumer struct {
Expand Down
8 changes: 4 additions & 4 deletions cmd/kafka-consumer/option.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,11 @@ func (o *option) Adjust(upstreamURIStr string, configFile string) {
}
o.partitionNum = int32(c)
}
partitionNum, err := getPartitionNum(o)
if err != nil {
log.Panic("cannot get the partition number", zap.String("topic", o.topic), zap.Error(err))
}
if o.partitionNum == 0 {
partitionNum, err := getPartitionNum(o)
if err != nil {
log.Panic("cannot get the partition number", zap.String("topic", o.topic), zap.Error(err))
}
o.partitionNum = partitionNum
}

Expand Down
6 changes: 4 additions & 2 deletions cmd/kafka-consumer/writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ func newWriter(ctx context.Context, o *option) *writer {
w.progresses[i] = newPartitionProgress(int32(i), decoder)
}

eventRouter, err := eventrouter.NewEventRouter(o.sinkConfig, o.topic, false, o.protocol == config.ProtocolAvro)
isAvroLike := o.protocol == config.ProtocolAvro || o.protocol == config.ProtocolDebeziumAvro
eventRouter, err := eventrouter.NewEventRouter(o.sinkConfig, o.topic, false, isAvroLike)
if err != nil {
log.Panic("initialize the event router failed",
zap.Any("protocol", o.protocol), zap.Any("topic", o.topic),
Expand Down Expand Up @@ -493,7 +494,8 @@ func (w *writer) onDDL(ddl *event.DDLEvent) {
return
}
switch w.protocol {
case config.ProtocolCanalJSON, config.ProtocolOpen, config.ProtocolAvro, config.ProtocolSimple, config.ProtocolDebezium:
case config.ProtocolCanalJSON, config.ProtocolOpen, config.ProtocolAvro, config.ProtocolSimple,
config.ProtocolDebezium, config.ProtocolDebeziumAvro:
default:
return
}
Expand Down
2 changes: 1 addition & 1 deletion downstreamadapter/sink/helper/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ func GetProtocol(protocolStr string) (config.Protocol, error) {
// GetFileExtension returns the extension for specific protocol
func GetFileExtension(protocol config.Protocol) string {
switch protocol {
case config.ProtocolAvro, config.ProtocolCanalJSON, config.ProtocolMaxwell,
case config.ProtocolAvro, config.ProtocolDebeziumAvro, config.ProtocolCanalJSON, config.ProtocolMaxwell,
config.ProtocolOpen, config.ProtocolSimple:
return ".json"
case config.ProtocolCraft:
Expand Down
3 changes: 2 additions & 1 deletion downstreamadapter/sink/kafka/helper.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ func newKafkaSinkComponentWithFactory(ctx context.Context,
return kafkaComponent, protocol, errors.WrapError(errors.ErrKafkaNewProducer, err)
}

isAvroLike := protocol == config.ProtocolAvro || protocol == config.ProtocolDebeziumAvro
kafkaComponent.eventRouter, err = eventrouter.NewEventRouter(
sinkConfig, topic, false, protocol == config.ProtocolAvro)
sinkConfig, topic, false, isAvroLike)
if err != nil {
return kafkaComponent, protocol, errors.Trace(err)
}
Expand Down
5 changes: 3 additions & 2 deletions pkg/config/changefeed.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,9 @@ func (info *ChangeFeedInfo) RmUnusedFields() {
info.rmMQOnlyFields()
} else {
// remove schema registry for MQ downstream with
// protocol other than avro
if util.GetOrZero(info.Config.Sink.Protocol) != ProtocolAvro.String() {
// protocol other than avro or debezium-avro
protocol := util.GetOrZero(info.Config.Sink.Protocol)
if protocol != ProtocolAvro.String() && protocol != ProtocolDebeziumAvro.String() {
Comment on lines 493 to +496

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Normalize the protocol before deciding whether to drop SchemaRegistry.

Line 496 compares the raw sink string, so protocol=flat-avro still gets its schema registry cleared here even though ParseSinkProtocolFromString maps "flat-avro" to ProtocolAvro. That mutates a valid Avro-family config into an invalid one on save/reload.

Suggested fix
-		protocol := util.GetOrZero(info.Config.Sink.Protocol)
-		if protocol != ProtocolAvro.String() && protocol != ProtocolDebeziumAvro.String() {
+		protocol, err := ParseSinkProtocolFromString(util.GetOrZero(info.Config.Sink.Protocol))
+		if err != nil || (protocol != ProtocolAvro && protocol != ProtocolDebeziumAvro) {
 			info.Config.Sink.SchemaRegistry = nil
 		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// remove schema registry for MQ downstream with
// protocol other than avro
if util.GetOrZero(info.Config.Sink.Protocol) != ProtocolAvro.String() {
// protocol other than avro or debezium-avro
protocol := util.GetOrZero(info.Config.Sink.Protocol)
if protocol != ProtocolAvro.String() && protocol != ProtocolDebeziumAvro.String() {
// remove schema registry for MQ downstream with
// protocol other than avro or debezium-avro
protocol, err := ParseSinkProtocolFromString(util.GetOrZero(info.Config.Sink.Protocol))
if err != nil || (protocol != ProtocolAvro && protocol != ProtocolDebeziumAvro) {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/config/changefeed.go` around lines 493 - 496, Normalize the sink protocol
using the existing parsing path before checking whether to clear SchemaRegistry.
In changefeed.go, the logic around util.GetOrZero(info.Config.Sink.Protocol)
should compare the parsed protocol value returned by ParseSinkProtocolFromString
(or an equivalent normalized enum/string) rather than the raw config string, so
aliases like flat-avro are treated as Avro-family and preserved. Update the
conditional in the schema-registry removal block to use the normalized protocol
consistently with ProtocolAvro and ProtocolDebeziumAvro.

info.Config.Sink.SchemaRegistry = nil
}
}
Expand Down
35 changes: 35 additions & 0 deletions pkg/config/changefeed_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,38 @@ func TestChangeFeedInfoToChangefeedConfigBatchFields(t *testing.T) {
assertBatchFields(util.AddressOf(0), util.AddressOf(0))
assertBatchFields(util.AddressOf(123), util.AddressOf(456))
}

func TestChangeFeedInfoRmUnusedFieldsKeepsSchemaRegistryForAvroProtocols(t *testing.T) {
t.Parallel()

tests := []struct {
protocol Protocol
keepRegistry bool
}{
{protocol: ProtocolAvro, keepRegistry: true},
{protocol: ProtocolDebeziumAvro, keepRegistry: true},
{protocol: ProtocolDebezium, keepRegistry: false},
}

for _, tt := range tests {
t.Run(tt.protocol.String(), func(t *testing.T) {
t.Parallel()

cfg := GetDefaultReplicaConfig()
cfg.Sink.Protocol = util.AddressOf(tt.protocol.String())
cfg.Sink.SchemaRegistry = util.AddressOf("http://127.0.0.1:8088")
info := &ChangeFeedInfo{
SinkURI: "kafka://127.0.0.1:9092/topic",
Config: cfg,
}

info.RmUnusedFields()
if tt.keepRegistry {
require.NotNil(t, info.Config.Sink.SchemaRegistry)
require.Equal(t, "http://127.0.0.1:8088", *info.Config.Sink.SchemaRegistry)
} else {
require.Nil(t, info.Config.Sink.SchemaRegistry)
}
})
}
}
5 changes: 3 additions & 2 deletions pkg/config/sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ type SinkConfig struct {
DispatchRules []*DispatchRule `toml:"dispatchers" json:"dispatchers,omitempty"`

ColumnSelectors []*ColumnSelector `toml:"column-selectors" json:"column-selectors,omitempty"`
// SchemaRegistry is only available when the downstream is MQ using avro protocol.
// SchemaRegistry is only available when the downstream is MQ using avro protocol
// or debezium protocol with Confluent Avro encoding.
SchemaRegistry *string `toml:"schema-registry" json:"schema-registry,omitempty"`
// EncoderConcurrency is only available when the downstream is MQ.
EncoderConcurrency *int `toml:"encoder-concurrency" json:"encoder-concurrency,omitempty"`
Expand Down Expand Up @@ -965,7 +966,7 @@ func (s *SinkConfig) ValidateProtocol(scheme string) error {
if s.OpenProtocol != nil {
outputOldValue = s.OpenProtocol.OutputOldValue
}
case ProtocolDebezium:
case ProtocolDebezium, ProtocolDebeziumAvro:
if s.Debezium != nil {
outputOldValue = s.Debezium.OutputOldValue
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/config/sink_protocol.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const (
ProtocolCsv
ProtocolDebezium
ProtocolSimple
ProtocolDebeziumAvro
)

// IsBatchEncode returns whether the protocol is a batch encoder.
Expand Down Expand Up @@ -71,6 +72,8 @@ func ParseSinkProtocolFromString(protocol string) (Protocol, error) {
return ProtocolCsv, nil
case "debezium":
return ProtocolDebezium, nil
case "debezium-avro":
return ProtocolDebeziumAvro, nil
case "simple":
return ProtocolSimple, nil
default:
Expand Down Expand Up @@ -101,6 +104,8 @@ func (p Protocol) String() string {
return "debezium"
case ProtocolSimple:
return "simple"
case ProtocolDebeziumAvro:
return "debezium-avro"
default:
panic("unreachable")
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/config/sink_protocol_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ func TestParseSinkProtocolFromString(t *testing.T) {
protocol: "open-protocol",
expectedProtocolEnum: ProtocolOpen,
},
{
protocol: "debezium-avro",
expectedProtocolEnum: ProtocolDebeziumAvro,
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -109,6 +113,10 @@ func TestString(t *testing.T) {
protocolEnum: ProtocolOpen,
expectedProtocol: "open-protocol",
},
{
protocolEnum: ProtocolDebeziumAvro,
expectedProtocol: "debezium-avro",
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -151,6 +159,10 @@ func TestIsBatchEncoder(t *testing.T) {
protocolEnum: ProtocolOpen,
expect: true,
},
{
protocolEnum: ProtocolDebeziumAvro,
expect: false,
},
}

for _, tc := range testCases {
Expand Down
4 changes: 4 additions & 0 deletions pkg/sink/codec/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ func NewEventEncoder(ctx context.Context, cfg *common.Config) (common.EventEncod
return canal.NewJSONRowEventEncoder(ctx, cfg)
case config.ProtocolDebezium:
return debezium.NewBatchEncoder(cfg, config.GetGlobalServerConfig().ClusterID), nil
case config.ProtocolDebeziumAvro:
return debezium.NewAvroBatchEncoder(ctx, cfg, config.GetGlobalServerConfig().ClusterID)
case config.ProtocolSimple:
return simple.NewEncoder(ctx, cfg)
default:
Expand All @@ -67,6 +69,8 @@ func NewEventDecoder(
return simple.NewDecoder(ctx, codecConfig, upstreamTiDB)
case config.ProtocolDebezium:
return debezium.NewDecoder(codecConfig, idx, upstreamTiDB), nil
case config.ProtocolDebeziumAvro:
return debezium.NewAvroDecoder(ctx, codecConfig, idx, upstreamTiDB)
default:
}
log.Panic("Protocol not supported", zap.Any("Protocol", codecConfig.Protocol))
Expand Down
Loading
Loading