fix(clickhouse): check for S3 grant before validating CH-bound pipe - #4629
fix(clickhouse): check for S3 grant before validating CH-bound pipe#4629itsbilal wants to merge 2 commits into
Conversation
15c5f9c to
3b5b4a6
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Code reviewBug: S3 grant check runs unconditionally for all staging types — The new S3 grant validation loop runs unconditionally, but staging can also be GCS (see For a GCS-staging deployment where the ClickHouse user lacks S3 grants (which is valid), This check should be conditional on the staging provider being S3. |
3b5b4a6 to
85d539f
Compare
|
Marking as draft; I'm trying to refactor this code in such a way that it's easily reusable by the GCS side without making the imports too annoying. Should be done very soon. |
❌ 8 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
🔄 Flaky Test DetectedAnalysis: The MySQL_CH binlog test failed during connection setup with a transient TCP handshake EOF ("connection was bad") to the MySQL container—an infrastructure flake unrelated to the S3-grants PR, and only 1 of 3 matrix jobs was affected. ✅ Automatically retrying the workflow |
85d539f to
4c48db8
Compare
❌ Test FailureAnalysis: Not flaky — the build fails to compile in flow/connectors/clickhouse because the StagingValidator API gained logger/ClickhouseGrantChecker parameters but clickhouse.go:85, staging_gcs.go:126 and staging_s3.go:214 still use the old signatures, so no tests ran in any matrix job. |
14f776a to
9bd1178
Compare
❌ Test FailureAnalysis: Real bug: TestCancelTableAdditionDuringSetupFlow failed identically in all 4 API suites across all 3 matrix jobs at cancel_table_addition_test.go:1271 with |
❌ Test FailureAnalysis: Not flaky — the PR's new S3 grants check makes peer validation fail deterministically, so |
|
Test failure aside, this is ready for a review. |
Previously, if custom roles were selected instead of all access when creating a pipe, we did not validate that the ClickHouse user can read from the staging S3 bucket in the final step of ingestion. This would result in pipes getting stuck in provisioning without an obvious user-facing error. This change updates the ValidateClickHousePeer function to also run a CHECK GRANT that's as version-agnostic as possible, to confirm if the user can actually read from S3. If the CHECK GRANT returns a 0, we throw a validation failure.
9bd1178 to
a341317
Compare
| if err := validateBucketGrant(ctx, log, checker, "S3"); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I don't think we want to thread the clickhouse permission check here thru the objectstore validators. In Discovery today we do not use the s3/gcs validator, this was intentional because s3 is considered internal infra, whereas discovery validation is intended for validating user setup. So adding it here would have it silently skipped today.
Instead, can call the check directly inside ValidateClickHousePeer:
if stagingBucketIsS3 {
if err := validateS3ReadGrant(ctx, logger, conn); err != nil {
return err
}
}
One more NIT is when i read:
if err := validateBucketGrant(ctx, log, checker, "GCS"); err != nil {
return err
}
i assumed that we were trying to check READ ON GCS until i saw:
if checker == nil {
return nil
}
I find this a bit implicit and it's better to just hard-code it to S3 given that's all we care about right now.
There was a problem hiding this comment.
Agreed with moving to ValidateClickHousePeer and the duplication between checker and the object. For GCS, we actually need to validate READ ON URL instead, it's a separate grant that's not guaranteed
There was a problem hiding this comment.
If we do need a validation in the GCS case (CHECK GRANT READ on URL) should we keep the broad structure the way it is right now, remove the if checker == nil { return nil } , and just have the gcs path also call validateBucketGrant correctly?
I think that'd be cleanest, but also happy to move the logic to ValidateClickHousePeer if y'all think that's better - only reason I didn't want to encode it there is because it feels off-place to have an S3/GCS switch there when we already delegate S3/GCS specific logic to the appropriate StagingStore.
There was a problem hiding this comment.
I've made this change - let me know how you feel about this approach. Also happy to move this logic out of StagingStore entirely and move it in ValidateClickHousePeer if that's still preferred.
There was a problem hiding this comment.
move the logic to ValidateClickHousePeer
To clarify, today in Discovery we use NoStagingValidator, not S3StagingValidator/GCSStagingValidator, so validateBucketGrant would never be called here; which is why moving it to ValidateClickHousePeer might make more sense.
There was a problem hiding this comment.
it feels off-place to have an S3/GCS switch there when we already delegate S3/GCS specific logic to the appropriate StagingStore
if there's a better way to introduce just the s3/gcp perm check logic into the staging store and only only trigger that path, that works too. (I don't feel too strong either way, because currently the validator check write perm using the object storage client; vs. the read perm check you introduced is purely a clickhouse permission check; so i can see it belong to either)
There was a problem hiding this comment.
There are two kinds of staging store validations: can PeerDB access the store and does CH user have permissions to do read queries. The first one is only relevant in OSS, as people would do ground-up S3 setups or self-host MinIO, and in our managed environment S3 is set up uniformly and the service doing the validation can't even access it. Only the second one is relevant there.
Reading through the code again, I realized that the whole StagingValidator infra was a result of a misunderstanding of how it all works e2e, and it doesn't need to be in pkg. This must've been confusing to read and try to align with, submitted a cleanup in #4643. ValidateClickHousePeer would lose the last arg, and you'd be able to cleanly add the "S3"/"URL" (or a boolean/enum) instead.
Hopefully now it's clearer how we don't really need extra abstractions besides doing a CH query and validating it with various kinds of users in real CH.
| if err := validateBucketGrant(ctx, log, checker, "S3"); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Agreed with moving to ValidateClickHousePeer and the duplication between checker and the object. For GCS, we actually need to validate READ ON URL instead, it's a separate grant that's not guaranteed
| chHost, strings.Join(allowedDomains, ",")) | ||
| } | ||
|
|
||
| type grantChecker struct { |
There was a problem hiding this comment.
Was trying to understand the need for more abstractions here and in the tests, and realized it's because we want to test something but the package doesn't have the integration test infra hooked up to do proper CH calls and keep the actual validation change to 1-2 simple functions. We only sorted out the test infra in the recent couple of months, so the only reason it's not simple is historical. Added scaffolding in #4639, let's make the tests set up an actual user with/without grants and see how the validation responds.
9e2009a to
bb6ec66
Compare
🔄 Flaky Test DetectedAnalysis: Two unrelated timing races in separate matrix legs — a MySQL testcontainer accepted a connection before mysqld finished handshake init, and a Temporal ListWorkflow assertion read the eventually-consistent visibility store with no retry — neither related to the PR's S3-grants change, and the third matrix leg passed. ✅ Automatically retrying the workflow |
3 participants
Previously, if custom roles were selected instead of all access when creating a pipe, we did not validate that the ClickHouse user can read from the staging S3 bucket in the final step of ingestion. This would result in pipes getting stuck in provisioning without an obvious user-facing error.
This change updates the ValidateClickHousePeer function to also run a CHECK GRANT that's as version-agnostic as possible, to confirm if the user can actually read from S3. If the CHECK GRANT returns a 0, we throw a validation failure.