Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gcs): add property skip_credential #847

Merged
merged 3 commits into from
Feb 22, 2024

Conversation

sanadhis
Copy link
Contributor

@sanadhis sanadhis commented Feb 21, 2024

Follow-up on #837

Turns out specifying no credential is not equal with adding option.WithoutAuthentication() to gcloud SDK.

My setup is to have this clickhouse-backup runs as side container next to Clickhouse, configured with k8s service account that has proper workload-identity-federation in GKE, see here.

If we set option.WithoutAuthentication(), ADC wouldn't even bother to ask metadata server, which makes the auth fails for K8s service account. See here

error observed in k8s:

2024/02/21 18:27:12.908155  info SELECT macro, substitution FROM system.macros logger=clickhouse
2024/02/21 18:27:12.994845  warn BackupList bd.Walk return error: googleapi: Error 401: Anonymous caller does not have storage.objects.list access to the Google Cloud Storage bucket. Permission 'storage.objects.list' denied on resource (or it may not exist)., required logger=gcs

test from local:

2024/02/21 20:30:36.631375  info docker exec clickhouse ls -1 /var/lib/clickhouse/backup/*TestIntegrationGCSWithCustomEndpoint*
2024/02/21 20:30:36.759534  info Drop all databases       
2024/02/21 20:30:36.912380  info docker exec gcs sh -c ls -lh /data/altinity-qa-test/
--- PASS: TestIntegrationGCSWithCustomEndpoint (79.47s)
PASS
ok      command-line-arguments  79.848s

@sanadhis sanadhis force-pushed the feat-gcs-new-skip-credential-property branch from bac4f08 to b53b871 Compare February 21, 2024 20:14
@Slach
Copy link
Collaborator

Slach commented Feb 22, 2024

to fix tests please run

# carefull it delete all containers
for id in $(docker ps -a -q); do docker stop "$id"; docker rm -f "$id"; done;
rm -fv ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
cp -fv ./test/testflows/.env.example ./test/testflows/.env
# provide actual credentials
mcedit ./test/testflows/.env 
RUN_TESTS="/clickhouse backup/cli/default config" ./test/testflows/run.sh
git add ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
git commit -s -m "update snapshot"
git push

@Slach Slach self-requested a review February 22, 2024 04:52
@sanadhis
Copy link
Contributor Author

to fix tests please run

# carefull it delete all containers
for id in $(docker ps -a -q); do docker stop "$id"; docker rm -f "$id"; done;
rm -fv ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
cp -fv ./test/testflows/.env.example ./test/testflows/.env
# provide actual credentials
mcedit ./test/testflows/.env 
RUN_TESTS="/clickhouse backup/cli/default config" ./test/testflows/run.sh
git add ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
git commit -s -m "update snapshot"
git push

Hey, the test pass suites pass but doesn't re-generate ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot

Passing

✔ [ OK ] /clickhouse backup/cli (166ms)
✔ [ OK ] /clickhouse backup (50s 758ms)

Coverage

QA-SRS013 ClickHouse Backup Utility
  72 requirements (2 satisfied 2.8%, 70 untested 97.2%)

1 suite (1 ok)
2 scenarios (2 ok)
43 steps (43 ok)

Total time 50s 758ms

Executed on Feb 22,2024 7:50
TestFlows.com Open-Source Software Testing Framework v1.9.230705.1122619

I am not sure if it's okay to remove this ./test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot

@Slach
Copy link
Collaborator

Slach commented Feb 22, 2024

you deleted cli.py.cli.snapshot instead of patch it
try just run ./test/testflows/run.sh

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8000810025

Details

  • -1 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 240 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-2.0%) to 64.464%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/storage/gcs.go 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/backup/delete.go 1 65.14%
cmd/clickhouse-backup/main.go 2 72.22%
pkg/backup/create.go 2 74.16%
pkg/backup/list.go 2 58.74%
pkg/clickhouse/clickhouse.go 2 77.02%
pkg/backup/restore.go 3 68.66%
pkg/storage/object_disk/object_disk.go 5 70.95%
pkg/status/status.go 9 70.2%
pkg/config/config.go 10 70.96%
pkg/storage/general.go 14 58.23%
Totals Coverage Status
Change from base Build 7986788512: -2.0%
Covered Lines: 7795
Relevant Lines: 12092

💛 - Coveralls

@@ -1,6 +0,0 @@
default_config = r"""'[\'general:\', \' remote_storage: none\', \' disable_progress_bar: true\', \' backups_to_keep_local: 0\', \' backups_to_keep_remote: 0\', \' log_level: info\', \' allow_empty_backups: false\', \' use_resumable_state: true\', \' restore_schema_on_cluster: ""\', \' upload_by_part: true\', \' download_by_part: true\', \' restore_database_mapping: {}\', \' retries_on_failure: 3\', \' retries_pause: 30s\', \' watch_interval: 1h\', \' full_interval: 24h\', \' watch_backup_name_template: shard{shard}-{type}-{time:20060102150405}\', \' sharded_operation_mode: ""\', \' cpu_nice_priority: 15\', \' io_nice_priority: idle\', \' retriesduration: 100ms\', \' watchduration: 1h0m0s\', \' fullduration: 24h0m0s\', \'clickhouse:\', \' username: default\', \' password: ""\', \' host: localhost\', \' port: 9000\', \' disk_mapping: {}\', \' skip_tables:\', \' - system.*\', \' - INFORMATION_SCHEMA.*\', \' - information_schema.*\', \' - _temporary_and_external_tables.*\', \' skip_table_engines: []\', \' timeout: 5m\', \' freeze_by_part: false\', \' freeze_by_part_where: ""\', \' use_embedded_backup_restore: false\', \' embedded_backup_disk: ""\', \' backup_mutations: true\', \' restore_as_attach: false\', \' check_parts_columns: true\', \' secure: false\', \' skip_verify: false\', \' sync_replicated_tables: false\', \' log_sql_queries: true\', \' config_dir: /etc/clickhouse-server/\', \' restart_command: exec:systemctl restart clickhouse-server\', \' ignore_not_exists_error_during_freeze: true\', \' check_replicas_before_attach: true\', \' tls_key: ""\', \' tls_cert: ""\', \' tls_ca: ""\', \' debug: false\', \'s3:\', \' access_key: ""\', \' secret_key: ""\', \' bucket: ""\', \' endpoint: ""\', \' region: us-east-1\', \' acl: private\', \' assume_role_arn: ""\', \' force_path_style: false\', \' path: ""\', \' object_disk_path: ""\', \' disable_ssl: false\', \' compression_level: 1\', \' compression_format: tar\', \' sse: ""\', \' sse_kms_key_id: ""\', \' sse_customer_algorithm: ""\', \' sse_customer_key: ""\', \' sse_customer_key_md5: ""\', \' sse_kms_encryption_context: ""\', \' disable_cert_verification: false\', \' use_custom_storage_class: false\', \' storage_class: STANDARD\', \' custom_storage_class_map: {}\', \' part_size: 0\', \' allow_multipart_download: false\', \' object_labels: {}\', \' request_payer: ""\', \' check_sum_algorithm: ""\', \' debug: false\', \'gcs:\', \' credentials_file: ""\', \' credentials_json: ""\', \' credentials_json_encoded: ""\', \' bucket: ""\', \' path: ""\', \' object_disk_path: ""\', \' compression_level: 1\', \' compression_format: tar\', \' debug: false\', \' force_http: false\', \' endpoint: ""\', \' storage_class: STANDARD\', \' object_labels: {}\', \' custom_storage_class_map: {}\', \'cos:\', \' url: ""\', \' timeout: 2m\', \' secret_id: ""\', \' secret_key: ""\', \' path: ""\', \' compression_format: tar\', \' compression_level: 1\', \' debug: false\', \'api:\', \' listen: localhost:7171\', \' enable_metrics: true\', \' enable_pprof: false\', \' username: ""\', \' password: ""\', \' secure: false\', \' certificate_file: ""\', \' private_key_file: ""\', \' ca_cert_file: ""\', \' ca_key_file: ""\', \' create_integration_tables: false\', \' integration_tables_host: ""\', \' allow_parallel: false\', \' complete_resumable_after_restart: true\', \'ftp:\', \' address: ""\', \' timeout: 2m\', \' username: ""\', \' password: ""\', \' tls: false\', \' skip_tls_verify: false\', \' path: ""\', \' object_disk_path: ""\', \' compression_format: tar\', \' compression_level: 1\', \' debug: false\', \'sftp:\', \' address: ""\', \' port: 22\', \' username: ""\', \' password: ""\', \' key: ""\', \' path: ""\', \' object_disk_path: ""\', \' compression_format: tar\', \' compression_level: 1\', \' debug: false\', \'azblob:\', \' endpoint_schema: https\', \' endpoint_suffix: core.windows.net\', \' account_name: ""\', \' account_key: ""\', \' sas: ""\', \' use_managed_identity: false\', \' container: ""\', \' path: ""\', \' object_disk_path: ""\', \' compression_level: 1\', \' compression_format: tar\', \' sse_key: ""\', \' buffer_size: 0\', \' buffer_count: 3\', \' timeout: 4h\', \' debug: false\', \'custom:\', \' upload_command: ""\', \' download_command: ""\', \' list_command: ""\', \' delete_command: ""\', \' command_timeout: 4h\', \' commandtimeoutduration: 4h0m0s\']'"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

please rollback these changes

@Slach
Copy link
Collaborator

Slach commented Feb 22, 2024

ok. let me merge your changes, will restore snapshot from my side

@Slach Slach merged commit 28aaea0 into Altinity:master Feb 22, 2024
17 of 18 checks passed
@Slach Slach added this to the 2.4.33 milestone Feb 22, 2024
@sanadhis
Copy link
Contributor Author

@Slach thanks a lot! Sorry for the regression. Appreciate your fast response.

@sanadhis sanadhis deleted the feat-gcs-new-skip-credential-property branch February 22, 2024 09:31
Slach added a commit that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants