-
Notifications
You must be signed in to change notification settings - Fork 0
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
Firewalls on subnet UI #4
base: main
Are you sure you want to change the base?
Changes from all commits
542d30d
91d1f3c
a8e5709
110d10e
fcd6a33
07ce1ce
06150d3
1e1709b
19307d2
098b15b
d13acb2
77a9bda
f917623
09e9cb7
ac91c8f
c60bf21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,9 +121,15 @@ def self.validate_postgres_superuser_password(original_password, repeat_password | |
end | ||
|
||
def self.validate_cidr(cidr) | ||
NetAddr::IPv4Net.parse(cidr) | ||
if cidr.include?(".") | ||
NetAddr::IPv4Net.parse(cidr) | ||
elsif cidr.include?(":") | ||
NetAddr::IPv6Net.parse(cidr) | ||
else | ||
fail ValidationFailed.new({cidr: "Invalid CIDR"}) | ||
end | ||
rescue NetAddr::ValidationError | ||
fail ValidationFailed.new({CIDR: "Invalid CIDR"}) | ||
fail ValidationFailed.new({cidr: "Invalid CIDR"}) | ||
Comment on lines
123
to
+132
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the new IPv6 validation logic correctly handles edge cases, such as abbreviated IPv6 addresses and dual-stack (IPv4-mapped IPv6) addresses, to prevent potential validation bypasses or incorrect rejections. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using consistent key casing in the error message (e.g., 'cidr' instead of 'CIDR'). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed error message key from 'CIDR' to 'cidr' for consistency. |
||
end | ||
|
||
def self.validate_port_range(port_range) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# frozen_string_literal: true | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding |
||
end | ||
|
||
run <<~SQL | ||
UPDATE firewall f | ||
SET private_subnet_id = ( | ||
SELECT n.private_subnet_id | ||
FROM nic n | ||
WHERE n.vm_id = f.vm_id | ||
); | ||
Comment on lines
+10
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the subquery in the UPDATE statement does not return multiple rows, which would cause an error. |
||
SQL | ||
end | ||
|
||
down do | ||
run <<~SQL | ||
UPDATE firewall f | ||
SET vm_id = ( | ||
SELECT n.vm_id | ||
FROM nic n | ||
WHERE n.private_subnet_id = f.private_subnet_id | ||
); | ||
Comment on lines
+20
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Comment on lines
+21
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the subquery in the down migration's UPDATE statement does not return multiple rows, which would cause an error. |
||
SQL | ||
|
||
alter_table(:firewall) do | ||
drop_column :private_subnet_id | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# frozen_string_literal: true | ||
|
||
Sequel.migration do | ||
up do | ||
create_table(:firewalls_private_subnets) do | ||
foreign_key :private_subnet_id, :private_subnet, type: :uuid | ||
foreign_key :firewall_id, :firewall, type: :uuid | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure the referenced table names are plural as per convention. It should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use singular, which is unusual. And so, this is incorrect. |
||
primary_key %i[private_subnet_id firewall_id] | ||
end | ||
|
||
run <<~SQL | ||
INSERT INTO firewalls_private_subnets (private_subnet_id, firewall_id) | ||
SELECT private_subnet_id, id AS firewall_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the |
||
FROM firewall; | ||
SQL | ||
|
||
alter_table(:firewall) do | ||
drop_column :private_subnet_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify cascading effects of dropping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't really say anything... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before dropping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
|
||
Sequel.migration do | ||
up do | ||
alter_table(:firewall) do | ||
drop_column :vm_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that all references to |
||
end | ||
end | ||
|
||
down do | ||
alter_table(:firewall) do | ||
add_foreign_key :vm_id, :vm, type: :uuid | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,11 +4,28 @@ | |
|
||
class Firewall < Sequel::Model | ||
one_to_many :firewall_rules, key: :firewall_id | ||
many_to_one :vm, key: :vm_id | ||
many_to_many :private_subnets | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
plugin :association_dependencies, firewall_rules: :destroy | ||
|
||
include ResourceMethods | ||
include Authorization::TaggableMethods | ||
include Authorization::HyperTagMethods | ||
def hyper_tag_name(project) | ||
"project/#{project.ubid}/firewall/#{ubid}" | ||
end | ||
|
||
dataset_module Pagination | ||
dataset_module Authorization::Dataset | ||
|
||
def path | ||
"/firewall/#{ubid}" | ||
end | ||
|
||
def remove_firewall_rule(firewall_rule) | ||
firewall_rule.destroy | ||
private_subnets.map(&:incr_update_firewall_rules) | ||
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
end | ||
|
||
def insert_firewall_rule(cidr, port_range) | ||
fwr = FirewallRule.create_with_id( | ||
|
@@ -17,7 +34,29 @@ def insert_firewall_rule(cidr, port_range) | |
port_range: port_range | ||
) | ||
|
||
vm&.incr_update_firewall_rules | ||
private_subnets.each(&:incr_update_firewall_rules) | ||
fwr | ||
end | ||
|
||
def destroy | ||
DB.transaction do | ||
private_subnets.each(&:incr_update_firewall_rules) | ||
FirewallsPrivateSubnets.where(firewall_id: id).all.each(&:destroy) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the |
||
super | ||
Comment on lines
+41
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the |
||
end | ||
end | ||
|
||
def associate_with_private_subnet(private_subnet, apply_firewalls: true) | ||
add_private_subnet(private_subnet) | ||
private_subnet.incr_update_firewall_rules if apply_firewalls | ||
Comment on lines
+49
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For |
||
end | ||
|
||
def disassociate_from_private_subnet(private_subnet, apply_firewalls: true) | ||
FirewallsPrivateSubnets.where( | ||
private_subnet_id: private_subnet.id, | ||
firewall_id: id | ||
).destroy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
|
||
private_subnet.incr_update_firewall_rules if apply_firewalls | ||
Comment on lines
+54
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative "../model" | ||
|
||
class FirewallsPrivateSubnets < Sequel::Model | ||
include ResourceMethods | ||
end | ||
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The model lacks associations (e.g.,
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define associations for FirewallsPrivateSubnets. Consider adding many_to_many associations to link Firewalls and PrivateSubnets, enabling the management of their relationship. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,7 +165,8 @@ def health_monitor_socket_path | |
end | ||
|
||
def create_resource_firewall_rules | ||
fw = Firewall.create_with_id(vm_id: vm.id, name: ubid.to_s, description: "Postgres default firewall") | ||
fw = Firewall.create_with_id(name: ubid.to_s, description: "Postgres default firewall") | ||
fw.add_private_subnet(vm.private_subnets.first) | ||
Comment on lines
+168
to
+169
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that |
||
resource.firewall_rules.each do |pg_fwr| | ||
fw.insert_firewall_rule(pg_fwr.cidr.to_s, Sequel.pg_range(5432..5432)) | ||
end | ||
Comment on lines
167
to
172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ class PrivateSubnet < Sequel::Model | |
many_to_many :vms, join_table: Nic.table_name, left_key: :private_subnet_id, right_key: :vm_id | ||
one_to_many :nics, key: :private_subnet_id | ||
one_to_one :strand, key: :id | ||
one_to_many :firewall_rules | ||
many_to_many :firewalls | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Transition from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the |
||
|
||
PRIVATE_SUBNET_RANGES = [ | ||
"10.0.0.0/8", | ||
|
@@ -23,6 +23,13 @@ def hyper_tag_name(project) | |
|
||
include Authorization::TaggableMethods | ||
|
||
def destroy | ||
DB.transaction do | ||
FirewallsPrivateSubnets.where(private_subnet_id: id).all.each(&:destroy) | ||
super | ||
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
end | ||
Comment on lines
+26
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The custom |
||
end | ||
|
||
def display_location | ||
LocationNameConverter.to_display_name(location) | ||
end | ||
|
@@ -42,7 +49,7 @@ def display_state | |
end | ||
|
||
include SemaphoreMethods | ||
semaphore :destroy, :refresh_keys, :add_new_nic | ||
semaphore :destroy, :refresh_keys, :add_new_nic, :update_firewall_rules | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of |
||
|
||
def self.random_subnet | ||
PRIVATE_SUBNET_RANGES.sample | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,9 +11,8 @@ class Vm < Sequel::Model | |
one_to_one :assigned_vm_address, key: :dst_vm_id, class: :AssignedVmAddress | ||
one_to_many :vm_storage_volumes, key: :vm_id, order: Sequel.desc(:boot) | ||
one_to_one :active_billing_record, class: :BillingRecord, key: :resource_id do |ds| ds.active end | ||
one_to_many :firewalls, key: :vm_id | ||
|
||
plugin :association_dependencies, sshable: :destroy, assigned_vm_address: :destroy, vm_storage_volumes: :destroy, firewalls: :destroy | ||
plugin :association_dependencies, sshable: :destroy, assigned_vm_address: :destroy, vm_storage_volumes: :destroy | ||
Comment on lines
14
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that removing the direct association of firewalls with VMs ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the |
||
|
||
dataset_module Pagination | ||
dataset_module Authorization::Dataset | ||
|
@@ -31,6 +30,10 @@ def hyper_tag_name(project) | |
|
||
include Authorization::TaggableMethods | ||
|
||
def firewalls | ||
private_subnets.flat_map(&:firewalls) | ||
end | ||
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify that the new
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a new |
||
|
||
def display_location | ||
LocationNameConverter.to_display_name(location) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -291,7 +291,6 @@ def before_run | |
|
||
# create a new set of firewall rules | ||
postgres_server.create_resource_firewall_rules | ||
Comment on lines
291
to
293
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the removal of |
||
vm.incr_update_firewall_rules | ||
|
||
hop_wait | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -352,7 +352,10 @@ def setup_info | |
end | ||
|
||
if vm | ||
vm.private_subnets.each { _1.incr_destroy } | ||
vm.private_subnets.each do |subnet| | ||
subnet.firewalls.map(&:destroy) | ||
subnet.incr_destroy | ||
Comment on lines
+355
to
+357
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that destroying firewalls does not violate any foreign key constraints or trigger unintended side effects in the database. Consider adding checks or transactions to handle potential exceptions.
Comment on lines
+355
to
+357
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that destroying firewalls before incrementing the destroy counter for subnets does not introduce any race conditions or dependencies that might affect the destruction process.
Comment on lines
+355
to
+357
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure firewalls are destroyed before subnets. |
||
end | ||
|
||
# If the runner is not assigned any job and we destroy it after a | ||
# timeline, the workflow_job is nil, in that case, we want to be able to | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,7 @@ def self.assemble(public_key, project_id, name: nil, size: "standard-2", | |
raise "Given subnet doesn't exist with the id #{private_subnet_id}" unless subnet | ||
raise "Given subnet is not available in the given project" unless project.private_subnets.any? { |ps| ps.id == subnet.id } | ||
else | ||
subnet_s = Prog::Vnet::SubnetNexus.assemble(project_id, name: "#{name}-subnet", location: location) | ||
subnet_s = Prog::Vnet::SubnetNexus.assemble(project_id, name: "#{name}-subnet", location: location, allow_only_ssh: allow_only_ssh) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing |
||
subnet = PrivateSubnet[subnet_s.id] | ||
end | ||
nic_s = Prog::Vnet::NicNexus.assemble(subnet.id, name: "#{name}-nic") | ||
|
@@ -90,10 +90,6 @@ def self.assemble(public_key, project_id, name: nil, size: "standard-2", | |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removal of default firewall rule creation for the VM is appropriate given the new subnet-level firewall management approach. |
||
|
||
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) | ||
Comment on lines
90
to
93
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
Strand.create( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,9 @@ | |
|
||
class Prog::Vnet::SubnetNexus < Prog::Base | ||
subject_is :private_subnet | ||
semaphore :destroy, :refresh_keys, :add_new_nic | ||
semaphore :destroy, :refresh_keys, :add_new_nic, :update_firewall_rules | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure the new semaphore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a semaphore for |
||
|
||
def self.assemble(project_id, name: nil, location: "hetzner-hel1", ipv6_range: nil, ipv4_range: nil) | ||
def self.assemble(project_id, name: nil, location: "hetzner-hel1", ipv6_range: nil, ipv4_range: nil, allow_only_ssh: false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify the default firewall creation logic, especially the condition for SSH-only access, aligns with security policies. |
||
unless (project = Project[project_id]) | ||
fail "No existing project" | ||
end | ||
|
@@ -20,6 +20,12 @@ def self.assemble(project_id, name: nil, location: "hetzner-hel1", ipv6_range: n | |
DB.transaction do | ||
ps = PrivateSubnet.create(name: name, location: location, net6: ipv6_range, net4: ipv4_range, state: "waiting") { _1.id = ubid.to_uuid } | ||
ps.associate_with_project(project) | ||
port_range = allow_only_ssh ? 22..22 : 0..65535 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure that the |
||
fw = Firewall.create_with_id(name: "#{name}-default") | ||
fw.associate_with_project(project) | ||
["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)) } | ||
fw.associate_with_private_subnet(ps, apply_firewalls: false) | ||
|
||
Strand.create(prog: "Vnet::SubnetNexus", label: "wait") { _1.id = ubid.to_uuid } | ||
end | ||
end | ||
|
@@ -44,6 +50,11 @@ def before_run | |
hop_add_new_nic | ||
end | ||
|
||
when_update_firewall_rules_set? do | ||
private_subnet.vms.map(&:incr_update_firewall_rules) | ||
decr_update_firewall_rules | ||
end | ||
|
||
if private_subnet.last_rekey_at < Time.now - 60 * 60 * 24 | ||
private_subnet.incr_refresh_keys | ||
end | ||
|
@@ -128,6 +139,7 @@ def gen_reqid | |
|
||
decr_destroy | ||
strand.children.each { _1.destroy } | ||
private_subnet.firewalls.map { _1.disassociate_from_private_subnet(private_subnet, apply_firewalls: false) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirm that disassociating firewalls from the private subnet does not inadvertently affect other resources or leave orphaned firewall rules. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check if |
||
|
||
if private_subnet.nics.empty? | ||
DB.transaction do | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enhancement to support both IPv4 and IPv6 CIDR formats.