-
Notifications
You must be signed in to change notification settings - Fork 18
feat: Add firewall rule enable/disable commands #630
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
Open
mgajda
wants to merge
35
commits into
UpCloudLtd:main
Choose a base branch
from
mgajda:feat/firewall-rule-enable-disable
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…content-type flag - Support IANA-registered content types: application/gzip, application/x-xz, application/x-tar, application/x-bzip2, application/x-7z-compressed, application/zip, and application/octet-stream - Add --content-type flag to allow explicit content type specification - Auto-detect content type from file extension with fallback to octet-stream - Add getContentType() function for extensible content type mapping - Add comprehensive unit tests for content type detection Fixes UpCloudLtd#581
- Move supportedContentTypes map to package level for reusability - Add getSupportedExtensionsText() to list all supported file extensions - Add getSupportedContentTypesText() to list all supported content types - Update --source-location help to show supported extensions dynamically - Update --content-type help to show supported types dynamically - Simplify getContentType() to use the package-level map - Help text now automatically reflects all supported formats
kangasta
requested changes
Dec 29, 2025
Contributor
Author
|
Summary of changes: All three work with filters:
Filter Behavior:
Additional Options:
@kangasta Is it what you meant? |
5c1bfce to
00132ed
Compare
Implements UpCloudLtd#244 - Enable/disable specific firewall rules via CLI UpCloud API does not support modifying individual firewall rules directly. Instead, this implementation: 1. Fetches all firewall rules for the server 2. Modifies the target rule's action field (accept/drop) 3. Replaces the entire ruleset atomically using CreateFirewallRules This pattern follows the Terraform provider implementation and is the only viable approach with current UpCloud API design. Changes: - Add `upctl server firewall rule enable <uuid> --position <N>` command - Add `upctl server firewall rule disable <uuid> --position <N>` command - Comprehensive tests for both commands - Input validation for position (1-1000) - Clear error messages when rule position not found Usage Examples: # Create firewall rules first upctl server firewall create my-server --direction in --action drop --comment "Block SSH" --protocol tcp --destination-port-start 22 --destination-port-end 22 upctl server firewall create my-server --direction in --action accept --comment "Allow HTTP" --protocol tcp --destination-port-start 80 --destination-port-end 80 upctl server firewall create my-server --direction in --action accept --comment "Allow HTTPS" --protocol tcp --destination-port-start 443 --destination-port-end 443 # View current rules to get positions upctl server firewall show my-server # Enable a specific rule (sets action to "accept") upctl server firewall rule enable my-server --position 1 # Disable a specific rule (sets action to "drop") upctl server firewall rule disable my-server --position 3 Note: Rules are identified by position (1-1000). Use the comment field when creating rules to help identify them later.
…commands Extends firewall rule enable/disable commands with multiple filter options beyond just position-based selection, addressing feedback on issue UpCloudLtd#244. The same filter options used when creating firewall rules (--direction, --protocol, --dest-port, --src-address, --comment) can now be used to select which existing rules to enable or disable. Filter options (can be combined): - --comment: Match rules by comment text (partial, case-insensitive) - --direction: Filter by direction (in/out) - --protocol: Filter by protocol (tcp/udp/icmp) - --dest-port: Match destination port - --src-address: Match source IP address (partial) - --position: Match exact position (mutually exclusive with others) Multiple filters use AND logic to narrow results. For example: upctl server firewall rule enable <uuid> --comment "Dev" --direction in --protocol tcp Safety features: - --skip-confirmation N: Set max rules to modify without confirmation - Default: confirms if modifying >1 rule - Lists all matching rules before requiring confirmation Examples prioritize comment-based selection over position, as position-based selection is fragile when rules are reordered. Comments provide stable, human-readable rule identification. Resolves: UpCloudLtd#244
Verifies that --skip-confirmation 0 correctly requires confirmation even for a single rule modification, ensuring users can always opt into manual confirmation for safety.
Improve documentation for --skip-confirmation flag to clearly explain that setting it to 0 will always require confirmation, even for a single rule modification.
Extract common filter flag definitions, matching logic, and rule modification code into rule_modify_shared.go to eliminate duplication between enable and disable commands. Benefits: - Reduces code from ~200 lines each to ~45 lines per command - Single source of truth for filter logic and validation - Easier to maintain and extend with new features - All tests pass, no behavior changes The shared functions: - addRuleFilterFlags: Defines all filter flags - configureRuleFilterFlagsPostAdd: Sets up mutual exclusivity and completion - findMatchingRules: Filters rules based on criteria - modifyFirewallRules: Main modification logic with confirmation
Implements intelligent shell completion for: - --dest-port: Suggests common ports (SSH, HTTP, HTTPS, databases) and parses /etc/services for well-known TCP/UDP ports - --src-address: Suggests common IP ranges (private networks, localhost, any IPv4/IPv6) - --skip-confirmation: Suggests 0 (always confirm) and 10 (batch operations) This improves UX by helping users discover valid values without consulting documentation.
- Remove FTP (21) - not recommended for production use - Remove all Dev-Server entries (3000, 5000) - not relevant for firewall rules - Update descriptions for ports 80, 8000, 8080 to clarify HTTP/HTTPS usage Changed from 'HTTP-Alt' to 'HTTP (or HTTPS w/TLS)' for clarity
…tive to catch-all - delete command now supports filters to delete multiple rules - enable command moves rules before catch-all drop rule (activates them) - disable command moves rules after catch-all drop rule (deactivates them) - consistent filter flags across delete/enable/disable commands This addresses PR review feedback to make command semantics clearer: - create/delete for rule CRUD operations - enable/disable for rule activation by position relative to catch-all
…oudLtd#611) See https://github.com/yaml/go-yaml#project-status. Various dependencies have already switched, most recently [cobra in 1.10.2](https://github.com/spf13/cobra/releases/tag/v1.10.2). For a diff between the old 3.0.1 and the new 3.0.2, see https://gist.github.com/scop/6ec72debf62a9603cff9dc97e6814ddd. 3.0.1 are identical. Upgrade to 3.0.4 while at it: yaml/go-yaml@v3.0.2...v3.0.4
Big PR, but the bulk of it are modernize's `interface{}` to `any`
autofixes.
---------
Co-authored-by: Ville Välimäki <ville.valimaki@upcloud.com>
…Ltd#627) Co-authored-by: Toni Kangas <kangasta@users.noreply.github.com>
…Ltd#649) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Closes UpCloudLtd#339 Co-authored-by: Toni Kangas <toni.kangas@upcloud.com>
…Ltd#651) Co-authored-by: Toni Kangas <toni.kangas@upcloud.com>
…pCloudLtd#652) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…UpCloudLtd#646) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…UpCloudLtd#648) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- Replace server.FirewallRules with GetFirewallRules API call - Fix CreateFirewallRule to use upcloud.FirewallRule type - Replace ui.Confirm with error-based confirmation pattern - Fix PushProgress method calls to match Executor interface - Add strings import for string operations
- Update mocks to use GetFirewallRules() instead of server.FirewallRules - Mock DeleteFirewallRule and CreateFirewallRule for enable/disable operations - Fix expected error messages to match new implementation behavior - All firewall command tests now pass successfully
067b041 to
4549fc7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements
upctl server firewall rule enableandupctl server firewall rule disablecommands to modify existing firewall rules.Features:
Closes: #244