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

Use Vec instead of HashMap for firewall rules, don't paginate #668

Merged
merged 4 commits into from
Feb 7, 2022

Conversation

david-crespo
Copy link
Contributor

@david-crespo david-crespo commented Feb 4, 2022

As discussed in the 2/4/22 control plane huddle, after working on the firewall rules UI, I found some issues that are orthogonal to and less controversial than the PUT vs. POST debate in #623.

Issues

  • Firewall rules are the only collection in the API that we represent as a hashmap instead of a list
  • The GET endpoint returns a list but the PUT endpoint expects a hashmap. These should be the same, one or the other.
  • The GET endpoint is paginated, which would require the client to pull all the pages before doing a PUT

Fixes proposed here

  • Standardize on using a list of rules in both GET response body and PUT request/response body
  • Don't paginate GET

#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
pub struct VpcFirewallRules {
pub rules: Vec<VpcFirewallRule>,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should be called a VpcFirewall and the endpoint should simply be /firewall. That way a Firewall is essentially the name for a collection of rules. I don't see why we would want to have multiple firewalls per VPC, but we don't have to support that. It could just be /vpc/:vpcName/firewall and there is exactly one firewall.

@david-crespo david-crespo marked this pull request as ready for review February 4, 2022 21:57
@david-crespo david-crespo changed the title Firewall rules API tweaks Use Vec instead of HashMap for firewall rules, don't paginate Feb 4, 2022
}

impl FromIterator<VpcFirewallRule> for VpcFirewallRuleUpdateResult {
impl FromIterator<VpcFirewallRule> for VpcFirewallRules {

This comment was marked as resolved.

@@ -1753,22 +1749,15 @@ impl Nexus {
project_name: &Name,
vpc_name: &Name,
params: &VpcFirewallRuleUpdateParams,
) -> UpdateResult<VpcFirewallRuleUpdateResult> {
) -> UpdateResult<Vec<db::model::VpcFirewallRule>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

functions in this file should return models

# Conflicts:
#	nexus/src/nexus.rs
@david-crespo
Copy link
Contributor Author

@rmustacc this change implies a change to RFD 21. Should I open a PR there if this gets merged?

@david-crespo
Copy link
Contributor Author

I'm going to merge this because it's blocking me on console. I've gotten independent confirmation from @davepacheco and @rmustacc that this approach is good. I will tweak in a followup if necessary.

@david-crespo david-crespo merged commit f615ee4 into main Feb 7, 2022
@david-crespo david-crespo deleted the firewall-rule-tweaks branch February 7, 2022 22:38
@rmustacc
Copy link

rmustacc commented Feb 8, 2022

@david-crespo let's get that PR open as that's the main way for us to track the actual bits that we want to have. I'll try to get through the missed discussion to make sure I'm fully up to speed. In the future, I'd prefer to see the RFD updated before we land changes like this so we don't end up churning too much.

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.

2 participants