fix(clickhouse sink)!: make skip_unknown_fields optional

[PriceHiller]

Summary

Copied from the body of the commit:

Problem: The Clickhouse sink's skip_unknown_fields doesn't follow the Clickhouse server default. It always sends a value for skip_unknown_fields; furthermore, there is also no way to disable skip_unknown_fields setting a "strict" mode for Clickhouse. We really want to permit either a default value from the Clickhouse server, meaning we shouldn't specify skip_unknown_fields by default. Otherwise, if a user wants to specify the strict mode for unknown fields, they should then pass either true or false for click house.

Solution: Change the skip_unknown_fields value to be of an Option<bool> instead of just a bool. This permits using the defaults provided by the Clickhouse server and doesn't send the skip_unknown_fields value to the server if left unspecified.

See #22013 for the original issue report.

Closes #22013

Allow the skip_unknown_fields setting to be optional, thereby allowing use of the defaults provided by the ClickHouse server. Setting it to true will permit skipping unknown fields and false will make ClickHouse strict on what fields it accepts.

NOTE: I am unfamiliar with ClickHouse and Vector's code as a whole, so it is very possible I have missed something.

Also, does this qualify as a Bug fix or a New feature? Technically this does add functionality that didn't exist previously, but does so by encoding a fix.

Change Type

• Bug fix
• New feature
• Non-functional (chore, refactoring, docs)
• Performance

Is this a breaking change?

• Yes
• No

How did you test this PR?

By running:

1. cargo test --lib --no-default-features --features "sinks-console,sinks-clickhouse,clickhouse-integration-tests" 'sinks::clickhouse::service'
2. cargo test --lib --no-default-features --features "sinks-console,sinks-clickhouse,clickhouse-integration-tests" 'sinks::clickhouse::config'
3. make test-integration-clickhouse

Does this PR include user facing changes?

• Yes. Please add a changelog fragment based on our guidelines.
• No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

• Please read our Vector contributor resources.
• If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
  run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

• Closes: Clickhouse skip_unknown_fields has no effect #22013

[PriceHiller]

Modified the changelog from fix -> breaking.

[pront]

Modified the changelog from fix -> breaking.

I wonder if we can avoid a breaking change, if we set it to Some(false) by default. Which is what we have today: https://vector.dev/docs/reference/configuration/sinks/clickhouse/#skip_unknown_fields

[PriceHiller]

I wonder if we can avoid a breaking change, if we set it to Some(false) by default. Which is what we have today: https://vector.dev/docs/reference/configuration/sinks/clickhouse/#skip_unknown_fields

The issue is that the false value wasn't actually doing the right thing. It should have put ClickHouse into "strict" mode and reject any unknown fields for that request.

Instead, Vector was incorrectly using the false value to not send input_format_skip_unknown_fields at all and instead rely on the ClickHouse's server setting default.

See

vector/src/sinks/clickhouse/service.rs

Lines 138 to 140 in 0b801eb

	if skip_unknown {
	uri.push_str("input_format_skip_unknown_fields=1&");
	}

What needs to happen is if skip_unknown_fields is specified at all we need to set the request's preference on whether or not to skip the unknown fields, ignoring the server's default setting for that.

There's 3 cases:

1. skip_unknown_fields is true: we need to allow skipping unknown fields, overriding the server's default
2. skip_unknown_fields is false: we need to enable "strict" mode and not allow skipping unknown fields for the request, overriding the server's default
3. skip_unknown_fields is None: we need to follow the server's default by not specifying input_format_skip_unknown_fields

At least, that's based on my understanding of #22013. If I'm in error, I'll gladly make the change, that's just my interpretation.

[jszwedko]

Awesome, thank you @PriceHiller . This is a breaking change, but I'd argue that the behavior you have here is what users would have reasonably expected: if they explicitly set false it should set input_format_skip_unknown_fields=0.

[PriceHiller]

Sorry about the delay, just updated the cue files on the latest force push.