Skip to content

refactor(digital-ocean): Use tags to manage firewall and droplets - #6242

Merged
Aradhya-Tripathi merged 3 commits into
developfrom
do-limits
Apr 24, 2026
Merged

refactor(digital-ocean): Use tags to manage firewall and droplets#6242
Aradhya-Tripathi merged 3 commits into
developfrom
do-limits

Conversation

@Aradhya-Tripathi

@Aradhya-Tripathi Aradhya-Tripathi commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Ensure we use tags to group and add multiple droplets to a firewall since, digital ocean caps number of droplets attached to a firewall to 10.

@greptile-apps

greptile-apps Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR refactors DigitalOcean droplet provisioning to use a tag-based firewall model: a new cluster-scoped tag (Frappe-Cloud-Production-<cluster>) is attached to the droplet on creation, and firewalls.add_tags is called instead of the previous firewalls.assign_droplets.

  • Firewall fan-out (P1): droplet_tag is shared by every machine in the cluster. When a series == \"n\" or series == \"nat\" machine is provisioned, add_tags registers this cluster-wide tag against proxy_security_group_id / nat_security_group_id, meaning ALL other cluster droplets (app servers, DB servers, etc.) immediately inherit those series-specific firewall rules. A per-series or per-role tag is needed to preserve the original per-machine scoping.
  • Firewall status guard (P2): The pre-check on cluster.security_group_id does not cover self.security_group_id or the series-specific IDs actually passed to add_tags.

Confidence Score: 3/5

Not safe to merge as-is — the shared cluster tag causes series-specific firewalls to fan out to all cluster droplets, widening firewall rules beyond their intended scope.

The tag-based approach is a sound idea, but using a single cluster-scoped tag means that provisioning any special-series machine (proxy, NAT) attaches that series' firewall to every other droplet in the cluster. This is a present security-rule scoping defect on the changed code path, warranting a P1 rating and a score below 4.

press/press/doctype/virtual_machine/virtual_machine.py — specifically the tag naming strategy and the firewalls.add_tags loop.

Important Files Changed

Filename Overview
press/press/doctype/virtual_machine/virtual_machine.py Switches DigitalOcean firewall assignment from per-droplet (assign_droplets) to tag-based (add_tags) using a single cluster-scoped tag, which causes all machines in the cluster to inherit every series-specific firewall (proxy, NAT) whenever any such machine is provisioned.

Sequence Diagram

sequenceDiagram
    participant P as _provision_digital_ocean
    participant DO_Droplets as DO Droplets API
    participant DO_Firewalls as DO Firewalls API

    P->>DO_Firewalls: firewalls.list()
    DO_Firewalls-->>P: all firewalls
    P->>P: check cluster_firewall status (cluster.security_group_id)

    P->>DO_Droplets: droplets.create({..., tags: [droplet_tag]})
    DO_Droplets-->>P: droplet (instance_id)

    loop for each group in get_security_groups()
        Note over P: groups = [self.security_group_id,<br/>+ proxy_security_group_id (series=n)<br/>+ nat_security_group_id (series=nat)]
        P->>DO_Firewalls: firewalls.add_tags(group, {tags: [droplet_tag]})
        Note over DO_Firewalls: tag is cluster-scoped - ALL cluster droplets now inherit this firewall rule
        DO_Firewalls-->>P: ok
    end

    P->>P: save() & db.commit()
Loading

Comments Outside Diff (1)

  1. press/press/doctype/virtual_machine/virtual_machine.py, line 492-498 (link)

    P2 Cluster firewall status check is for a different firewall than what gets tagged

    cluster_firewall validates cluster.security_group_id, but the firewalls that add_tags is subsequently called on come from get_security_groups(), which returns self.security_group_id (a VM-level field) and possibly the cluster's proxy_security_group_id / nat_security_group_id. If self.security_group_id is in a failed state the guard on line 497–498 won't catch it, and add_tags will silently succeed on a broken firewall.

Reviews (1): Last reviewed commit: "refactor(digital-ocean): Add tags to dro..." | Re-trigger Greptile

Comment thread press/press/doctype/virtual_machine/virtual_machine.py
Comment thread press/press/doctype/virtual_machine/virtual_machine.py
@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (develop@e57ade8). Learn more about missing BASE report.
⚠️ Report is 141 commits behind head on develop.

Files with missing lines Patch % Lines
...s/press/doctype/virtual_machine/virtual_machine.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #6242   +/-   ##
==========================================
  Coverage           ?   55.93%           
==========================================
  Files              ?      910           
  Lines              ?    75758           
  Branches           ?      524           
==========================================
  Hits               ?    42373           
  Misses             ?    33357           
  Partials           ?       28           
Flag Coverage Δ
dashboard 90.75% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Aradhya-Tripathi
Aradhya-Tripathi merged commit 3eba831 into develop Apr 24, 2026
15 checks passed
@Aradhya-Tripathi
Aradhya-Tripathi deleted the do-limits branch April 24, 2026 06:17
Aradhya-Tripathi added a commit that referenced this pull request Apr 24, 2026
refactor(digital-ocean): Use tags to manage firewall and droplets (backport #6242)
@frappe-pr-bot

Copy link
Copy Markdown
Collaborator

🎉 This PR is included in version 0.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants