Skip to content

Conversation

@davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Nov 18, 2021

This change prototypes a few interfaces for more ergonomically making HTTP requests in the Nexus test suite. The meat is in nexus/tests/common/http_testing.rs. It includes:

  • RequestBuilder, which essentially prototypes Proposal: make_request helpers shouldn't assert status code dropshot#165. This struct is Nexus-agnostic and could go into Dropshot instead of keeping it in Nexus. My hope is that we could eventually move this into Dropshot, deprecate all the dropshot::ClientTestContext::make_request*() functions and replace them with this. Those functions should be implementable (for compatibility) in terms of this new RequestBuilder. The objects_{post,get,delete} family of functions in dropshot::test_util could also be implemented in terms of this -- in fact, I included a dropshot_compat module here with drop-in replacements for these functions.
  • NexusRequest is a light wrapper around RequestBuilder that gives us a place to put Nexus-specific customization. The one (really important) example of this right now is NexusRequest::authn_as, which lets the test say how it wants to authenticate without having to know about spoof authentication, the specific header names and values, etc.

Reviewers: I would actually start with the places in the Nexus test suite that I've updated to use the new functions:

  • create_organization() now uses NexusRequest. Previously, this function had a bit of authn boilerplate that would have to be replicated to every place where we need to authenticate. That is, as we protect more external API endpoints with authorization, we'd need to copy around this boilerplate. That's basically what pushed me to make this PR.
  • test_authz.rs: this wasn't so bad before, but it now uses RequestBuilder. (It can't use NexusBuilder because it's not actually talking to Nexus -- it needs to provide different kinds of values for the "spoof" header than a valid Nexus client does)

This change currently depends on this tiny Dropshot change:

diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs
index 83799fe..8f15d32 100644
--- a/dropshot/src/test_util.rs
+++ b/dropshot/src/test_util.rs
@@ -52,7 +52,7 @@ pub struct ClientTestContext {
     /** HTTP client, used for making requests against the test server */
     client: Client<HttpConnector>,
     /** logger for the test suite HTTP client */
-    client_log: Logger,
+    pub client_log: Logger,
 }

 impl ClientTestContext {

Here's what I'd like to do:

  • get feedback on RequestBuilder and NexusRequest interfaces (and get them to where I've got some +1's)
  • [ ] get feedback on whether we want to put RequestBuilder straight into Dropshot or keep it in Omicron for now. (The main reason to keep it out is to get more experience with it before we commit to it in Dropshot.) I want to move this into Dropshot in a follow-on PR, not now, so that it's easier to iterate on as I convert more endpoints to use NexusRequest.
  • make a Dropshot PR for the above diff (ClientTestContext could expose client and log dropshot#192)
  • land this change into Omicron

In follow-up PRs, I'll:

  • convert more of the Nexus test suite to use NexusRequest -- it will be needed as we convert API endpoints to use authz.
  • move RequestBuilder into Dropshot to replace what's there today

@davepacheco davepacheco marked this pull request as draft November 18, 2021 00:17
@david-crespo
Copy link
Contributor

Not done looking through it, but I'd like to leave this in Nexus for a week or two even after this PR for more iteration.

@davepacheco davepacheco marked this pull request as ready for review November 19, 2021 02:21
Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

This is great! I'm going to use it RIGHT NOW

@davepacheco davepacheco merged commit 60f4fb3 into main Nov 19, 2021
@davepacheco davepacheco deleted the request-tester branch November 19, 2021 17:36
@davepacheco
Copy link
Collaborator Author

Thanks for taking a close look and playing with it!

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.

3 participants