Copilot commented on code in PR #305:
URL:
https://github.com/apache/cloudstack-terraform-provider/pull/305#discussion_r3642585283
##########
website/docs/r/port_forward.html.markdown:
##########
@@ -70,3 +70,22 @@ The following attributes are exported:
* `id` - The ID of the IP address for which the port forwards are created.
* `vm_guest_ip` - The IP address of the virtual machine that is used
for the port forwarding rule.
+
+## Import
+
+Port forwards can be imported; use the `<IPADDRESSID>` for which the rules
+are configured as the import ID. For example:
+
+```shell
+$ terraform import cloudstack_port_forward.default
6226ea4d-9cbe-4cc9-b30c-b9532146da5b
+```
+
+When importing into a project you need to prefix the import ID with the
project name:
+
+```shell
+$ terraform import cloudstack_port_forward.default
my-project/6226ea4d-9cbe-4cc9-b30c-b9532146da5b
+```
+
+*NOTE: All existing port forwarding rules for the IP address are imported into
+a single `forward` set, and `managed` is set to `true` so unrelated rules
+created outside of this resource aren't dropped on the next apply.*
Review Comment:
The new Import note says setting `managed` to `true` prevents unrelated
rules from being dropped, but `managed` is documented earlier as deleting any
port forwards not in config. This note should clarify that users must ensure
their config matches the imported `forward` set (or keep `managed` disabled) to
avoid accidental deletion.
##########
cloudstack/resource_cloudstack_port_forward.go:
##########
@@ -230,6 +233,89 @@ func createPortForward(d *schema.ResourceData, meta
interface{}, forward map[str
return nil
}
+func resourceCloudStackPortForwardImport(d *schema.ResourceData, meta
interface{}) ([]*schema.ResourceData, error) {
+ cs := meta.(*cloudstack.CloudStackClient)
+
+ // Try to split the ID to extract the optional project name.
+ s := strings.SplitN(d.Id(), "/", 2)
+ if len(s) == 2 {
+ d.Set("project", s[0])
+ }
+
+ ipAddressID := s[len(s)-1]
+ d.SetId(ipAddressID)
+
+ // Make sure the IP address exists
+ if _, count, err := cs.Address.GetPublicIpAddressByID(
+ ipAddressID,
+ cloudstack.WithProject(d.Get("project").(string)),
+ ); err != nil {
+ if count == 0 {
+ return nil, fmt.Errorf("IP address with ID %s does not
exist", ipAddressID)
+ }
+ return nil, err
+ }
+
+ // Get all the forwards configured for this IP address
+ p := cs.Firewall.NewListPortForwardingRulesParams()
+ p.SetIpaddressid(ipAddressID)
+ p.SetListall(true)
+
+ if err := setProjectid(p, cs, d); err != nil {
+ return nil, err
+ }
+
+ l, err := cs.Firewall.ListPortForwardingRules(p)
+ if err != nil {
+ return nil, err
+ }
+
+ forwards :=
resourceCloudStackPortForward().Schema["forward"].ZeroValue().(*schema.Set)
+ for _, f := range l.PortForwardingRules {
+ privPort, err := strconv.Atoi(f.Privateport)
+ if err != nil {
+ return nil, err
+ }
+
+ pubPort, err := strconv.Atoi(f.Publicport)
+ if err != nil {
+ return nil, err
+ }
+
+ forward := map[string]interface{}{
+ "protocol": f.Protocol,
+ "private_port": privPort,
+ "public_port": pubPort,
+ "virtual_machine_id": f.Virtualmachineid,
+ "vm_guest_ip": f.Vmguestip,
+ "uuid": f.Id,
+ }
+
+ if f.Privateendport != "" && f.Privateendport != f.Privateport {
+ privEndPort, err := strconv.Atoi(f.Privateendport)
+ if err != nil {
+ return nil, err
+ }
+ forward["private_end_port"] = privEndPort
+ }
+
+ if f.Publicendport != "" && f.Publicendport != f.Publicport {
+ pubEndPort, err := strconv.Atoi(f.Publicendport)
+ if err != nil {
+ return nil, err
+ }
+ forward["public_end_port"] = pubEndPort
+ }
+
+ forwards.Add(forward)
+ }
+
+ d.Set("forward", forwards)
+ d.Set("managed", true)
+
+ return []*schema.ResourceData{d}, nil
Review Comment:
The importer unconditionally sets `managed = true`. Given `managed` is
documented as deleting any port forwards not present in config, importing into
a partial/stub config can lead to unintended rule deletions on the next apply.
Consider leaving `managed` at its default (false) or documenting very
explicitly that import requires users to adopt the full imported `forward` set
before applying.
##########
cloudstack/resource_cloudstack_port_forward_test.go:
##########
@@ -57,6 +57,28 @@ func TestAccCloudStackPortForward_basic(t *testing.T) {
})
}
+func TestAccCloudStackPortForward_import(t *testing.T) {
+ resource.Test(t, resource.TestCase{
+ PreCheck: func() { testAccPreCheck(t) },
+ Providers: testAccProviders,
+ CheckDestroy: testAccCheckCloudStackPortForwardDestroy,
+ Steps: []resource.TestStep{
+ {
+ Config: testAccCloudStackPortForward_basic,
+ },
+
+ {
+ ResourceName:
"cloudstack_port_forward.foo",
+ ImportState: true,
+ ImportStateVerify: true,
+ // The importer sets managed=true so unrelated
forwards on the same IP
+ // aren't silently dropped; the applied config
leaves it at its default.
+ ImportStateVerifyIgnore: []string{"managed"},
Review Comment:
This comment is misleading: `managed=true` means the resource will delete
port forwards not in config (per the resource docs). The ignore is fine for the
test, but the comment should explain that the importer sets `managed=true` and
the test config leaves it at default, hence the ignore.
##########
cloudstack/resource_cloudstack_service_offering.go:
##########
@@ -34,6 +34,9 @@ func resourceCloudStackServiceOffering() *schema.Resource {
Read: resourceCloudStackServiceOfferingRead,
Update: resourceCloudStackServiceOfferingUpdate,
Delete: resourceCloudStackServiceOfferingDelete,
+ Importer: &schema.ResourceImporter{
+ State: resourceCloudStackServiceOfferingImport,
+ },
Review Comment:
Import support was added for service offerings, but this PR doesn't include
a corresponding website docs update describing the import ID format (service
offering ID) and that the importer resolves the required `name` from the ID.
Without docs, users won't discover or use this feature correctly.
##########
cloudstack/resource_cloudstack_host.go:
##########
@@ -37,6 +37,9 @@ func resourceCloudStackHost() *schema.Resource {
Update: resourceCloudStackHostUpdate,
Create: resourceCloudStackHostCreate,
Delete: resourceCloudStackHostDelete,
+ Importer: &schema.ResourceImporter{
+ State: schema.ImportStatePassthrough,
+ },
Review Comment:
Host import is enabled via passthrough, but several required arguments
(e.g., `url`, `username`, `password`, and provider-local timeouts/flags) cannot
be read back from CloudStack and thus won't be populated on import/Read. Users
are likely to see perpetual diffs or forced replacement unless they use
`lifecycle.ignore_changes` for these fields. Consider documenting these import
caveats (and/or switching to a custom importer/schema approach) so imports are
not misleading.
##########
cloudstack/resource_cloudstack_network_offering.go:
##########
@@ -33,6 +33,9 @@ func resourceCloudStackNetworkOffering() *schema.Resource {
Read: resourceCloudStackNetworkOfferingRead,
Update: resourceCloudStackNetworkOfferingUpdate,
Delete: resourceCloudStackNetworkOfferingDelete,
+ Importer: &schema.ResourceImporter{
+ State: resourceCloudStackNetworkOfferingImport,
+ },
Review Comment:
Import support was added for network offerings, but this PR doesn't include
a corresponding website docs update describing the import ID format (network
offering ID) and that the importer resolves the required `name` from the ID.
This should be documented alongside the resource.
##########
website/docs/r/autoscale_vm_profile.html.markdown:
##########
@@ -74,3 +74,11 @@ The following arguments are supported:
The following attributes are exported:
* `id` - The autoscale VM profile ID.
+
+## Import
+
+Autoscale VM profiles can be imported using the `id`, e.g.
+
+```shell
+$ terraform import cloudstack_autoscale_vm_profile.default
6eb22f91-7454-4107-89f4-36afcdf33021
+```
Review Comment:
This doc advertises import support, but the corresponding acceptance test is
currently skipped due to a cloudstack-go issue. Adding a brief note here would
set expectations and reduce surprise if import behaves unexpectedly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
