Repository navigation
Fix GCS CSEK fallback detection and kill timed-out docker exec processes - #1621
Merged
Merged
Conversation
GCS returns the reason "resourceNotEncryptedWithCustomerEncryptionKey" in lowerCamelCase, isNotEncryptedError matched "ResourceNot..." and never fired. CopyObject of disk_gcs objects with a CSEK-configured destination burned the whole retry backoff per object before falling back to streaming, TestGCS create exceeded the 15m docker exec timeout. Killing the `docker exec` client on timeout left clickhouse-backup running inside the container, so the cleanup `delete local` failed on the pid lock. DockerExecOut now tags the exec process tree with a unique env marker and kills marked processes via /proc/*/environ on timeout. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Report for CI Build 39210683150Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.06%) to 70.265%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions22 previously-covered lines in 6 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When GCS gets a request for a plain (non-CSEK) object using a customer-supplied encryption key, it replies with the reason
resourceNotEncryptedWithCustomerEncryptionKey(lowerCamelCase). Our check looked for the UpperCamelCase spelling, so it never matched. The fallback for objects that aren't encrypted never ran, and those requests failed.Separately, in the integration tests, a
docker execthat timed out only had its local client killed. The process inside the container kept running and could interfere with later tests.Solution
isNotEncryptedErrorinpkg/storage/gcs.gonow compares case-insensitively, so it matches the reason GCS actually returns.DockerExecOutintest/integration/utils.gotags each exec with a unique env variable. If the exec times out, it finds andkill -9s the tagged processes inside the container, then logs what was killed.Testing
TestGCSIsNotEncryptedErrorinpkg/storage/gcs_test.go. It uses a realgoogleapi.Errorwith the lowerCamelCase reason, including a wrapped error. It also checks thatniland an unrelated 404 return false.