google-cloud-go: compute: service account-related BulkInsert error not surfaced to user
Client compute “cloud.google.com/go/compute/apiv1” computepb “cloud.google.com/go/compute/apiv1/computepb”
instanceClient := compute.NewInstancesRESTClient(context.TODO()) instanceClient.BulkInsert()
Go Environment
$ go version go version go1.20.3 linux/amd64 $ go env Not relevant
Code
e.g.
package main
func main() {
input := &computepb.BulkInsertInstanceRequest{}
op, err1 := adpt.gceClient.BulkInsert(ctx, input)
err2 := op.Wait(ctx)
// instances launched == 0
// err1 == nil and err2 == nil
}
Expected behavior
- when error is service account-related (e.g. not found), then either
err1 != nil
orerr2 != nil
Actual behavior
- when error is service account-related (e.g. not found), then instances launched == 0,
err1 == nil
anderr2 == nil
- verified by using Google Logs Explorer and found the error “VM_MIN_COUNT_NOT_REACHED,EXTERNAL_RESOURCE_NOT_FOUND”
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 18 (10 by maintainers)
I’m hoping we can get it in this week’s release, but if not this week then definitely next.
Ok so we have the initial changes merged into
main
here: https://github.com/googleapis/google-cloud-go/blob/151908ab02a0b22c8e537d1bee00afc7f1465511/compute/apiv1/operations.go#L192-L199We want to get a test case into our integration tests, and waiting on finding a good error mode to reliably test with.
I’m hoping we can release this code next week. Feel free to try it yourself though if you want to wire up the unreleased dependency.
I just hit this today while trying to delete a network that still had a subnetwork in it, and the error was undetected. I’ve noticed that the workaround of checking the http status doesn’t work if the operation succeeded (at least in the network deletion case) because that field is not present in the result. So you have to check if it’s there at all, and then check it against
http.StatusOK
.Here’s my temporary workaround:
Well, you nailed it when you pointed out
Compute Engine uses its own Operations client, not the longrunning one.
😃 This is getting in the weeds, and frankly as user, you shouldn’t have to care about this difference, but because pretty much every other Cloud API uses LRO, we are able to generate highly regular code, especially when that code requires inspecting the response in some way (e.g. checking if the Operation.result is an error). Since Compute spun its own Operations pattern, with its own types and services, we had to start annotating the schema with indicators of what each element means relative to something like LRO.So, this was just a mistake on our part while implementing generator support. We left out this admittedly crucial piece.
@ijrsvt you’re right, but we can’t use your suggestion directly. I’ve filed https://github.com/googleapis/gapic-generator-go/issues/1320 to fix this in our client generator.
We can’t use
h.proto.Error != nil
directly because the ComputeOperation.error
field is “non standard” vs. other APIs. Thehttp_error_message
andhttp_error_status_code
fields, however, are simple to consume and annotated as the “error code” and “error message” fields. Other non-standard APIs will have similar such fields annotated in the same way. We can use these to generically construct an error type.@hanming-lu in the mean time, you can do something similar to what @ijrsvt suggests and check the error via
op.Proto().GetHttpErrorStatusCode() != http.StatusOK
. I know this isn’t ideal and we will work to get this rectified ASAP.