Firewalls on subnet UI #4
Conversation
We made the decision to make Firewalls to be added to the whole subnet instead of individual VMs. This commit implements the migration file.
This commit implements the Firewalls move from VMs to Subnets. Therefore, there are multiple changes regarding model relationships, Vm::Nexus.assemble, Vnet::SubnetNexus.assemble and finally at the routes. The changes are not very interesting as they mostly involve semaphore increments being performed on subnets instead of individual VMs or entity creations referring to subnets intead of VMs. One additional small but interesting change is the cidr validation. It involves 2 changes; 1. Validate IPv6 as well 2. Return the parsed cidr and use its string representation while creating the record. This is necessary because when NetAddr is able to parse a cidr like "1.1.1.1/8" without an issue, db insert fails because the valid form of that cidr is actually "1.0.0.0/8". This used to cause 500 error in console.
With this commit, our backend starts supporting Firewalls <-> PrivateSubnets many_to_many relationship. For that to work, we introduce a new model called FirewallsPrivateSubnets and maintain the relationship properly at the de/provisioning times.
This commit updates sidebar to have firewalls. Additionally, we change the Networking tab to Private Subnet to make it more inline with the endpoint name.
This commit does not only adds a show page but implements various functionality to manage Firewall <-> Private Subnet and Firewall <-> Firewall Rules relationships. Here are the itemized actions; 1. Show Firewall. 2. Show Firewall Rules. 3. List Attached Private Subnets. 4. Attach Firewall to a Private Subnet. 5. Detach Firewall from a Private Subnet. 6. Add a Firewall Rule to Firewall. 7. Remove a Firewall Rule from a Firewall.
This commit removes the Network Interfaces box and refactors the Firewall Rules in the Vm Show page. The main reason we remove the Nics is that we do not have multiple Nics infrastructure just yet. It is staying in the main VM page unnecessarily. Also, with the introduction of the new multiple Firewalls functionality, the main VM page would get longer. Instead; 1. Since every VM will have a single Private Subnet and the Firewalls are attached to it, I have moved the private IPv4/6 addresses to the main details card. 2. Removed the Nics box. 3. Added a new line to see the private subnet. 4. Added a new box for Firewalls to list the firewall and the applied rules. 5. Remove Firewall rule editing from the VM. This is important because now firewall rules are applied at the subnet level and editing it directly in the VM page may result in a change that impacts more than one resources.
As firewalls are started to be associated with private subnets, adding private subnet information to the detailed firewall api serialization.
Adding API endpoints to manage firewalls. Endpoints include - Adding Firewall - Deleting Firewall - Getting/Listing Firewalls - Associate/Dissociate with subnets Firewall rule management will be added with the subsequent commit.
Adding API endpoints to manage firewall rules. Endpoints include - Adding firewall rules - Deleting firewall rules
![greptile-apps-local[bot]](https://avatars.githubusercontent.com/u/140149887?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request significantly enhances the management and representation of firewalls within the system, focusing on their relationship with private subnets and virtual machines (VMs). Key changes include:
- Introduction of UI and API updates for managing firewalls, including new pages for creating, showing, and listing firewalls, as well as attaching them to private subnets.
- Migration of firewall associations from VMs to private subnets, allowing multiple firewalls to be attached to a single private subnet.
- Refactoring of the VM page to better integrate with the new firewall management approach, removing direct firewall rule editing from VMs to reflect the shift towards subnet-level rule management.
- Updates to various models, routes, and serializers to support these changes, ensuring that firewalls are correctly managed and represented in relation to private subnets and VMs.
Optional notes:
- The migration scripts and model updates are critical for transitioning to the new architecture, but careful attention should be paid to ensure data integrity and correct functionality during and after migration.
- The changes open opportunities for further enhancements in network security management and could serve as a foundation for introducing more granular control over firewall rules and associations in future updates.
| run <<~SQL | ||
| UPDATE firewall f | ||
| SET vm_id = ( | ||
| SELECT n.vm_id | ||
| FROM nic n | ||
| WHERE n.private_subnet_id = f.private_subnet_id | ||
| ); |
There was a problem hiding this comment.
The down migration assumes a one-to-one relationship between nic and private_subnet, which might not hold true if a subnet is linked to multiple VMs. This could result in incorrect vm_id assignments when rolling back. Consider adding a validation or a more complex SQL query to handle cases where a private_subnet is associated with multiple VMs.
| SQL | ||
|
|
||
| alter_table(:firewall) do | ||
| drop_column :private_subnet_id |
There was a problem hiding this comment.
Before dropping the private_subnet_id column, ensure that no code paths rely on this column. This change might require updates in model associations or queries that reference private_subnet_id directly.
| class FirewallsPrivateSubnets < Sequel::Model | ||
| include ResourceMethods | ||
| end |
There was a problem hiding this comment.
Define associations for FirewallsPrivateSubnets. Consider adding many_to_many associations to link Firewalls and PrivateSubnets, enabling the management of their relationship.
| boot_image: boot_image, ip4_enabled: enable_ip4, pool_id: pool_id, arch: arch) { _1.id = ubid.to_uuid } | ||
| nic.update(vm_id: vm.id) | ||
|
|
||
| port_range = allow_only_ssh ? 22..22 : 0..65535 | ||
| fw = Firewall.create_with_id(vm_id: vm.id, name: "#{name}-default") | ||
| ["0.0.0.0/0", "::/0"].each { |cidr| FirewallRule.create_with_id(firewall_id: fw.id, cidr: cidr, port_range: Sequel.pg_range(port_range)) } | ||
|
|
||
| vm.associate_with_project(project) |
There was a problem hiding this comment.
The removal of direct firewall rule creation for VMs here is crucial. Confirm that all necessary firewall configurations are now adequately managed at the subnet level and that this change does not inadvertently affect VM security or connectivity.
| port_range = if request_body_params["port_range"].nil? | ||
| [0, 65535] | ||
| else | ||
| request_body_params["port_range"] = Validation.validate_port_range(request_body_params["port_range"]) | ||
| end |
There was a problem hiding this comment.
The assignment to request_body_params["port_range"] within the else block does not affect the port_range variable. Consider directly assigning the validated port range to port_range.
| private_ip4: vm.nics.first.private_ipv4.network, | ||
| private_ip6: vm.nics.first.private_ipv6.nth(2), | ||
| subnet: vm.nics.first.private_subnet.name |
There was a problem hiding this comment.
Assuming vm.nics.first exists and has the necessary associated objects (private_ipv4, private_ipv6, private_subnet) could lead to errors if a VM lacks NICs or these associations. Consider adding checks to ensure these objects exist before accessing their properties.
| [[{subjects: users[0].hyper_tag_name, actions: "Vm:view", objects: projects[0].hyper_tag_name}], users[0].id, "Vm:view", 8], | ||
| [[{subjects: users[0].hyper_tag_name, actions: "Vm:view", objects: projects[0].hyper_tag_name}], users[0].id, ["Vm:view", "Vm:create"], 8], | ||
| [[{subjects: [users[0].hyper_tag_name], actions: ["Vm:view"], objects: [projects[0].hyper_tag_name]}], users[0].id, "Vm:view", 8], | ||
| [[{subjects: [users[0].hyper_tag_name, users[1].hyper_tag_name], actions: ["Vm:view", "Vm:delete"], objects: [projects[0].hyper_tag_name]}], users[0].id, ["Vm:view", "Vm:create"], 8], |
There was a problem hiding this comment.
The increase in matched policies count from 6 to 8 should be verified to ensure it aligns with the intended authorization logic changes, especially considering the new firewall associations.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Summary: Firewalls on Subnet UI
- Enhanced Firewall Management: Firewalls are now managed at the subnet level instead of the VM level, including attaching/detaching firewalls to/from subnets and managing firewall rules.
- UI Updates: Added new pages for creating, showing, and listing firewalls, and updated the VM and subnet pages to reflect the new firewall management approach.
- Database Migrations: Introduced new migrations to support the many-to-many relationship between firewalls and subnets, including adding and removing relevant columns and tables.
- Code Refactoring: Updated models, routes, and serializers to align with the new firewall management structure, ensuring consistent and efficient handling of firewall rules.
Additional Notes:
- Potential Pitfalls: Ensure that the new many-to-many relationships are correctly managed to avoid data integrity issues.
- Opportunities for Reuse: The new
FirewallsPrivateSubnetsmodel and related methods can be reused for other similar many-to-many relationships in the future.
| @@ -121,9 +121,15 @@ def self.validate_postgres_superuser_password(original_password, repeat_password | |||
| end | |||
|
|
|||
| def self.validate_cidr(cidr) | |||
There was a problem hiding this comment.
Good enhancement to support both IPv4 and IPv6 CIDR formats.
| rescue NetAddr::ValidationError | ||
| fail ValidationFailed.new({CIDR: "Invalid CIDR"}) | ||
| fail ValidationFailed.new({cidr: "Invalid CIDR"}) |
There was a problem hiding this comment.
Consider using consistent key casing in the error message (e.g., 'cidr' instead of 'CIDR').
| Sequel.migration do | ||
| up do | ||
| alter_table(:firewall) do | ||
| add_foreign_key :private_subnet_id, :private_subnet, type: :uuid |
There was a problem hiding this comment.
Consider adding null: false to the private_subnet_id column if it should always be present.
| UPDATE firewall f | ||
| SET private_subnet_id = ( | ||
| SELECT n.private_subnet_id | ||
| FROM nic n | ||
| WHERE n.vm_id = f.vm_id | ||
| ); |
There was a problem hiding this comment.
Ensure that the subquery in the UPDATE statement does not return multiple rows, which would cause an error.
| UPDATE firewall f | ||
| SET vm_id = ( | ||
| SELECT n.vm_id | ||
| FROM nic n | ||
| WHERE n.private_subnet_id = f.private_subnet_id | ||
| ); |
There was a problem hiding this comment.
Ensure that the subquery in the down migration's UPDATE statement does not return multiple rows, which would cause an error.
| firewalls: vm.firewalls.map { |fw| Serializers::Web::Firewall.serialize(fw) }, | ||
| private_ip4: vm.nics.first.private_ipv4.network, | ||
| private_ip6: vm.nics.first.private_ipv6.nth(2), | ||
| subnet: vm.nics.first.private_subnet.name |
There was a problem hiding this comment.
Consider adding a nil check for vm.nics.first.private_ipv6 to avoid potential NoMethodError if private_ipv6 is nil.
| firewalls: vm.firewalls.map { |fw| Serializers::Web::Firewall.serialize(fw) }, | ||
| private_ip4: vm.nics.first.private_ipv4.network, | ||
| private_ip6: vm.nics.first.private_ipv6.nth(2), | ||
| subnet: vm.nics.first.private_subnet.name | ||
| ) |
There was a problem hiding this comment.
Consider adding a nil check for vm.nics.first.private_subnet to avoid potential NoMethodError if private_subnet is nil.
| [[{subjects: users[0].hyper_tag_name, actions: "Vm:view", objects: projects[0].hyper_tag_name}], users[0].id, "Vm:view", 8], | ||
| [[{subjects: users[0].hyper_tag_name, actions: "Vm:view", objects: projects[0].hyper_tag_name}], users[0].id, ["Vm:view", "Vm:create"], 8], | ||
| [[{subjects: [users[0].hyper_tag_name], actions: ["Vm:view"], objects: [projects[0].hyper_tag_name]}], users[0].id, "Vm:view", 8], | ||
| [[{subjects: [users[0].hyper_tag_name, users[1].hyper_tag_name], actions: ["Vm:view", "Vm:delete"], objects: [projects[0].hyper_tag_name]}], users[0].id, ["Vm:view", "Vm:create"], 8], |
There was a problem hiding this comment.
Updated expected counts to reflect the addition of firewalls associated with VMs and private subnets.
| @@ -87,7 +87,7 @@ | |||
|
|
|||
| describe "#authorized_resources" do | |||
| it "returns resource ids when has matched policies" do | |||
| ids = [vms[0].id, vms[1].id, projects[0].id, users[0].id, vms[0].private_subnets[0].id, vms[1].private_subnets[0].id] | |||
| ids = [vms[0].id, vms[1].id, projects[0].id, users[0].id, vms[0].private_subnets[0].id, vms[1].private_subnets[0].id, vms[0].firewalls[0].id, vms[1].firewalls[0].id] | |||
There was a problem hiding this comment.
Added firewall IDs to the list of expected authorized resource IDs.
| describe "destroy" do | ||
| it "destroys firewalls private subnets" do | ||
| ps = described_class.create_with_id(name: "test-ps", location: "hetzner-hel1", net6: "2001:db8::/64", net4: "10.0.0.0/24") | ||
| fwps = instance_double(FirewallsPrivateSubnets) | ||
| expect(FirewallsPrivateSubnets).to receive(:where).with(private_subnet_id: ps.id).and_return(instance_double(Sequel::Dataset, all: [fwps])) | ||
| expect(fwps).to receive(:destroy).once | ||
| ps.destroy | ||
| end |
There was a problem hiding this comment.
Good addition of the test case to ensure associated firewalls are destroyed when a private subnet is deleted.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
PR Summary
- Updated
lib/validation.rbto handle both IPv4 and IPv6 CIDR formats. - Added migrations to associate firewalls with private subnets and remove VM associations.
- Modified
model/firewall.rbto manage relationships withPrivateSubnets. - Introduced new API endpoints for managing firewalls and firewall rules.
- Updated serializers to include detailed firewall and subnet information.
| hash_branch(:project_firewall_prefix, "firewall-rule") do |r| | ||
| @serializer = Serializers::Api::FirewallRule | ||
|
|
||
| r.post true do |
There was a problem hiding this comment.
Ensure @firewall is properly initialized before this line.
|
|
||
| request_body_params = Validation.validate_request_body(request.body.read, required_parameters, allowed_optional_parameters) | ||
|
|
||
| parsed_cidr = Validation.validate_cidr(request_body_params["cidr"]) |
There was a problem hiding this comment.
Add error handling for invalid CIDR formats.
| port_range = if request_body_params["port_range"].nil? | ||
| [0, 65535] | ||
| else | ||
| request_body_params["port_range"] = Validation.validate_port_range(request_body_params["port_range"]) |
There was a problem hiding this comment.
Ensure validate_port_range returns a valid range or raises an appropriate error.
| end | ||
|
|
||
| r.is String do |firewall_rule_ubid| | ||
| firewall_rule = FirewallRule.from_ubid(firewall_rule_ubid) |
There was a problem hiding this comment.
Add error handling for cases where firewall_rule is not found.
| end | ||
|
|
||
| r.post true do | ||
| Authorization.authorize(@current_user.id, "Firewall:create", @project.id) |
There was a problem hiding this comment.
Ensure r.params["name"] is sanitized to prevent injection attacks.
| ) | ||
| fw.associate_with_project(@project) | ||
|
|
||
| ps = PrivateSubnet.from_ubid(r.params["private-subnet-id"]) |
There was a problem hiding this comment.
Check if PrivateSubnet.from_ubid(r.params["private-subnet-id"]) returns nil to avoid potential errors.
| r.on "attach-subnet" do | ||
| r.post true do | ||
| Authorization.authorize(@current_user.id, "Firewall:view", fw.id) | ||
| ps = PrivateSubnet.from_ubid(r.params["private-subnet-id"]) |
There was a problem hiding this comment.
Handle the case where PrivateSubnet.from_ubid(r.params["private-subnet-id"]) returns nil to avoid potential errors.
| r.on "detach-subnet" do | ||
| r.post true do | ||
| Authorization.authorize(@current_user.id, "Firewall:view", fw.id) | ||
| ps = PrivateSubnet.from_ubid(r.params["private-subnet-id"]) |
There was a problem hiding this comment.
Handle the case where PrivateSubnet.from_ubid(r.params["private-subnet-id"]) returns nil to avoid potential errors.
| r.post true do | ||
| Authorization.authorize(@current_user.id, "Firewall:edit", fw.id) | ||
|
|
||
| port_range = if r.params["port_range"].empty? |
There was a problem hiding this comment.
Consider validating r.params["port_range"] before using it to avoid potential errors.
| Validation.validate_port_range(r.params["port_range"]) | ||
| end | ||
|
|
||
| parsed_cidr = Validation.validate_cidr(r.params["cidr"]) |
There was a problem hiding this comment.
Ensure Validation.validate_cidr(r.params["cidr"]) handles invalid CIDR inputs gracefully.
Update Sidebar to have Firewalls
This commit updates sidebar to have firewalls. Additionally, we change
the Networking tab to Private Subnet to make it more inline with the
endpoint name.
Add Firewall Index page
Screenshot 2024-04-30 at 17 57 10
Add Firewall Create page
Screenshot 2024-04-30 at 17 57 37
Add Firewall Show page
This commit does not only adds a show page but implements various
functionality to manage Firewall <-> Private Subnet and Firewall <->
Firewall Rules relationships. Here are the itemized actions;
Show Firewall.
Show Firewall Rules.
List Attached Private Subnets.
Attach Firewall to a Private Subnet.
Detach Firewall from a Private Subnet.
Add a Firewall Rule to Firewall.
Remove a Firewall Rule from a Firewall.
Screenshot 2024-04-30 at 17 57 57
Refactor VM Page to accommodate the Firewalls in a better way
This commit removes the Network Interfaces box and refactors the
Firewall Rules in the Vm Show page. The main reason we remove the Nics
is that we do not have multiple Nics infrastructure just yet. It is
staying in the main VM page unnecessarily. Also, with the introduction
of the new multiple Firewalls functionality, the main VM page would get
longer. Instead;
Since every VM will have a single Private Subnet and the Firewalls
are attached to it, I have moved the private IPv4/6 addresses to the
main details card.
Removed the Nics box.
Added a new line to see the private subnet.
Added a new box for Firewalls to list the firewall and the applied
rules.
Remove Firewall rule editing from the VM. This is important because
now firewall rules are applied at the subnet level and editing it
directly in the VM page may result in a change that impacts more than
one resources.
Screenshot 2024-04-30 at 17 58 18
Add attached Firewalls to the Subnet show page
Screenshot 2024-04-30 at 17 58 39