Instance network interfaces don't need to be paginated (#1009)

[ryaeng]

Removed page params from entrypoint, app, and db calls.

[david-crespo]

It seems you're checking if the vpc_id equals the instance ID — that can't be right.

[ryaeng]

@david-crespo What's a good way to test this?

[bnaecker]

I believe this would be caught by the existing integration tests. Specifically, this call here fetches the existing NICs attached to an instance. I can't see how that would return anything if the wrong UUID were used.

Can you show the output of your tests against this commit?

[david-crespo]

If you look at CI on this commit, the tests passed even when that bug was in place

c563bfe

[david-crespo]

Thank you for your contribution. I have been meaning to make a similar change at some point.

[ryaeng]

I'll have these fixed momentarily.

What's the best way to amend this PR with the changes?

[bnaecker]

@ryaeng Pushing a new commit is fine. It'll all be squashed at merge time.

[ryaeng]

@ryaeng Pushing a new commit is fine. It'll all be squashed at merge time.

Awesome! Thanks.

[ryaeng]

Done

[david-crespo]

This is looking pretty good. The reason I haven't merged is I'm mulling over whether the endpoint should still return ResultsPage<NetworkInterface>, which has the items key with the items and a next_page key will which always be null. If you look at firewall rules, which are similarly not paginated

omicron/nexus/src/external_api/http_entrypoints.rs

Lines 2458 to 2461 in 215491b

	async fn vpc_firewall_rules_get(
	rqctx: Arc<RequestContext<Arc<ServerContext>>>,
	path_params: Path<VpcPathParam>,
	) -> Result<HttpResponseOk<VpcFirewallRules>, HttpError> {

we return a dedicated struct that just has the rules on it:

omicron/common/src/api/external/mod.rs

Lines 1409 to 1413 in 215491b

	/// Collection of a [`Vpc`]'s firewall rules
	#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
	pub struct VpcFirewallRules {
	pub rules: Vec<VpcFirewallRule>,
	}

That is probably what we should do here too, though I would prefer it live in nexus/src/external_api/views.rs rather than omicron_common spot.

[zephraph]

ResultsPage should be used only for paginated endpoints. Not doing so introduces ambiguity into the API that'd be a potential point of confusion for users.

[ryaeng]

I'll make this happen tomorrow, fellas.

[ryaeng]

Got this done. Spent a couple of hours working on it before I realized I was comparing it to subnets and not firewall rules 🤦‍♂️.

What is the rule for creating tests? Trying to determine whether this change needs a test or not. Seems rather trivial so I didn't include one but I can fix that if necessary.

[ryaeng]

Seems there's a conflict. Lemme pull that in real quick and then I'll perform another push.

[david-crespo]

Ok, this is good! Thank you!

Re: tests, I would have said no additional test is required beyond what we already have for basic functionality, but the vpc_id == instance_id mistake was not caught by any existing test. So if you want to add one that would have caught that, that would be great.

[ryaeng]

@david-crespo Came back around to this one. Should be up to par. Thanks.

[david-crespo]

Thanks for your work on this @ryaeng, but in #1495 we decided that even an endpoint with a capped-length result set should still return the paginated interface for consistency (and flexibility, in case we paginate later). So I'm going to close this and leave the endpoint as-is.