Skip to content

Update progenitor, progenitor-client, oauth2, and reqwest #870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 15, 2024
Merged

Conversation

ahl
Copy link
Collaborator

@ahl ahl commented Oct 8, 2024

... more rooting out of hyper v0.x

There are two small output changes due to our use of the built-in functionality of oauth2 5.0.0-rc.1 that we had previously been doing by hand.

See ramosbugs/oauth2-rs#288 for the output changes

I'm not sure if we should merge this before or after the next release. My inclination is to do so... we can always revert and ship new binaries if we kick over some problem.

@ahl ahl requested review from wfchandler and inickles October 8, 2024 00:26
@wfchandler
Copy link
Contributor

We're losing some useful context with this change, the nested error is no longer printed:

# Branch: update-several
$ oxide auth status
  Checking https://oxide.sys.rack2.eng.oxide.computer...
Profile "dogfood" (https://oxide.sys.rack2.eng.oxide.computer) status: Communication Error: error sending request for url (https://oxide.sys.rack2.eng.oxide.computer/v1/me)

# Branch: main
$ oxide auth status
  Checking https://oxide.sys.rack2.eng.oxide.computer...
Profile "dogfood" (https://oxide.sys.rack2.eng.oxide.computer) status: Communication Error: error sending request for url (https://oxide.sys.rack2.eng.oxide.computer/v1/me): operation timed out

@ahl
Copy link
Collaborator Author

ahl commented Oct 8, 2024

We're losing some useful context with this change, the nested error is no longer printed:

Indeed! That seems to be due to the new reqwest. I this sort of thing in omicron (oxidecomputer/omicron#6717, oxidecomputer/omicron#6716). I'll ask @seanmonstar about this change.

@seanmonstar
Copy link

reqwest and hyper both changed such that the default Display does not also include any source errors, since "reporters" that print the source chain would result in duplicate results. The change was due to the official guidelines for errors, discussed at rust-lang/project-error-handling#27. I don't agree with the change, since I find it confusing to people who don't know about reporters (I prefer making it better by default and require advanced users to opt-in), which I brought up, but many see it the other way. 🤷

@wfchandler
Copy link
Contributor

Thanks for the context, I've pushed 78293ec to restore printing the error sources.

@ahl
Copy link
Collaborator Author

ahl commented Oct 9, 2024

Thanks for the context, I've pushed 78293ec to restore printing the error sources.

That didn't seem like the right approach so I reverted the change. In particular, taking the source chain (except for the first source) seems to produce the maximal set of output but certainly includes information beneath the level of detail that would make sense for a user, would have internal implementation details, and is only validated in a limited number of our tests.

@wfchandler
Copy link
Contributor

Thanks for the context, I've pushed 78293ec to restore printing the error sources.

That didn't seem like the right approach so I reverted the change. In particular, taking the source chain (except for the first source) seems to produce the maximal set of output but certainly includes information beneath the level of detail that would make sense for a user, would have internal implementation details, and is only validated in a limited number of our tests.

The the status quo ante was already printing several sources deep, I don't think we have a way of retaining the same level of detail previously present without walking the source chain in some manner.

The approach I pushed earlier works a bit better by stopping when the first io::Error is hit, but I don't have any confidence this is a general solution. Perhaps you can find a more principled way to find the relevant errors.

let mut s = ee.source().and_then(|e| e.source());
while let Some(source) = s {
    if source.downcast_ref::<std::io::Error>().is_some() {
        break;
    }
    err += &format!(": {source}");
    s = source.source();
}
# main, reqwest 0.11.27
$ oxide auth status
Profile "dogfood" (https://oxide.sys.rack2.eng.oxide.computer) status: Communication Error: error sending request for url (https://oxide.sys.rack2.eng.oxide.computer/v1/me): operation timed out
  Checking https://unresolvabledomainnameihope...
Profile "string" (https://unresolvabledomainnameihope) status: Communication Error: error sending request for url (https://unresolvabledomainnameihope/v1/me): error trying to connect: dns error: failed to lookup address information: nodename nor servname provided, or not known

# with above patch, reqwest 12.8
$ oxide auth status
Profile "dogfood" (https://oxide.sys.rack2.eng.oxide.computer) status: Communication Error: error sending request for url (https://oxide.sys.rack2.eng.oxide.computer/v1/me): operation timed out
  Checking https://unresolvabledomainnameihope...
Profile "string" (https://unresolvabledomainnameihope) status: Communication Error: error sending request for url (https://unresolvabledomainnameihope/v1/me): client error (Connect): dns error: failed to lookup address information: nodename nor servname provided, or not known

Copy link
Contributor

@wfchandler wfchandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that --debug is working with #878 it's less of an issue if we don't capture the full error chain in the default output.

Copy link
Contributor

@inickles inickles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to see the upgrades for reqwest and progenitor-client. I think moving to a release candidate version of oauth2 is fine, not sure it's worth waiting for the official 5 release.

@ahl ahl merged commit 29bfd97 into main Oct 15, 2024
16 checks passed
@ahl ahl deleted the update-several branch October 15, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants