fix(logging): Specifying resourceNames should fetch logs only from those resources #1596
Conversation
When `getEntries` is called with a `resourceNames` array that specifies a logging bucket (particularly one in a
different project), the library was incorrectly appending the current project's ID to the array.
This resulted in an invalid request, causing the Logging API to reject it with an `INVALID_ARGUMENT` error.
The fix adjusts the logic to only inject the default project ID if the `resourceNames` array is explicitly empty. This
preserves the expected default behavior of searching within the current project, while respecting user-provided
resource names for cross-project queries.
Fixes: googleapis#1593
product-auto-label
bot
added
the
api: logging
| this.projectId = await this.auth.getProjectId(); | ||
| const resourceName = 'projects/' + this.projectId; | ||
| if (reqOpts.resourceNames.indexOf(resourceName) === -1) { | ||
| if (reqOpts.resourceNames.length === 0) { |
There was a problem hiding this comment.
I would definitely add a comment just inside this if statement explaining the reason why we only add the resourceName when the resourceNames array is empty.
| } | ||
|
|
||
| reqOpts.resourceNames = arrify(reqOpts.resourceNames!); | ||
| this.projectId = await this.auth.getProjectId(); |
There was a problem hiding this comment.
I don't know enough about this code to know that if we only set the projectid of this object inside the if statement now how that might affect other code in this class. We would have to do a search on this class for the projectId of this property.
I might consider just keeping this line of code where it is?
| }); | ||
|
|
||
| it('should accept GAX options', async () => { | ||
| const config = { |
There was a problem hiding this comment.
No need to do this now, but I think it would be good to have a system test to capture the intended behaviour. Something like main...fix/resource-name-other-projects-dan. I'm not sure how hard this is though. Maybe it requires setting up another project which is a lot of work.
I don't want this to block the merge, but maybe adding a TODO or internal issue would be good.
There was a problem hiding this comment.
We want a test to ensure the request returns entries because that is a reproduction of the issue.
|
I don't have permission to approve, but I would if I could. I think just keeping the |
bhshkh
changed the title
|
Hi @danieljbruce, The PR looks good to me. However CI testing failed due to node version. Do you know if there is a way that we can bypass these checks? |
|
Try removing 14 from the array at nodejs-logging/.github/workflows/ci.yaml Line 12 in 9d1d480 |
|
Closing PR as we have merged the forked PR with testing configurations: #1597 |
3 participants
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1593 🦕
This PR addresses an issue where users were unable to fetch logs from a logging bucket located in a different Google Cloud project.
Issue
if projects/${BUCKET_CONTAINING_PROJECT}/locations/global/buckets/my-repro-bucket-01/views/_AllLogs has log entry "log-01"
and projects/${BUCKET_CONTAINING_PROJECT}/locations/global/buckets/my-repro-bucket-02/views/_AllLogs has log entry "log-02",
and projects/${BUCKET_CONTAINING_PROJECT}/locations/global/buckets/_Default/views/_AllLogs has log entry "log-01", "log-02", "log-03", "log-04". Then, providing resourceNames: my-repro-bucket-01 and my-repro-bucket-02
should retrieve only "log-01" and "log-02" but in the current library, it returns "log-01", "log-02", "log-03", "log-04"
Fix
The fix refines the logic for handling default resource names. The library now checks if the resourceNames array is empty before adding the default project ID.
This change ensures that developers can correctly query logs from any bucket they have access to, including those outside of the client's default project. A unit test has been added to verify this behavior and prevent future regressions.