-
Notifications
You must be signed in to change notification settings - Fork 109
Add FirewallID to NodePool #2169
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: dev
Are you sure you want to change the base?
Conversation
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 adds support for associating firewalls with LKE node pools by introducing a firewall_id field. This enables users to specify which firewall should be attached to nodes in a pool during creation or update operations.
Key Changes:
- Added
firewall_idfield to node pool data models and schemas across both framework and SDK implementations - Implemented create and update logic to handle firewall assignment and changes
- Updated documentation with examples showing firewall usage
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| linode/lkenodepool/framework_resource_schema.go | Added firewall_id schema attribute definition |
| linode/lkenodepool/framework_models.go | Integrated firewall_id into model flattening, create/update operations |
| linode/lke/schema_resource.go | Added firewall_id to pool schema with default value |
| linode/lke/resource.go | Added firewall_id handling in cluster creation |
| linode/lke/framework_models.go | Added firewall_id parsing for framework data model |
| linode/lke/cluster.go | Implemented firewall_id reconciliation logic for create/update/match operations |
| linode/lkenodepool/tmpl/template.go | Added FirewallID to template data structure |
| linode/lkenodepool/tmpl/lke_e_nodepool.gotf | Added firewall_id to test template |
| linode/lke/framework_datasource_schema.go | Added firewall_id to datasource schema |
| linode/lke/framework_resource_test.go | Added test assertion for firewall_id |
| docs/resources/lke_node_pool.md | Added documentation and example for firewall_id |
| docs/resources/lke_cluster.md | Added documentation and example for firewall_id in pools |
| docs/data-sources/lke_cluster.md | Documented firewall_id in datasource output |
| go.mod | Updated dependencies and added local linodego replacement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
linode/lke/schema_resource.go
Outdated
| Type: schema.TypeInt, | ||
| Description: "The ID of the Firewall to attach to nodes in this node pool.", | ||
| Optional: true, | ||
| Default: 0, |
Copilot
AI
Nov 17, 2025
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.
Setting a default value of 0 for firewall_id can be misleading since 0 is being used as a sentinel value to mean 'no firewall' throughout the codebase. Consider removing the Default field to make the absence of a firewall explicit through a null value, which better aligns with the Optional semantics.
| Default: 0, |
|
|
||
| toolchain go1.24.1 | ||
|
|
||
| replace github.com/linode/linodego => ../linodego/ |
Copilot
AI
Nov 17, 2025
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.
Local module replacement should not be committed to the repository. This replace directive points to a local filesystem path that won't exist in other development environments or CI/CD pipelines. Remove this line before merging.
| replace github.com/linode/linodego => ../linodego/ |
Co-authored-by: Copilot <[email protected]>
…ider-linode into add-lnp-firewall
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
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 Description
What does this PR do and why is this change necessary?
firewall_idto NodePool.✔️ How to Test
linode_lke_clusterresourceHow do I run the relevant unit/integration tests?
📷 Preview
If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.