Skip to content
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

Add params zone_id to cloudstack ipaddress resource #11306

Merged

Conversation

sawanoboly
Copy link
Contributor

Hi, I've add zone_id to cloudstack_ipaddress for CloudStack based provider IDCF. Their API only allows associate new IPAddress by zone_id.

Ref: http://docs.idcf.jp/cloud/api/address/#listpublicipaddresses

@svanharmelen
Copy link
Contributor

@sawanoboly thanks for the PR, but I wonder if this is really needed and if we would really want to customize the generic CloudStack provider to fix a specific private implementation.

So I am trying to get in contact with the provider (using their contact form) to discuss if they really have a fork of CloudStack running which requires this field (opposed to the Apache CloudStack API).

I'll wait for their response and get back to you when I have their feedback.

@svanharmelen
Copy link
Contributor

@sawanoboly did you (I suppose you did, but still asking to be sure) try to associate an IP address with the existing cloudstack_ipaddress resource by giving it either a network_id or a vpc_id?

If not, could you give it a try? And if so, could you share the results of those tests (and possibly the error response if any)?

The initial response from the cloud provider seems to indicate it should also work if network_id or vpc_id is supplied:

As you said, if you would like to execute associateIpAddress command with CloudStack API,
you need to specify zoneid.
We made sure that it also works when you specify either of networkid or vpcid.
If you can specify networkid or vpcid on Terraform,
please try it and see if you can execute associateIpAddress command.

I asked again (for clarity) if zone_id is still required when supplying either a network_id or a vpc_id, or if in that case the zone_id becomes optional (which I think he meant and what would mean we do not need this change).

But in the meantime (while waiting for their response), it would be nice if you could test it as well.

Thx!

@sawanoboly
Copy link
Contributor Author

@svanharmelen Thanks!

$ terraform version
Terraform v0.8.5

network_id

First of all, I attempted to acquire the IP address by network_id. Resource definition is as below.

resource "cloudstack_ipaddress" "ip-vm01" {
  network_id = "xxxx-xxxxxx-xxxxxx"
}

But, I caught error CSExceptionErrorCode: 4350 => Unable to figure out zone to assign ip to....

$ terraform plan

+ cloudstack_ipaddress.ip-vm01
    ip_address: "<computed>"
    network_id: "xxxx-xxxxxx-xxxxxx"
    project:    "<computed>"


Plan: 1 to add, 0 to change, 0 to destroy.

$ terraform apply
cloudstack_ipaddress.ip-vm01: Creating...
  ip_address: "" => "<computed>"
  network_id: "" => "xxxx-xxxxxx-xxxxxx"
  project:    "" => "<computed>"
Error applying plan:

1 error(s) occurred:

* cloudstack_ipaddress.ip-vm01: Error associating a new IP address: CloudStack API error 431 (CSExceptionErrorCode: 4350): Unable to figure out zone to assign ip to

This network_id used the same one used for other public addresses.


vpc_id

Next, I'd tried to investigate vpc_id by cs. We could not use VPC with this provider.
Parameters of other resources also do not include vpc_id.

$ cs listVPCs
Cloudstack error: HTTP response 432
{
    "listvpcsresponse": {
        "errorcode": 432,
        "errortext": "The given command does not exist or it is not available for user",
        "cserrorcode": 9999
    }
}

The vpc_id may only be masked, but it can not be examined.


zone_id

According to the first error message(Unable to figure out zone), I made changes to the code like this pull request. PublicIPaddress can be acquired by using zone_id at last.

resource "cloudstack_ipaddress" "ip-vm01" {
  zone_id = "xxxxxxxxx-xxxxxxxxxxxxx-xxxxxxxxxx-xxx"
}
$ ~/forked_and_built/bin/terraform apply
cloudstack_ipaddress.ip-vm01: Creating...
  ip_address: "" => "<computed>"
  project:    "" => "<computed>"
  zone_id:    "" => "xxxxxx-xxxxxxxxxxx-xxxxxxxxxxxx-xxxxxxx"
cloudstack_ipaddress.ip-vm01: Creation complete

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

@svanharmelen
Copy link
Contributor

Thanks @sawanoboly! I'll take this info back to the provider...

@sawanoboly sawanoboly force-pushed the allow_zoneid_to_cloudstack_ipaddress branch from f193236 to 9684c8b Compare February 7, 2017 08:42
@sawanoboly
Copy link
Contributor Author

Sorry. I mistakenly merged 0-8-stable and pushed, so I overwrote it with the difference from master.

@svanharmelen
Copy link
Contributor

No worries 😉 I'm still in conversation with the cloud provider. Will update when I got an answer. It seems they are going to fix their docs/setup so it works as standard CloudStack should work. But let's wait it out first...

Copy link
Contributor

@svanharmelen svanharmelen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's taken long enough, so let's get this in 😉 But please first fix the one comment I've added. Thx!

return fmt.Errorf(
"You must supply a value for either (so not both) the 'network_id' or 'vpc_id' parameter")
"You must supply one value for either (so not more than two) the 'network_id' or 'vpc_id' or 'zone_id' parameter")
Copy link
Contributor

@svanharmelen svanharmelen Mar 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to keep this function as it was. The zone_id is optional so still only one of network or vpc is needed right? The fact that zone_id is required by your cloud provider seems to be a somewhat wierd choice they made, which makes them divert from standard CloudStack. So I'm good with adding this feature, but we should not make it required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks,

I will check if I can create a resource even if I give both zone_id and network_id at that provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I confirmed that it allows pass both zone_id and network_id together.

cloudstack_ipaddress.ip-vm01: Creating...
  ip_address: "" => "<computed>"
  network_id: "" => "xxxxxxxxxxxxxxxxxx"
  project:    "" => "<computed>"
  zone_id:    "" => "xxxxxxxxxxxxxxxxxxx"
cloudstack_ipaddress.ip-vm01: Creation complete

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

I will restore few functions.

@sawanoboly
Copy link
Contributor Author

@svanharmelen

I've cleaned up Code. It just add zone_id as new option. Should I rebase ?

@svanharmelen
Copy link
Contributor

LGTM! Thanks!

@svanharmelen svanharmelen merged commit 5a1f809 into hashicorp:master Mar 21, 2017
@sawanoboly sawanoboly deleted the allow_zoneid_to_cloudstack_ipaddress branch March 21, 2017 12:53
mbfrahry pushed a commit that referenced this pull request Mar 28, 2017
* add option zone_id

- Ref: http://docs.idcf.jp/cloud/api/address/#listpublicipaddresses

* Exclusion of `network_id`, `vpc_id` and `zone_id`

* Revert "Exclusion of `network_id`, `vpc_id` and `zone_id`"

This reverts commit 9684c8b.

* remove zone_id from one of required option.
@ghost
Copy link

ghost commented Apr 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants