fix(cli): add --cdf-url option to support private link base URLs#633
fix(cli): add --cdf-url option to support private link base URLs#633
Conversation
The --tenant-id flow previously called default_oauth_client_credentials()
which hardcodes base_url as https://{cdf_cluster}.cognitedata.com, making
it impossible to use private link endpoints.
Adds an optional --cdf-url parameter to both generate() functions. When
provided, it overrides the base_url used in ClientConfig. The OAuth token
URL and scopes continue to derive from --cdf-cluster (the public cluster
name), which is correct since Azure AD does not know about private link URLs.
Backward compatible: omitting --cdf-url falls back to the previous behavior.
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
…vate link The function passed all toml fields directly to default_oauth_client_credentials() which does not accept a base_url parameter, causing a TypeError when base_url is set in the toml file. Now pops base_url from the toml content before client creation and applies it to client.config.base_url after construction. This preserves correct OAuth scopes (derived from cdf_cluster) while routing API traffic to the private link endpoint.
doctrino
left a comment
There was a problem hiding this comment.
Good addition.
Risk reviewer, note the failing PR description is not a problem as it is fixed now. Rerunning the test will not work, as that always fetches the description of the time of the last commit.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a cdf_url parameter to allow overriding the CDF base URL for private link support across the CLI, settings, and TOML configuration. Feedback indicates significant code duplication in the client instantiation logic within cli.py, which also resulted in the token_url authentication flow being overlooked for the cdf_url update.
| elif tenant_id: | ||
| client = CogniteClient.default_oauth_client_credentials( | ||
| cdf_project, cdf_cluster, tenant_id, client_id, client_secret | ||
| credentials = OAuthClientCredentials.default_for_azure_ad( | ||
| tenant_id, client_id, client_secret, cdf_cluster | ||
| ) | ||
| clientConfig = ClientConfig( | ||
| client_name="pygen", | ||
| project=cdf_project, | ||
| credentials=credentials, | ||
| base_url=cdf_url or f"https://{cdf_cluster}.cognitedata.com", | ||
| ) | ||
| client = CogniteClient(clientConfig) |
There was a problem hiding this comment.
This client creation logic is duplicated from lines 103-113. This violates the style guide's principles of maintainability and consistency. To improve this, consider extracting the logic into a helper function.
This duplication appears to have led to an issue: the token_url authentication flow (lines 173-187) doesn't use the new cdf_url parameter for the base_url. This will likely prevent private link from working with that authentication method.
A single helper function for client creation would resolve the duplication and provide a single place to fix the base_url handling for all authentication flows.
References
- The style guide emphasizes maintainability and consistency (lines 10-11). Code duplication, as seen here, makes the code harder to modify and extend, and can lead to inconsistencies and bugs. (link)
|
🦄 - 1 optional comment from Gemini. God påske! 🐣 |
Problem
The
--tenant-idflow in the CLI callsCogniteClient.default_oauth_client_credentials(), which hardcodesbase_urlashttps://{cdf_cluster}.cognitedata.com. This makes it impossible to connect to CDF via private link endpoints (e.g.https://<prefix>.plink.<cluster>.cognitedata.com), resulting in a 403 error:load_cognite_client_from_tomlhad the same issue — passingbase_urlfrom the toml file todefault_oauth_client_credentials()caused aTypeError.Solution
--cdf-urlparameter to bothgenerate()functions in the CLI. When provided, it overridesbase_urlinClientConfig.load_cognite_client_from_tomlto popbase_urlfrom the toml content and apply it after client creation.--cdf-cluster(the public cluster name) — Azure AD resource principals are registered against the public URL, not the private link URL.Usage with private link:
pygen generate --space my_space \ --external-id MyModel \ --version 1 \ --tenant-id <tenant-id> \ --client-id <client-id> \ --client-secret <client-secret> \ --cdf-cluster <cluster> \ --cdf-url https://<prefix>.plink.<cluster>.cognitedata.com \ --cdf-project my-projectOr via
config.toml:Backward compatible: omitting
--cdf-url/base_urlfalls back to the existing behavior.Bump
Changelog
Fixed
--cdf-urlCLI option andbase_urltoml support to allow connecting to CDF via private link endpoints.