-
Notifications
You must be signed in to change notification settings - Fork 49
Fix egress firewall rules to support all-ports without explicit ports #218
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
base: main
Are you sure you want to change the base?
Conversation
@sidshas03 Could you please make sure the test pass |
Sure sir. I'm working on it. Sorry for the late reply! |
…t match; UDP acc test; docs (Fixes apache#202)
7d6a79d
to
e16d5a2
Compare
@kiranchavala I’ve pushed updates to support TCP/UDP all-ports when ports are omitted, normalize reads for 0/0, -1/-1, and 1/65535, add an acceptance test, and update the docs. I’m watching CI and will fix anything that fails as soon as the test cases start running. Thanks! Commit: e16d5a2 |
Status update: |
Thanks @sidshas03 i see that tests are failing at this point ![]() cc @vishesh92 |
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.
Pull Request Overview
This PR enables support for all-ports firewall rules in the CloudStack egress firewall resource by making the ports parameter optional for TCP/UDP protocols. When ports are omitted, the provider creates rules that encompass all ports, providing better flexibility for firewall configurations.
- Allow omitting ports parameter for TCP/UDP protocols to create all-ports rules
- Add comprehensive test coverage for all-ports functionality including transitions
- Update documentation with examples demonstrating all-ports usage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
cloudstack/resource_cloudstack_egress_firewall.go |
Core logic to handle all-ports rules creation and reading, with validation changes |
cloudstack/resource_cloudstack_egress_firewall_test.go |
Comprehensive test cases for all-ports functionality and transitions |
website/docs/r/egress_firewall.html.markdown |
Documentation updates with examples and clarification |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
* `ports` - (Optional) List of ports and/or port ranges to allow. This can only | ||
be specified if the protocol is TCP or UDP. | ||
be specified if the protocol is TCP or UDP. For TCP/UDP, omitting `ports` creates an all-ports rule. CloudStack may represent this as empty start/end, `0/0`, or `1/65535`; the provider handles all. |
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.
The sentence structure is unclear and contains a grammatical error. Consider revising to: 'be specified if the protocol is TCP or UDP. For TCP/UDP protocols, omitting ports
creates an all-ports rule. CloudStack may represent this as empty start/end ports, 0/0
, or 1/65535
; the provider handles all formats.'
be specified if the protocol is TCP or UDP. For TCP/UDP, omitting `ports` creates an all-ports rule. CloudStack may represent this as empty start/end, `0/0`, or `1/65535`; the provider handles all. | |
be specified if the protocol is TCP or UDP. For TCP/UDP protocols, omitting `ports` creates an all-ports rule. CloudStack may represent this as empty start/end ports, `0/0`, or `1/65535`; the provider handles all formats. |
Copilot uses AI. Check for mistakes.
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" | ||
) | ||
|
||
// treats 'all ports' for tcp/udp across CS versions returning 0/0, -1/-1, or 1/65535 |
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.
The comment has grammatical issues. Consider revising to: '// isAllPortsTCPUDP determines if a rule represents all ports for TCP/UDP protocols across CloudStack versions that may return 0/0, -1/-1, or 1/65535'
// treats 'all ports' for tcp/udp across CS versions returning 0/0, -1/-1, or 1/65535 | |
// isAllPortsTCPUDP determines if a rule represents all ports for TCP/UDP protocols across CloudStack versions that may return 0/0, -1/-1, or 1/65535 |
Copilot uses AI. Check for mistakes.
// Note: ports parameter is optional for TCP/UDP protocols | ||
// When omitted, the rule will encompass all ports |
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.
[nitpick] These comments are placed in an odd location within the validation logic. Consider moving them to the function-level documentation or closer to where the actual logic handles the omitted ports case for better code organization.
Copilot uses AI. Check for mistakes.
Fixes #202