Byron / google-apis-rs

A binding and CLI generator for all Google APIs
http://byron.github.io/google-apis-rs
Other
983 stars 132 forks source link

List compute engine virtual machines fails to parse API response if a VM has "owner" as a label key #502

Closed dantreiman closed 1 month ago

dantreiman commented 1 month ago

I've written a function which uses google_compute1 5.0.4 to list virtual machines in a zone:

 let compute_engine = get_compute_engine_client().await.expect("Failed to create compute engine API client.");
 match compute_engine.instances().list(PROJECT_ID, ZONES).doit().await {
    ...
}

This function succeeds normally. However, if I add a label with key "owner" to a VM in the specified zone, the request fails with error: Invalid symbol 45, offset 3. at line 92 column 40

For example, I added the label "owner": "dtreiman" to a VM and the request fails. Remove the "owner" label, request succeeds.

Update: I tried using "resource_owner" : "dtreiman" and "requester":"dtreiman" and the error appears. However, it works with "requester":"me" or "foo": "bar"

Byron commented 1 month ago

Thanks for reporting!

I think here it would be easiest to vendor the crate (or put it locally), use a [patch."crates-io"] in the dependent crate to point to it, and add debugging code to see what's going on. Maybe extract the received JSON verbatim to understand why it doesn't seem to parse.

From there, a local fix should be possible, and maybe it's possible too to derive a more general fix as well.

dantreiman commented 1 month ago

Thanks! Per your suggestion, I cloned the crate locally so I could insert debugging code. The error appears to be happening during parsing of the response. Invalid symbol 45, offset 8. at line 25 column 39 refers to ASCII character 45 which is -.

Line 25 in the response below is: "fingerprint": "1cecahVF-n8=",. So, maybe this error happens when the - character is present in the fingerprint.

{
"kind": "compute#instanceList",
"id": "<REDACTED>",
"items": [
{
"kind": "compute#instance",
"id": "1879471610156305019",
"creationTimestamp": "2024-06-06T10:40:38.876-07:00",
"name": "instance-20240606-174003",
"description": "",
"tags": {
"fingerprint": "42WmSpB8rSM="
},
"machineType": "<REDACTED>",
"status": "RUNNING",
"zone": "<REDACTED>",
"canIpForward": false,
"networkInterfaces": [
{
"kind": "compute#networkInterface",
"network": "<REDACTED>",
"subnetwork": "<REDACTED>",
"networkIP": "<REDACTED>",
"name": "nic0",
"fingerprint": "1cecahVF-n8=",
"stackType": "IPV4_ONLY"
}
],
...
dantreiman commented 1 month ago

Listing different configurations, I also got Invalid symbol 95. Therefore I think these fingerprint fields are URL-safe base64, and serde is attempting to read them as standard base64.

Replacing #[serde_as(as = "Option<::client::serde::standard_base64::Wrapper>")] with #[serde_as(as = "Option<::client::serde::urlsafe_base64::Wrapper>")] in api.rs fixes the issue.

Now, how to fix this so api.rs gets generated with urlsafe_base64?

dantreiman commented 1 month ago

@Byron I made a branch which fixes my problem by re-generating with urlsafe_base64 for all bytes fields: https://github.com/dantreiman/google-apis-rs/tree/url_safe_base64

However, after reading some other issues (ex. https://github.com/Byron/google-apis-rs/issues/442) it seems like this risks breaking other APIs which rely on standard encoding. What do you think about adding a "tolerant" base64 deserializer which accepts either standard or url-safe encoding?

Byron commented 1 month ago

However, after reading some other issues (ex. #442) it seems like this risks breaking other APIs which rely on standard encoding. What do you think about adding a "tolerant" base64 deserializer which accepts either standard or url-safe encoding?

Yes, please, I'd love that and believe have been mentioning this solution for quite a while now. Somehow it never came to fruition, but I think the reason wouldn't be technical.

The question of course will be what happens when trying to send payloads with base64 that the server expects to be URL-safe base64, but that's a problem for another day.

In any case, I will be looking forward to the PR, and will even be re-releasing all APIs to have this bug fixed for good.

paolobarbolini commented 1 month ago

This could be fixed by #507 if it weren't for the fact that the fields description doesn't specify that it's using the URL dictionary :disappointed:

Byron commented 1 month ago

Fortunately this should be addressed by the previously linked PR (#506). That won't help with sending data, but at least receiving it should not have issues being decoded.

paolobarbolini commented 1 month ago

That won't help with sending data, but at least receiving it should not have issues being decoded.

I wander how other client implementations handle this documentation shortcoming.

Byron commented 1 month ago

I was thinking the same! Once I tried to figure it out but it wasn't as easy as I hoped. With more determination, that should be possible though and it would definitely be interesting to know if there are better ways. After all, some type-information is provided, maybe it's there without us seeing it?