Repository navigation
Restore keyless GCP authentication in NVIDIA CI - #104
danielmawhirter wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
Change mapNVIDIA's imported workflows still gated Google authentication on the former repository name. This skipped OIDC and exposed a stored-key fallback/identity-switch failure. Follow the original keyless migration in kumo-ai/kumo #29915: enable the intended CI path and prevent explicitly configured workload-exchange tests from silently loading stored keys. The local/deployment fallback remains unchanged. Scope ends at credential helper behavior and the three affected Google CI workflows; cloud IAM changes and unrelated provider migrations are excluded.
All 31 focused credential tests pass, including rejected credentials and synchronous/asynchronous impersonation restoration after failure. Changed-file flake8, isort, YAML lint, and whitespace checks pass. Live OIDC now executes, but the GCP provider rejects federation with Environment selection passes actionlint (shellcheck disabled) and 48 routing cases, including fork exclusion. The five temporarily copied GCP variables were removed from azure-adls-tests; all other settings were preserved. GCP provider trust remains unresolved. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the
Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Change Map Design Review |  |
The required <!-- kumo-pr-change-map --> conversation comment is unavailable. The repository contains only the change-map template and review instructions; the PR description links to an external co… |
Provide the full marked conversation comment, or enable retrieval of PR conversation comments, then review its file rows and rationale against the five changed files and repository behavior. |
-
Check name Status Explanation Pr Size The pull request changes 5 files. This does not exceed the 40-file limit. Public Resource Access The pull request adds the repository reference NVIDIA/kumo-enginein three workflow files:.github/workflows/testing.yml:84,93,.github/workflows/testing_gcs.yml:53, and `.github/workflows/testi…Full details: Public Resource Access
-
Explanation
The pull request adds the repository reference
NVIDIA/kumo-enginein three workflow files:.github/workflows/testing.yml:84,93,.github/workflows/testing_gcs.yml:53, and.github/workflows/testing_snowflake.yml:53. Anonymous requests tohttps://github.com/NVIDIA/kumo-engineand the unauthenticated GitHub API returned HTTP 200. The removedkumo-ai/kumoreferences exist only on the base branch and are excluded by the check.
-
Full details: Change Map Design Review
-
Explanation
The required
<!-- kumo-pr-change-map -->conversation comment is unavailable. The repository contains only the change-map template and review instructions; the PR description links to an external comment but does not include its Change map. The code diff is available, but it cannot establish whether each proposed change was necessary, correctly layered, minimally scoped, and supported by the required verification and stopping condition.Resolution
Provide the full marked conversation comment, or enable retrieval of PR conversation comments, then review its file rows and rationale against the five changed files and repository behavior.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments: 2
test/testing/test_bigquery_credentials.py: 1
170-170: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winTrack
GOOGLE_CREDENTIALS_JSONbefore invoking the helper.
monkeypatchalready restoresGOOGLE_APPLICATION_CREDENTIALS. However,setup_bigquery_credentialsalso writes the distinctGOOGLE_CREDENTIALS_JSONvariable directly. This test does not register that variable, so it can point later connector calls to this test's credential file.🐛 Suggested fix
monkeypatch.delenv('GOOGLE_APPLICATION_CREDENTIALS', raising=False) + monkeypatch.delenv( + testing_bigquery.ENV_GOOGLE_CREDENTIALS_JSON, raising=False) monkeypatch.delenv( testing_bigquery.ENV_BIGQUERY_NO_TABLE_SERVICE_ACCOUNT, raising=False)kumo/testing/bigquery.py: 1
61-64: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not use the pathname as proof that this helper owns the ADC file.
If an external service-account ADC exists at
<cwd>/.google_credential.json, this branch treats it as helper-owned and falls back to a stored key. If the working directory changes after the helper writes the file, the helper-owned file no longer matchesos.getcwd(). A restricted-key call can then select impersonation and fail when its target-account variable is absent.Track credential provenance independently from the current pathname, and cover both cases in the credential-selection tests.
🧹 Nitpick comments: 1
test/testing/test_bigquery_credentials.py: 1
201-204: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClarify the scope of the external ADC test.
test_external_service_account_adcwrites aservice_accountfile. For that type,BigQueryConnector._use_credentialscallsfrom_jsonand does not readGOOGLE_IMPERSONATE_SERVICE_ACCOUNT. Therefore the restricted case can pass while the connector still uses the external service-account credentials.If this test covers helper environment configuration only, state that in its name or docstring. If it covers restricted identity selection, exercise
_use_credentialsand assert the effective credential instead.
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: NVIDIA/kumo-engine
- Branch: dm-bigquery-credential-mode
- File: .coderabbit.yaml
- Commit: 57982bf
- Layers: Repository YAML
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
72da0f1d-9770-4eb9-881e-94c8d80da584
📒 Files selected for processing: 2
kumo/testing/bigquery.pytest/testing/test_bigquery_credentials.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
57982bf to
8336f29
Compare
GitHub Actions
Inactive
danielmawhirter
changed the title
8336f29 to
b2d1cba
Compare
GitHub Actions
Inactive
| """ | ||
| credentials_path = os.getenv('GOOGLE_APPLICATION_CREDENTIALS') | ||
| if not credentials_path or not os.path.exists(credentials_path): | ||
| if os.getenv('GITHUB_ACTIONS') == 'true': |
There was a problem hiding this comment.
Can we decouple the auth mode from where/how the tests run?
E.g. KUMO_GCP_TEST_AUTH_MODE=wif
| testing_bigquery.setup_bigquery_credentials() | ||
|
|
||
|
|
||
| def test_ci_switches_impersonation(monkeypatch, tmp_path, ci_credentials): |
There was a problem hiding this comment.
[P2] New tests leak credential configuration. test_ci_switches_impersonation
This calls a helper that directly writes GOOGLE_CREDENTIALS_JSON, but never registers that variable with monkeypatch. That can contaminate later connector tests. Restore every environment variable the helper mutates through a shared fixture.
b2d1cba to
b1af889
Compare
GitHub Actions
Inactive
b1af889 to
0f11287
Compare
GitHub Actions
Inactive
|
Superseded by #224 (minimal NVIDIA GCP CI enablement) and #225 (explicit, scoped test credentials), following the GCP provider trust update. Those changes preserve the useful authentication and lifecycle fixes without enabling GCP login in the Snowflake workflow. Closing this earlier approach; no separate follow-up from this PR is needed. |
Summary
Restore the intended keyless BigQuery/GCS CI path in NVIDIA/kumo-engine. The imported workflows still restricted Google authentication to
kumo-ai/kumo, so NVIDIA jobs skipped OIDC and reached the stored-key fallback.The original migration, kumo-ai/kumo #29915, replaced CI's stored GCP keys with GitHub OIDC and Workload Identity Federation. Update the repository guards and require federated ADC when
KUMO_GCP_WORKLOAD_EXCHANGE_ENABLEDis enabled, independently of the execution host, including IAM impersonation for restricted permission tests. Missing ADC or stored-key credentials now fail explicitly. The existing local/deployment fallback is unchanged. The main test workflow selectsgha-kumo-workflow-testsfor GCP-enabled shards andazure-adls-testsotherwise, using the same repository, shard, and fork restrictions as the GCP test gate.Change map
File-by-file change map.
Testing Done
unauthorized_client: “The given credential is rejected by the attribute condition.” Provider trust policy requires follow-up; its exact condition has not been inspected. No IAM changes were made, and credentialed integration tests remain unverified.Hotfix Context
Not a hotfix or backport.
Checklist
Environment selection passes actionlint (shellcheck disabled) and 48 routing cases, including fork exclusion. The five temporarily copied GCP variables were removed from azure-adls-tests; all other settings were preserved. GCP provider trust remains unresolved.