-
Notifications
You must be signed in to change notification settings - Fork 98
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
Terraform Support for Managing Reserved IP Addresses #1598
base: dev
Are you sure you want to change the base?
Conversation
…anch before next release
… tests from being run by unit test targets
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.
Thank you for your hard work on the IP reserve resource and data source. I left some suggestions to build this resource better:
…responding test. Removed unnessary logs
Merging recent changes to dev branch into ipreservation-newendpoints
…tually reserve an ip and use the same to create/assign to linode
var frameworkDataSourceFetchSchema = schema.Schema{ | ||
Attributes: map[string]schema.Attribute{ | ||
"region": schema.StringAttribute{ | ||
Description: "The Region in which to reserve the IP address.", | ||
Optional: true, | ||
}, | ||
"address": schema.StringAttribute{ | ||
Description: "The reserved IP address.", | ||
Computed: true, | ||
Optional: true, | ||
}, | ||
"gateway": schema.StringAttribute{ | ||
Description: "The default gateway for this address.", | ||
Computed: true, | ||
}, | ||
"subnet_mask": schema.StringAttribute{ | ||
Description: "The mask that separates host bits from network bits for this address.", | ||
Computed: true, | ||
}, | ||
"prefix": schema.Int64Attribute{ | ||
Description: "The number of bits set in the subnet mask.", | ||
Computed: true, | ||
}, | ||
"type": schema.StringAttribute{ | ||
Description: "The type of address this is (ipv4, ipv6, ipv6/pool, ipv6/range).", | ||
Computed: true, | ||
}, | ||
"public": schema.BoolAttribute{ | ||
Description: "Whether this is a public or private IP address.", | ||
Computed: true, | ||
}, | ||
"rdns": schema.StringAttribute{ | ||
Description: "The reverse DNS assigned to this address.", | ||
Computed: true, | ||
}, | ||
"linode_id": schema.Int64Attribute{ | ||
Description: "The ID of the Linode this address currently belongs to.", | ||
Computed: true, | ||
}, | ||
"reserved": schema.BoolAttribute{ | ||
Description: "Whether this IP is reserved or not.", | ||
Computed: true, | ||
}, | ||
"id": schema.StringAttribute{ | ||
Description: "The unique ID of the reserved IP address.", | ||
Computed: true, | ||
}, | ||
}, | ||
} |
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.
So what's the required attribute for the data source? Maybe address
?
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.
Yeah address should be a required attribute here. I will make that change now. Thanks!
type ReservedIPObject struct { | ||
ID types.String `tfsdk:"id"` | ||
Address types.String `tfsdk:"address"` | ||
Region types.String `tfsdk:"region"` | ||
Gateway types.String `tfsdk:"gateway"` | ||
SubnetMask types.String `tfsdk:"subnet_mask"` | ||
Prefix types.Int64 `tfsdk:"prefix"` | ||
Type types.String `tfsdk:"type"` | ||
Public types.Bool `tfsdk:"public"` | ||
RDNS types.String `tfsdk:"rdns"` | ||
LinodeID types.Int64 `tfsdk:"linode_id"` | ||
Reserved types.Bool `tfsdk:"reserved"` | ||
} |
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.
Is this struct used anywhere?
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.
You are right! I must have forgotten to remove this before pushing. I have removed it now.
I think a data source in TF is usually used to obtain information about a cloud resource (not necessarily a TF resource) with a given ID or attributes that are sufficient for identifying the resource on the cloud. Is that what you would like to implement here for data sources? |
return &DataSource{ | ||
BaseDataSource: helper.NewBaseDataSource( | ||
helper.BaseDataSourceConfig{ | ||
Name: "linode_reserved_ip_fetch", |
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.
Can we name this data source linode_reserved_ip
?
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.
Yeah I think that removes the unnecessary long resource names. Will make the change.
return &DataSourceList{ | ||
BaseDataSource: helper.NewBaseDataSource( | ||
helper.BaseDataSourceConfig{ | ||
Name: "linode_reserved_ip_list", |
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.
Can we name this linode_reserved_ips
?
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.
Yes we can! I think this is more appropriate. I will be making this change
} | ||
|
||
reservedIPs := make([]ReservedIPObject, len(ips)) | ||
for i, ip := range ips { |
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.
Is vpc_nat_1_1
missing?
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.
I followed the schema of the closest existing resource - linode_networking_ip
. We can still add this. Is it always null for reserved IPs? I see that when I reserve an IP address the vpc_nat_1_1
value is null. Need further clarification.
"github.com/hashicorp/terraform-plugin-framework/types" | ||
) | ||
|
||
type ReservedIPObject struct { |
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.
Missing vpc_nat_1_1
?
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.
I followed the schema of the closest existing resource - linode_networking_ip. We can still add this. Is it always null for reserved IPs? I see that when I reserve an IP address the vpc_nat_1_1 value is null. Need further clarification.
"github.com/linode/terraform-provider-linode/v2/linode/helper" | ||
) | ||
|
||
type ReservedIPModel struct { |
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.
Are we missing the ability to filter by fields? (If so, can be a follow up PR, probably)
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.
Filtering on region makes sense. I will be implementing the filter ability now.
…sources, made the datasource for listing reserved IPs filterable on region
📝 Description
This PR introduces comprehensive functionality and tests for Reserved IP Addresses in the terraform provider. The changes are necessary to provide robust support for managing Reserved IP addresses through terraform. The changes include:
Implementation of core Reserved IP operations:
Test coverage:
✔️ How to Test
What are the steps to reproduce the issue or verify the changes?
Ensure you have a valid Linode API token. Set up your environment:
export LINODE_TOKEN="your_token_here"
To run only Reserved IP related tests:
make int-test PKG_NAME="linode/networkreservedips"
To run a specific test:
Note:
Ensure you have proper permissions and sufficient quota in your Linode account to perform Reserved IP operations. Some tests may create and delete resources, so use a testing environment if possible. This test expects the account to have 0 prior reservations.