feat: Add full support for Universe Domain by danieljbruce · Pull Request #1604 · googleapis/nodejs-bigtable

danieljbruce · 2025-05-02T17:17:43Z

Description

Adds local testing for all universe domain use cases with a service account. Adds support for universeDomain option passed into the handwritten client. Passes universeDomain to the Gapic client to solve the Error: The configured universe domain (googleapis.com) does not match the universe domain found in the credentials (apis-tpczero.goog) error.

Also, an upstream change from google-gax is now providing more information in errors so some assert statements needed to be slightly modified.

Impact

Fixes all universe domain use case and provides code so that we can continue to validate it going forward.

Testing

Added tests to be run locally with the service account credentials for the project with the universe domain. Adding new service account credentials so that this can be tested in the CI pipeline is out of scope.

Additional Information

An Alternative was considered where we only pass a service path into the gapic client when a custom API is provided, but that produced an error that says Error: 14 UNAVAILABLE: Name resolution failed for target dns:null:443 before any response was received..

to be passed in

bshaffer

I am a bit confused by these changes.

It seems like for the GAPIC clients, the universe domain needs to be set in two places - in the servicePath and the options. Why not just let the GAPIC determine their own servicePath? If this is just keeping with the existing paradigm, then that's fine
The adminOptions and instanceAdminOptions are formed by merging the options from the existing BigtableTableAdminClient and BigtableInstanceAdminClient. Why? It seems circular to me - we create the options for those GAPICs and then save the options back from the GAPICs?

bshaffer · 2025-05-14T17:57:02Z

src/index.ts

+ * 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`.


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?

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.

bshaffer · 2025-05-14T17:59:32Z

src/index.ts

        servicePath:
          customEndpointBaseUrl ||
-          getDomain('bigtable', options.BigtableClient),
+          getDomain('bigtable', options, options.BigtableClient),


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

bshaffer · 2025-05-14T18:00:23Z

src/index.ts

+    options?.universeDomain ??
+    gaxOpts?.universeDomain ??
+    gaxOpts?.universe_domain ??
+    universeDomainEnvVar


what is the difference between options.universeDomain and gaxOpts.universeDomain from a user perspective? What does it look like when one overrides the other?

gaxOpts is the settings the user provided for the specific Gapic client and options is the options provided to the handwritten layer. We discussed offline that we should really allow the gapic options to take priority over the handwritten layer options so I can apply the change to make gaxOpts?.universeDomain come first.

Ok. gaxOpts?.universeDomain comes first now

bshaffer · 2025-05-14T18:01:15Z

src/index.ts

    const dataOptions = Object.assign(
      {},
      baseOptions,
+      getUniverseDomainOptions(options, options.BigtableClient),


Why not just be opinionated here and pass in getDomain?

bshaffer · 2025-05-14T19:04:12Z

src/index.ts

+  return (
+    options?.universeDomain ??
+    gaxOpts?.universeDomain ??
+    gaxOpts?.universe_domain ??


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

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.)

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?

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:

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

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.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

danieljbruce · 2025-05-26T15:43:43Z

I am a bit confused by these changes.

It seems like for the GAPIC clients, the universe domain needs to be set in two places - in the servicePath and the options. Why not just let the GAPIC determine their own servicePath? If this is just keeping with the existing paradigm, then that's fine

The adminOptions and instanceAdminOptions are formed by merging the options from the existing BigtableTableAdminClient and BigtableInstanceAdminClient. Why? It seems circular to me - we create the options for those GAPICs and then save the options back from the GAPICs?

In response to 1. Because as I said in one of the PR comments, if we don't set universeDomain in the options then the test fails! When universe domain is set in a client option or using an environment variable by the user then universe domain has to be set on the gapic client options for the test to pass. Try it out yourself.

In response to 2. The client library worked this way before and I don't want to change the way it works because the minimal amount of changes necessary to support universe domains is ideal. I think these values adminOptions and dataOptions are saved so that they are ready when a gapic client gets initialized just before a request is made. If you want to change the way gapic options are handled in nodejs-bigtable then I would suggest filing a separate bug.

…h' of https://github.com/googleapis/nodejs-bigtable into universe-domain-service-path-ordinary-new-fix-one-branch

feywind · 2025-05-28T18:00:32Z

In response to 2. The client library worked this way before and I don't want to change the way it works because the minimal amount of changes necessary to support universe domains is ideal.

I'm behind this idea. If it was already that way before, let's leave it for now and circle back if it's weird.

…into universe-domain-service-path-ordinary-new-fix-one-branch # Conflicts: # system-test/service-path.ts

🤖 I have created a release *beep* *boop* --- ## [6.1.0](https://togithub.com/googleapis/nodejs-bigtable/compare/v6.0.0...v6.1.0) (2025-05-30) ### Features * Add full support for Universe Domain ([#1604](https://togithub.com/googleapis/nodejs-bigtable/issues/1604)) ([4562e23](https://togithub.com/googleapis/nodejs-bigtable/commit/4562e2329e734c0c9d9f00cfa83aa2be13e9a7fe)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).

danieljbruce added 30 commits March 12, 2025 14:09

Add test for testing tpc

823d03b

Update the test to create an instance

3d9822a

Remove TPC aspect and bug persists

bdca46b

More debugging

d93adfa

no universe domain stuff

52f31af

New working test

a9a1f01

Clean file up

0c4e2da

Eliminate arguments

c441f15

This is okay too

32fb943

Eliminating the project fetch is okay too

2caf336

Override Bigtable options

03b2bd3

Trying with an environment variable

f618252

Take one of the tests off of only

f00d375

Add a fix to allow universe domain option

94926c8

to be passed in

Use the bigtable client with the universe domain

f5bf88d

Change zone

3cc6785

Add the Google Application Credentials

414cc28

run linter

7c3c431

should have multiple tests

30b2c48

Add a third test that uses the universe domain

2d4b4a3

Remove env variable from test 3

36255ce

Use environment variable

482e922

Better names for the tests

b0868ce

Adjust options according to client type

e4ccb2e

Add application credentials to the test.

05a95c2

Pass universe domain into the gapic clients

b1bea62

Remove the before hooks

b52de15

Better factoring in the test

a556b40

remove console logs

53867bd

Remove only

334904c

danieljbruce requested a review from feywind May 14, 2025 14:46

bshaffer reviewed May 14, 2025

View reviewed changes

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2025

bshaffer reviewed May 14, 2025

View reviewed changes

danieljbruce added 4 commits May 26, 2025 10:37

Gapic client should take priority

5a989a4

turn on the test

96d69ec

Update tests with better instructions

3a058b6

Update protos

524ae61

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels May 26, 2025

🦉 Updates from OwlBot post-processor

76e8b65

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 26, 2025

danieljbruce added 2 commits May 26, 2025 11:52

comment application credentials

7444993

Merge branch 'universe-domain-service-path-ordinary-new-fix-one-branc…

23c52b4

…h' of https://github.com/googleapis/nodejs-bigtable into universe-domain-service-path-ordinary-new-fix-one-branch

danieljbruce requested a review from bshaffer May 26, 2025 15:54

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 27, 2025

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 27, 2025

feywind previously approved these changes May 28, 2025

View reviewed changes

Merge branch 'main' of https://github.com/googleapis/nodejs-bigtable …

b88c9b7

…into universe-domain-service-path-ordinary-new-fix-one-branch # Conflicts: # system-test/service-path.ts

danieljbruce dismissed feywind’s stale review via b88c9b7 May 28, 2025 18:06

feywind approved these changes May 28, 2025

View reviewed changes

bshaffer approved these changes May 29, 2025

View reviewed changes

danieljbruce merged commit 4562e23 into main May 30, 2025
19 of 22 checks passed

danieljbruce deleted the universe-domain-service-path-ordinary-new-fix-one-branch branch May 30, 2025 13:33

release-please bot mentioned this pull request May 30, 2025

chore(main): release 6.1.0 #1610

Merged

Conversation

danieljbruce commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Impact

Testing

Additional Information

Uh oh!

bshaffer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljbruce commented May 26, 2025

Uh oh!

feywind commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danieljbruce commented May 2, 2025 •

edited

Loading

bshaffer left a comment •

edited

Loading

danieljbruce May 14, 2025 •

edited

Loading