fix: Allow quota project to be used in combination with null credentials

[irock]

Credentials may be null, e.g. when connecting to an emulator. This fix makes it possible to use null credentials in combination with setting quota project id, something that may be useful in testing.

Fixes #1687 ☕️

[lqiu96]

Thanks @irock for the PR! I ran a sample locally and I believe I was able to reproduce the error (by setting the quotaProjectId
and using NoCredentialsProvider):

Caused by: io.grpc.StatusRuntimeException: UNAUTHENTICATED: Failed computing credential metadata
	at io.grpc.Status.asRuntimeException(Status.java:539)
	... 17 more
Caused by: java.lang.NullPointerException: Cannot invoke "com.google.auth.Credentials.getRequestMetadata(java.net.URI)" because "this.wrappedCredentials" is null
	at com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials.getRequestMetadata(QuotaProjectIdHidingCredentials.java:64)
	at com.google.auth.Credentials.blockingGetToCallback(Credentials.java:112)
	at com.google.auth.Credentials$1.run(Credentials.java:98)
	... 3 more

Similar error exists for httpjson as well.

Is there a specific sample or reference that you were using (or is the a specific case for your tests)? Just trying to figure out the use case for NoCredentialsProvider + setting the QuotaProjectId.

[sonarqubecloud]

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[sonarqubecloud]

[java_showcase_integration_tests] SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

40.0% Coverage
0.0% Duplication

[sonarqubecloud]

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[irock]

Thanks @irock for the PR! I ran a sample locally and I believe I was able to reproduce the error (by setting the quotaProjectId and using NoCredentialsProvider):

Caused by: io.grpc.StatusRuntimeException: UNAUTHENTICATED: Failed computing credential metadata
	at io.grpc.Status.asRuntimeException(Status.java:539)
	... 17 more
Caused by: java.lang.NullPointerException: Cannot invoke "com.google.auth.Credentials.getRequestMetadata(java.net.URI)" because "this.wrappedCredentials" is null
	at com.google.api.gax.rpc.internal.QuotaProjectIdHidingCredentials.getRequestMetadata(QuotaProjectIdHidingCredentials.java:64)
	at com.google.auth.Credentials.blockingGetToCallback(Credentials.java:112)
	at com.google.auth.Credentials$1.run(Credentials.java:98)
	... 3 more

Similar error exists for httpjson as well.

Is there a specific sample or reference that you were using (or is the a specific case for your tests)? Just trying to figure out the use case for NoCredentialsProvider + setting the QuotaProjectId.

We're using QuotaProjectId internally at Spotify to make sure the correct GCP project is attributed when running Pub/Sub workloads in multi-tenant kubernetes clusters.

The combination of QuotaProjectId + NoCredentialsProvider appears during testing, e.g. when we configure a Pub/Sub consumer to connect to a local emulator.

Btw, one workaround we have found is to use a different case for the header key, e.g. X-Goog-User-Project instead of the lower case x-goog-user-project. In practice this attributes the quota correctly, but bypasses the use of QuotaProjectIdHidingCredentials.

[lqiu96]

Thanks for the info. I haven't seen the combo of QuotaProjectId + NoCredentialsProvider before and was kind of curious if this was something certain services were suggesting to do. Makes sense that this appeared during testing.

I do think that this is an odd behavior and that this change shouldn't have any impact. Null credentials is not going to have any conflicting quota project ids.

Looping in @blakeli0 for a second opinion.

[blakeli0]

Thanks @lqiu96 for the initial review!

The combination of QuotaProjectId + NoCredentialsProvider appears during testing, e.g. when we configure a Pub/Sub consumer to connect to a local emulator.

@irock I'm not sure I understand the usefulness of this test. Why do you need to set QuotaProjectId in a local emulator testing? Is it being verified somewhere in the test? Your workaround makes sense because that is effectively not setting QuotaProjectId, if the workaround is acceptable, I would suggest not to set x-goog-user-project header at all in your tests.
In general, adding test only code is not ideal, and this change also has some side effect. e.g. If someone sets QuotaProjectId but couldn't get credentials in production, the request is now failing much later on the server side instead of client side.

[irock]

Thanks @lqiu96 for the initial review!

The combination of QuotaProjectId + NoCredentialsProvider appears during testing, e.g. when we configure a Pub/Sub consumer to connect to a local emulator.

@irock I'm not sure I understand the usefulness of this test. Why do you need to set QuotaProjectId in a local emulator testing? Is it being verified somewhere in the test?

We are not testing the quota project id specifically, as we trust the library to do the right thing. Rather, we want our tests to be opaque to the fact that quota project id is set. As you can imagine, we could technically have tests explicitly disabling the quota project id, but that seems like a leaky abstraction.

Your workaround makes sense because that is effectively not setting QuotaProjectId, if the workaround is acceptable, I would suggest not to set x-goog-user-project header at all in your tests.

As far as I can tell, our workaround is effectively setting the quota project id in requests to Pub/Sub. Indeed, the requests will fail unless the service has permissions to consume quota from the project in question. The code in the library is checking for lower case only, but it seems like the server side is case agnostic.

In general, adding test only code is not ideal, and this change also has some side effect. e.g. If someone sets QuotaProjectId but couldn't get credentials in production, the request is now failing much later on the server side instead of client side.

As I understand it, the intention of QuotaProjectIdHidingCredentials is to hide any attempts by the credentials to set the quota project id. If credentials is null, it is impossible for this to happen, and there is no need for wrapping the credentials in the first place.

I think if you want the code here to protect users from configuring the library incorrectly, it would be better to check that explicitly. Indeed, NoCredentialsProvider is a supported use, and the rest of the code in this class does handle this appropriately.

[blakeli0]

Rather, we want our tests to be opaque to the fact that quota project id is set. As you can imagine, we could technically have tests explicitly disabling the quota project id, but that seems like a leaky abstraction

I don't think I agree that it is a leaky abstraction, the test setups are supposed to be different if you are testing locally. For example, as you mentioned, you have to initialize the client with NoCredentialsProvider, I don't see much difference between setting NoCredentialsProvider and not setting quota project id. Also how did you set quota project id in the tests? It should not be set by default, but explicitly disabling the quota project id makes me guessing that you have a generic test setup that is always setting it?

I think if you want the code here to protect users from configuring the library incorrectly, it would be better to check that explicitly. Indeed, NoCredentialsProvider is a supported use, and the rest of the code in this class does handle this appropriately

Yes I agree it would be better to check it explicitly. I'm using this as an example to show that this is technically a behavior breaking change, as we may have customers that are dependent on this behavior(even if the behavior may not be ideal), e.g. Some customers may have a test verifying NullPointerException for the combination of QuotaProjectId + NoCredentialsProvider, this change would break that test now. I would probably go with your approach if we are implementing QuotaProjectId from scratch but I would be hesitate to make the change now. Also NoCredentialsProvider is a supported use case but maybe it should not be used in combination with QuotaProjectId.

In short, thanks for finding this issue and I think it's a miss on our side, we should've made it clear that whether QuotaProjectId can work together with NoCredentialsProvider or not when we implemented QuotaProjectId. I'll dig a little more and see the full impact of the change, but in the mean time, I would suggest to not setting QuotaProjectId in the tests.

[irock]

Rather, we want our tests to be opaque to the fact that quota project id is set. As you can imagine, we could technically have tests explicitly disabling the quota project id, but that seems like a leaky abstraction

I don't think I agree that it is a leaky abstraction, the test setups are supposed to be different if you are testing locally. For example, as you mentioned, you have to initialize the client with NoCredentialsProvider, I don't see much difference between setting NoCredentialsProvider and not setting quota project id. Also how did you set quota project id in the tests? It should not be set by default, but explicitly disabling the quota project id makes me guessing that you have a generic test setup that is always setting it?

To clarify how this works in more detail, our test code is written to diverge from production settings as little as possible.

For example, the production code may have configuration like so:

# service.conf
pubsub {
  subscriber {
    project-id: my-gcp-project-id
    subscription-id: my-subscription-id
    quota-project-id: my-gcp-project-id
  }
}

Dependencies for the service are discovered during boot, through service discovery mechanisms such as DNS and other means. Our test environments mimics production here, using mock test-containers and network configuration to tie the service to its dependencies without the need for the service to have custom test code.

For Pub/Sub, that discovery mechanism does not work, and we need to specifically configure the service to connect to the Pub/Sub emulator host/port. Furthermore, as the emulator does not support credentials, we need to configure the service to connect using no credentials. From an end developer user experience point of view, this is quite cumbersome as it is.

Clearing quota project id is another emulator specific setting that developers need to be aware of when they set up tests.

So in summary, in order to test a Pub/Sub integration with an emulator, developers need to know not only that they should connect the service to the emulator explicitly through host/port configuration (e.g. setting up a custom transport channel), but also that the emulator does not support credentials, and that the library will malfunction if one sets the quota project id.

This is what I mean when I say that it's a leaky abstraction. The emulator is supposed to abstract away the Pub/Sub service so that developers can confidently test their code. But the difference in functionality is leaking to users.

I think if you want the code here to protect users from configuring the library incorrectly, it would be better to check that explicitly. Indeed, NoCredentialsProvider is a supported use, and the rest of the code in this class does handle this appropriately

Yes I agree it would be better to check it explicitly. I'm using this as an example to show that this is technically a behavior breaking change, as we may have customers that are dependent on this behavior(even if the behavior may not be ideal), e.g. Some customers may have a test verifying NullPointerException for the combination of QuotaProjectId + NoCredentialsProvider, this change would break that test now. I would probably go with your approach if we are implementing QuotaProjectId from scratch but I would be hesitate to make the change now. Also NoCredentialsProvider is a supported use case but maybe it should not be used in combination with QuotaProjectId.

This seems like a very hypotethical use case. The fact is that using QuotaProjectId plus NoCredentialsProvider does not work today, and this PR fixes this.

Yes, alluding to Hyrum's Law, if someone has written a test that confirms that the library breaks if this combination is used, then that test will not pass after this change. However, couldn't the same be said about any bug?

In short, thanks for finding this issue and I think it's a miss on our side, we should've made it clear that whether QuotaProjectId can work together with NoCredentialsProvider or not when we implemented QuotaProjectId. I'll dig a little more and see the full impact of the change, but in the mean time, I would suggest to not setting QuotaProjectId in the tests.

If you do not accept this change, would you accept a PR that explicitly throws if QuotaProjectId is used together with NoCredentialsProvider as it is not supported? I think that would break more tests than this PR.

[blakeli0]

This is what I mean when I say that it's a leaky abstraction. The emulator is supposed to abstract away the Pub/Sub service so that developers can confidently test their code. But the difference in functionality is leaking to users.

I totally understand the frustration of having to set up things differently in local testing, especially when these things are added on top of emulators, we suffer the same in our own local testing. Unfortunately our team does not own the emulators, if you can file a request(preferably through Support) with what exactly you want to improve on the emulators side, I would be more than happy to help you as much as I can internally.

This seems like a very hypotethical use case. The fact is that using QuotaProjectId plus NoCredentialsProvider does not work today, and this PR fixes this.
Yes, alluding to Hyrum's Law, if someone has written a test that confirms that the library breaks if this combination is used, then that test will not pass after this change. However, couldn't the same be said about any bug?

I don't disagree that this is a very unlikely use case, and you could say the same for any bug. But since the impact is huge with every little change in this repo, we have been very careful in fixing anything/making any behavior changes in this repo. Usually how we decide if we want to fix a bug is that:

1. Does this affect production? In this case, no.
2. Do we have a workaround? In this case, yes.

So that makes the fix of this bug low benefit, and high risk(probably could be lower but need more investigation to confirm).

If you do not accept this change, would you accept a PR that explicitly throws if QuotaProjectId is used together with NoCredentialsProvider as it is not supported? I think that would break more tests than this PR.

I would accept neither as of now, but I want this in as much as you want since it can definitely improve customer experience in testing. So as I mentioned previously, I'll dig more and see if we can determine the risk.

[irock]

This is what I mean when I say that it's a leaky abstraction. The emulator is supposed to abstract away the Pub/Sub service so that developers can confidently test their code. But the difference in functionality is leaking to users.

I totally understand the frustration of having to set up things differently in local testing, especially when these things are added on top of emulators, we suffer the same in our own local testing. Unfortunately our team does not own the emulators, if you can file a request(preferably through Support) with what exactly you want to improve on the emulators side, I would be more than happy to help you as much as I can internally.

Thanks, we might do that.

This seems like a very hypotethical use case. The fact is that using QuotaProjectId plus NoCredentialsProvider does not work today, and this PR fixes this.
Yes, alluding to Hyrum's Law, if someone has written a test that confirms that the library breaks if this combination is used, then that test will not pass after this change. However, couldn't the same be said about any bug?

I don't disagree that this is a very unlikely use case, and you could say the same for any bug. But since the impact is huge with every little change in this repo, we have been very careful in fixing anything/making any behavior changes in this repo. Usually how we decide if we want to fix a bug is that:

1. Does this affect production? In this case, no.

2. Do we have a workaround? In this case, yes.

So that makes the fix of this bug low benefit, and high risk(probably could be lower but need more investigation to confirm).

Thanks for clarifying. I understand I don't have all the context required to assess the risk of this change. From my perspective the implicit risk of keeping this behavior is not to be disregarded either, as the library malfunctions in a quite indirect way.

If you do not accept this change, would you accept a PR that explicitly throws if QuotaProjectId is used together with NoCredentialsProvider as it is not supported? I think that would break more tests than this PR.

I would accept neither as of now, but I want this in as much as you want since it can definitely improve customer experience in testing. So as I mentioned previously, I'll dig more and see if we can determine the risk.

I appreciate that!

[blakeli0]

@irock This PR is now merged and expected to be released in the next two weeks.

[irock]

@irock This PR is now merged and expected to be released in the next two weeks.

Thank you! 🙌