这是indexloc提供的服务,不要输入任何密码
Skip to content

feat: Add full support for Universe Domain #1604

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
823d03b
Add test for testing tpc
danieljbruce Mar 12, 2025
3d9822a
Update the test to create an instance
danieljbruce Mar 24, 2025
bdca46b
Remove TPC aspect and bug persists
danieljbruce Mar 24, 2025
d93adfa
More debugging
danieljbruce Mar 24, 2025
52f31af
no universe domain stuff
danieljbruce Mar 24, 2025
a9a1f01
New working test
danieljbruce Mar 24, 2025
0c4e2da
Clean file up
danieljbruce Mar 24, 2025
c441f15
Eliminate arguments
danieljbruce Mar 24, 2025
32fb943
This is okay too
danieljbruce Mar 24, 2025
2caf336
Eliminating the project fetch is okay too
danieljbruce Mar 24, 2025
03b2bd3
Override Bigtable options
danieljbruce Mar 24, 2025
f618252
Trying with an environment variable
danieljbruce Mar 25, 2025
f00d375
Take one of the tests off of only
danieljbruce Mar 25, 2025
94926c8
Add a fix to allow universe domain option
danieljbruce Apr 4, 2025
f5bf88d
Use the bigtable client with the universe domain
danieljbruce Apr 4, 2025
3cc6785
Change zone
danieljbruce May 1, 2025
414cc28
Add the Google Application Credentials
danieljbruce May 1, 2025
7c3c431
run linter
danieljbruce May 1, 2025
30b2c48
should have multiple tests
danieljbruce May 1, 2025
2d4b4a3
Add a third test that uses the universe domain
danieljbruce May 1, 2025
36255ce
Remove env variable from test 3
danieljbruce May 1, 2025
482e922
Use environment variable
danieljbruce May 1, 2025
b0868ce
Better names for the tests
danieljbruce May 1, 2025
e4ccb2e
Adjust options according to client type
danieljbruce May 1, 2025
05a95c2
Add application credentials to the test.
danieljbruce May 1, 2025
b1bea62
Pass universe domain into the gapic clients
danieljbruce May 2, 2025
b52de15
Remove the before hooks
danieljbruce May 2, 2025
a556b40
Better factoring in the test
danieljbruce May 2, 2025
53867bd
remove console logs
danieljbruce May 2, 2025
334904c
Remove only
danieljbruce May 2, 2025
800b55b
Remove unnecessary files
danieljbruce May 2, 2025
e44c1da
Merge everything into the runTest function
danieljbruce May 2, 2025
2cd27dd
Delete file that is not needed
danieljbruce May 2, 2025
3ec698d
back to only
danieljbruce May 2, 2025
0096646
Simplify universeDomainOnly method
danieljbruce May 2, 2025
ee50b02
Set the universe domain for each gapic client
danieljbruce May 2, 2025
07b988e
We actually don’t need to add universeDomain opt
danieljbruce May 2, 2025
8922bc9
Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …
danieljbruce May 2, 2025
69484f1
Skip TPC tests in the test suite
danieljbruce May 2, 2025
ab8c15d
Add header
danieljbruce May 2, 2025
de5c7b6
Remove the service-path file
danieljbruce May 2, 2025
b7115bb
ran the linter
danieljbruce May 2, 2025
31f7274
Create a getUniverseDomainOptions function
danieljbruce May 2, 2025
cbff5f5
Remove my credentials
danieljbruce May 2, 2025
60461b3
Add documentation for methods
danieljbruce May 2, 2025
9a55cce
service-path from main
danieljbruce May 2, 2025
3cb4820
Fix the service path tests
danieljbruce May 2, 2025
024027d
catch error
danieljbruce May 2, 2025
35ff346
Remove only
danieljbruce May 2, 2025
00396b6
Call the slice function
danieljbruce May 2, 2025
f12b686
Revert "Remove my credentials"
danieljbruce May 2, 2025
a518dff
Revert "Revert "Remove my credentials""
danieljbruce May 2, 2025
8fb5b32
Rename variables to gaxOpts
danieljbruce May 14, 2025
d16758c
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] May 14, 2025
761ccd2
Specify return type
danieljbruce May 14, 2025
56b2331
Merge branch 'universe-domain-service-path-ordinary-new-fix-one-branc…
danieljbruce May 14, 2025
001aeaf
Replace with universe domain
danieljbruce May 14, 2025
e5993ab
Revert "Merge branch 'universe-domain-service-path-ordinary-new-fix-o…
danieljbruce May 14, 2025
5418648
Make runTest async
danieljbruce May 14, 2025
952fc80
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] May 14, 2025
bea59c3
Revert "Revert "Merge branch 'universe-domain-service-path-ordinary-n…
danieljbruce May 14, 2025
8c8b988
Merge branch 'universe-domain-service-path-ordinary-new-fix-one-branc…
danieljbruce May 14, 2025
5a989a4
Gapic client should take priority
danieljbruce May 26, 2025
96d69ec
turn on the test
danieljbruce May 26, 2025
3a058b6
Update tests with better instructions
danieljbruce May 26, 2025
524ae61
Update protos
danieljbruce May 26, 2025
76e8b65
🦉 Updates from OwlBot post-processor
gcf-owl-bot[bot] May 26, 2025
7444993
comment application credentials
danieljbruce May 26, 2025
23c52b4
Merge branch 'universe-domain-service-path-ordinary-new-fix-one-branc…
danieljbruce May 26, 2025
b88c9b7
Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …
danieljbruce May 28, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions protos/protos.d.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 7 additions & 42 deletions protos/protos.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

92 changes: 76 additions & 16 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,28 +104,81 @@ export interface BigtableOptions extends gax.GoogleAuthOptions {
}

/**
* Retrieves the domain to be used for the service path.
* Retrieves the universe domain, if configured.
*
* This function retrieves the domain from gax.ClientOptions passed in or via an environment variable.
* It defaults to 'googleapis.com' if none has been set.
* @param {string} [prefix] The prefix for the domain.
* @param {gax.ClientOptions} [opts] The gax client options
* @returns {string} The universe domain.
* This function checks for a universe domain in the following order:
* 1. The `universeDomain` property within the provided options.
* 2. The `universeDomain` or `universe_domain` property within the `opts` object.
* 3. The `GOOGLE_CLOUD_UNIVERSE_DOMAIN` environment variable.
*
* If a universe domain is found in any of these locations, it is returned.
* Otherwise, the function returns `undefined`.
*
* @param {BigtableOptions} options - The Bigtable client options.
* @param {gax.ClientOptions} [gaxOpts] - Optional gax client options.
* @returns {string | undefined} The universe domain, or `undefined` if not found.
*/
function getDomain(prefix: string, opts?: gax.ClientOptions) {
function getUniverseDomainOnly(
options: BigtableOptions,
gaxOpts?: gax.ClientOptions,
): string | undefined {
// From https://github.com/googleapis/nodejs-bigtable/blob/589540475b0b2a055018a1cb6e475800fdd46a37/src/v2/bigtable_client.ts#L120-L128.
// This code for universe domain was taken from the Gapic Layer.
// It is reused here to build the service path.
const universeDomainEnvVar =
typeof process === 'object' && typeof process.env === 'object'
? process.env['GOOGLE_CLOUD_UNIVERSE_DOMAIN']
: undefined;
return `${prefix}.${
opts?.universeDomain ??
opts?.universe_domain ??
universeDomainEnvVar ??
'googleapis.com'
}`;
return (
gaxOpts?.universeDomain ??
gaxOpts?.universe_domain ??

Choose a reason for hiding this comment

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

Why do we check the snake-case universe_domain for the gaxOpts but not for the options opts?

Copy link

Choose a reason for hiding this comment

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

This something that Alex decided on a while back. I'm not entirely sure why we do it, but we do it in generated gapic clients too, so it's in all of the libraries. (At least the ones I've seen.)

Choose a reason for hiding this comment

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

Should it be added to options then as well? e.g.

  return (
    options?.universeDomain ??
    options?.universe_domain ??
    gaxOpts?.universeDomain ??
    gaxOpts?.universe_domain ??
    universeDomainEnvVar ??
 }

Or should snake-case be marked deprecated so that newer clients (e.g. this one) can ignore it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the BigtableOptions passed into the handwritten layer at https://github.com/googleapis/nodejs-bigtable/blob/main/src/index.ts#L82 extend GoogleAuthOptions and only support camel case according to the screenshot:
image

But the universe domain for the Gapic ClientOptions type supports both camel case and snake case:

image

Copy link

Choose a reason for hiding this comment

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

I have a suspicion that the reason we did this is that an earlier version of the spec wanted snake case, and then we decided to make it camel case to match everything else in Node. But the person who wrote it has left, so I'm not 100% sure. Let's see if there is a bread crumb trail...

...Unfortunately no. It looks like it was one big commit without further explanation.

options?.universeDomain ??
universeDomainEnvVar
);
}

/**
* Retrieves the universe domain options from the provided options.
*
* This function examines the provided BigtableOptions and an optional
* gax.ClientOptions object to determine the universe domain to be used.
* It prioritizes the `universeDomain` property in the options, then checks
* for `universeDomain` or `universe_domain` in the gax options, and finally
* falls back to the `GOOGLE_CLOUD_UNIVERSE_DOMAIN` environment variable.
* If a universe domain is found, it returns an object containing the
* `universeDomain` property; otherwise, it returns `null`.

Choose a reason for hiding this comment

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

it may simplify things to have universeDomain never be null, and always default to googleapis.com when set. Although, because you have a specific getUniverseDomainOnly method, I am assuming you have a specific reason for doing it this way? If so, what is it?

Copy link
Contributor Author

@danieljbruce danieljbruce May 14, 2025

Choose a reason for hiding this comment

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

The reason getUniverseDomainOptions returns null is that the return value is used in an Object.assign call. The behaviour of Object.assign is that it does nothing to the object being assigned, in this case, the Gapic client options if the object being assigned to it is null.

So the behaviour we want that we are getting with this code is don't modify the gapic client options when null gets returned.

*
* @param {BigtableOptions} options - The Bigtable client options.
* @param {gax.ClientOptions} [gaxOpts] - Optional gax client options.
* @returns {{universeDomain: string} | null} An object containing the `universeDomain` property if found,
* otherwise `null`.
*/
function getUniverseDomainOptions(
options: BigtableOptions,
gaxOpts?: gax.ClientOptions,
): {universeDomain: string} | null {
const universeDomainOnly = getUniverseDomainOnly(options, gaxOpts);
return universeDomainOnly ? {universeDomain: universeDomainOnly} : null;
}

/**
* Retrieves the domain to be used for the service path.
*
* This function retrieves the domain from gax.ClientOptions passed in or via an environment variable.
* It defaults to 'googleapis.com' if none has been set.
* @param {string} [prefix] The prefix for the domain.
* @param {BigtableOptions} [options] The options passed into the Bigtable client.
* @param {gax.ClientOptions} [gaxOpts] The gax client options.
* @returns {string} The universe domain.
*/
function getDomain(
prefix: string,
options: BigtableOptions,
gaxOpts?: gax.ClientOptions,
): string {
const universeDomainOnly = getUniverseDomainOnly(options, gaxOpts);
const suffix = universeDomainOnly ? universeDomainOnly : 'googleapis.com';
return `${prefix}.${suffix}`;
}

/**
Expand Down Expand Up @@ -467,10 +520,11 @@ export class Bigtable {
const dataOptions = Object.assign(
{},
baseOptions,
getUniverseDomainOptions(options, options.BigtableClient),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, ie. without providing a universeDomain to the gapic client we see an error that says: Error: The configured universe domain (googleapis.com) does not match the universe domain found in the credentials (apis-tpczero.goog). If you haven't configured the universe domain explicitly, googleapis.com is the default.

Choose a reason for hiding this comment

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

Why not just be opinionated here and pass in getDomain?

Choose a reason for hiding this comment

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

You don't need to pass the universe domain option through with the Env Var because that'll be done by the GAPICs. Even though you need it to calculate the servicePath, you don't need to set it as the UniverseDomain option as well.

I would suggest taking the path of either 1. "configure the bare minimum and let the GAPICs do the rest" or 2. "always explicitly set it so the GAPICs do nothing". So this would be, either always ensure the universeDomain option is set (make it googleapis.com if not), or make it minimally set (so that it would be null unless explicitly configured).

  1. getUniverseDomain always returns a string, and is used in getDomain to append the service name
  2. getUniverseDomainOption only returns a string if a universe domain option was explicitly configured in the client options or GAPIC client options

Copy link
Contributor Author

@danieljbruce danieljbruce May 26, 2025

Choose a reason for hiding this comment

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

getDomain returns a prefix and a suffix when we only want the suffix so passing in getDomain doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to choose option 1 because I want getUniverseDomainOptions to return null if no universe domain settings are provided by the user. This makes it so that the Gapic client options remain unchanged for all users that are not using universe domains. This is the majority of our users and the code behaviour should remain unchanged for these users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For your suggestion 2, if the user provides the universe domain in an environment variable and it doesn't get provided to the Gapic client in universe_domain then I actually get an error. It says Error: 16 UNAUTHENTICATED: Request had invalid authentication credentials. Expected OAuth 2 access token, login cookie or other valid authentication credential.. So we really need to provide a universe domain option if a universe domain option was explicitly configured in the client options or GAPIC client options OR AN ENVIRONMENT VARIABLE.

image

{
servicePath:
customEndpointBaseUrl ||
getDomain('bigtable', options.BigtableClient),
getDomain('bigtable', options, options.BigtableClient),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client.

Choose a reason for hiding this comment

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

What does this look like when the user provides universe domain in the GAPIC client but not in the handwritten client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it would be set to bigtable.${universeDomain} where universeDomain is the value provided in the Gapic client.

'grpc.callInvocationTransformer': grpcGcp.gcpCallInvocationTransformer,
'grpc.channelFactoryOverride': grpcGcp.gcpChannelFactoryOverride,
'grpc.gcpApiConfig': grpcGcp.createGcpApiConfig({
Expand All @@ -488,20 +542,26 @@ export class Bigtable {
const adminOptions = Object.assign(
{},
baseOptions,
getUniverseDomainOptions(options, options.BigtableTableAdminClient),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, ie. without providing a universeDomain to the gapic client we see an error that says: Error: The configured universe domain (googleapis.com) does not match the universe domain found in the credentials (apis-tpczero.goog). If you haven't configured the universe domain explicitly, googleapis.com is the default.

{
servicePath:
customEndpointBaseUrl ||
getDomain('bigtableadmin', options.BigtableClient),
getDomain('bigtableadmin', options, options.BigtableTableAdminClient),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this should be BigtableTableAdminClient as this is a setting for the adminOptions.

},
options,
);
const instanceAdminOptions = Object.assign(
{},
baseOptions,
getUniverseDomainOptions(options, options.BigtableInstanceAdminClient),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, ie. without providing a universeDomain to the gapic client we see an error that says: Error: The configured universe domain (googleapis.com) does not match the universe domain found in the credentials (apis-tpczero.goog). If you haven't configured the universe domain explicitly, googleapis.com is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this should be BigtableInstanceAdminClient as this is a setting for the instance admin client.

{
servicePath:
customEndpointBaseUrl ||
getDomain('bigtableadmin', options.BigtableClient),
getDomain(
'bigtableadmin',
options,
options.BigtableInstanceAdminClient,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now supports the use case where the universe domain is provided in the handwritten client options as well as maintaining support for when the universe domain is provided just for the gapic client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this should be BigtableInstanceAdminClient as this is a setting for the instance admin client.

},
options,
);
Expand Down
Loading
Loading