Skip to content
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

"unauthorized" test could print summary of requests made #832

Merged
merged 5 commits into from
Mar 30, 2022

Conversation

davepacheco
Copy link
Collaborator

This change causes the "unauthorized" integration test to print a summary table of all the requests it made. It's a pretty dense visual representation: it technically includes the URL path, HTTP method, and status code. Here's a text version:

$ cargo test -p omicron-nexus --test=test_all -- --nocapture integration_tests::unauthorized::test_unauthorized
    Finished test [unoptimized + debuginfo] target(s) in 0.76s
     Running tests/test_all.rs (target/debug/deps/test_all-41f3abf497b5cf9f)

running 1 test
log file: "/dangerzone/omicron_tmp/test_all-41f3abf497b5cf9f-test_unauthorized.9505.0.log"
note: configured to log to "/dangerzone/omicron_tmp/test_all-41f3abf497b5cf9f-test_unauthorized.9505.0.log"

SUMMARY OF REQUESTS MADE

          +----------------------------- privileged GET (expects 200)
          |                              (digit = last digit of 200-level response)
          |
          |   +----+----+----+----+----- HTTP methods tested (see below)
          |   |    |    |    |    |      (digit = last digit of 400-level response)
          |   |    |    |    |    |
          |   |    |    |    |    |  +-- privileged GET (expects same as above)
          |   |    |    |    |    |  |   (digit = last digit of 200-level response)
          |  _|   _|   _|   _|   _|  |   ('-' => skipped (N/A))
          v /  \ /  \ /  \ /  \ /  \ v
HEADER:   G GET  PUT  POST DEL  TRCE G  URL
EXAMPLE:  0 3111 5555 3111 5555 5555 0  /organizations
    ROW     ||||
            ||||                        TEST CASES FOR EACH HTTP METHOD:
            +|||-----------------------     authenticated, unauthorized request
             +||-----------------------     unauthenticated request
              +|-----------------------     bad authentication: no such user
               +-----------------------     bad authentication: invalid syntax

    In this case, an unauthenthicated request to "GET /organizations" returned
    401.  All requests to "PUT /organizations" returned 405.

G GET  PUT  POST DEL  TRCE G  URL
0 3111 5555 3111 5555 5555 0  /organizations
0 4411 4411 5555 4411 5555 0  /organizations/demo-org
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects
0 4411 4411 5555 4411 5555 0  /organizations/demo-org/projects/demo-project
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/vpcs
0 4411 4411 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc
0 4411 4411 5555 5555 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/firewall/rules
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/subnets
0 4411 4411 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/subnets/demo-vpc-subnet
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/routers
0 4411 4411 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/routers/demo-vpc-router
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/routers/demo-vpc-router/routes
0 4411 4411 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/vpcs/demo-vpc/routers/demo-vpc-router/routes/demo-router-route
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/disks
0 4411 5555 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/disks/demo-disk
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/disks/attach
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/disks/detach
0 4411 5555 4411 5555 5555 0  /organizations/demo-org/projects/demo-project/instances
0 4411 5555 5555 4411 5555 0  /organizations/demo-org/projects/demo-project/instances/demo-instance
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/start
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/stop
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/reboot
- 5555 5555 4411 5555 5555 -  /organizations/demo-org/projects/demo-project/instances/demo-instance/migrate
0 3111 5555 5555 5555 5555 0  /roles
0 4411 5555 5555 5555 5555 0  /roles/fleet.admin
0 3111 5555 5555 5555 5555 0  /users
0 4411 5555 5555 5555 5555 0  /users/db-init
0 3111 5555 5555 5555 5555 0  /hardware/racks
0 4411 5555 5555 5555 5555 0  /hardware/racks/c19a698f-c6f9-4a17-ae30-20d711b8f7dc
0 3111 5555 5555 5555 5555 0  /hardware/sleds
0 4411 5555 5555 5555 5555 0  /hardware/sleds/b6d65341-167c-41df-9b5c-41cded99c229
0 3111 5555 5555 5555 5555 0  /sagas
- 3111 5555 5555 5555 5555 -  /sagas/48a1b8c8-fc1c-6fea-9de9-fdeb8dda7823
0 3111 5555 5555 5555 5555 0  /timeseries/schema
- 5555 5555 3111 5555 5555 -  /updates/refresh
test integration_tests::unauthorized::test_unauthorized ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 59 filtered out; finished in 9.72s

It prints the numbers in green:

Screen Shot 2022-03-29 at 2 52 00 PM

Note that it never prints anything other than green -- if something produces an unexpected result, the test still panics. Still, it seemed useful to have the color here to emphasize that it's what's expected, since it's hard to compute what the right status code should be for each cell in the table.

Spoiler: the catalyst is to be able to demo authz, during which I would explain the various requests made by the test.

@davepacheco davepacheco requested a review from bnaecker March 29, 2022 21:55
@davepacheco davepacheco mentioned this pull request Mar 29, 2022
71 tasks
Copy link
Collaborator

@bnaecker bnaecker left a comment

Choose a reason for hiding this comment

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

Very cool. It took me a while to understand that the numbers are derived from the status codes after running the request described in the bottom right. It's a bit opaque, since that's probably the last thing most readers of a left-to-right language would see, but it's really the situation that resulted in the actual numbers. I don't think "rotating" the diagram is worth it, but maybe just putting a < or similar marker from the test case to the tail of the arrow to its left would help. I.e., a sort of "start here" marker.

+|||----------------------- authenticated, unauthorized request
+||----------------------- unauthenticated request
+|----------------------- bad authentication: no such user
+----------------------- bad authentication: invalid syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
+----------------------- bad authentication: invalid syntax
+----------------------< bad authentication: invalid syntax

v / \ / \ / \ / \ / \ v
HEADER: G GET PUT POST DEL TRCE G URL
EXAMPLE: 0 3111 5555 3111 5555 5555 0 /organizations
ROW ||||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ROW ||||
ROW ^^^^

| | | | | | +-- privileged GET (expects same as above)
| | | | | | | (digit = last digit of 200-level response)
| _| _| _| _| _| | ('-' => skipped (N/A))
v / \ / \ / \ / \ / \ v
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
v / \ / \ / \ / \ / \ v
| / \ / \ / \ / \ / \ |

| +----+----+----+----+----- HTTP methods tested (see below)
| | | | | | (digit = last digit of 400-level response)
| | | | | |
| | | | | | +-- privileged GET (expects same as above)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| | | | | | +-- privileged GET (expects same as above)
| | | | | | +-> privileged GET (expects same as above)

+----------------------------- privileged GET (expects 200)
| (digit = last digit of 200-level response)
|
| +----+----+----+----+----- HTTP methods tested (see below)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| +----+----+----+----+----- HTTP methods tested (see below)
| +----+----+----+----+----> HTTP methods tested (see below)

const VERIFY_HEADER: &str = r#"
SUMMARY OF REQUESTS MADE

+----------------------------- privileged GET (expects 200)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
+----------------------------- privileged GET (expects 200)
+----------------------------> privileged GET (expects 200)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is basically what I meant by my general comment. I added arrows hopefully showing the flow starting at the request that's made. May not be any better :)

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.

holy moly

@davepacheco
Copy link
Collaborator Author

@david-crespo thanks for taking a look!

@bnaecker Thanks for the comments. I'm definitely interested in better ways of presenting the data in the first place, as well as better ways to explain the presentation. My main goals are to convey exactly what happened in a way that basically fits on one (potentially wide) screen. (You probably figured this already but: the reason the labels are where they are now has more to do with where there's space than left-to-right top-to-bottom reading.)

I think your confusion results from parsing the ASCII arrows as describing flow, when they were only intended as labels (i.e., the text describes the thing it points to). But I like your suggestion of having the arrows reflect logical flow. I've done that in commit 8d88d69.

I also tried a different approach in b4a831f. I'd be interested to know if you find this clearer.

@bnaecker
Copy link
Collaborator

@davepacheco Yeah, I think either of the latter two options is better than the first. The third has slightly less clutter, without all the lines above the HTTP methods, but I actually think those help understand each entry. Both are good though, I'd pick whichever you like at this point.

One question actually. What's the difference between the leftmost G column and the rightmost?

@davepacheco
Copy link
Collaborator Author

Thanks!

One question actually. What's the difference between the leftmost G column and the rightmost?

Good question! For most paths, before we go through the various methods, we do a privileged GET and make sure we get a 200 back and we save the response body. That's the leftmost G. After doing all these requests, we do another privileged GET, make sure we get a 200 back, and check that the response body matches what it was before. This attempts to verify that none of the endpoints took any action. (Even though they all would have returned a 400-level error, it's conceivable that they still took some action to modify or delete the resource.) This second request is the rightmost 'G'.

This seemed like too much to explain in the output.

@davepacheco davepacheco merged commit 9749356 into main Mar 30, 2022
@davepacheco davepacheco deleted the unauthorized-demo branch March 30, 2022 16:31
leftwo pushed a commit that referenced this pull request Jul 15, 2023
Crucible:
Add quota to agent created datasets (#835)
Switch to building on heliosv2 (#830)
Minor clippy cleanup (#832)
Update to latest dropshot (#829)

Propolis:
The above crucible changes
Switch to building on heliosv2 (#461)
clean up cargo check/clippy errors when built with Rust 1.71 (#462)
Add some VMM_DESTROY_VM polish to bhyve-api
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