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

[CloudStack] Adding missing project ID on the get network API call, if present #6069

Closed
wants to merge 1 commit into from
Closed

[CloudStack] Adding missing project ID on the get network API call, if present #6069

wants to merge 1 commit into from

Conversation

pdube
Copy link

@pdube pdube commented Apr 7, 2016

Explicitly adding the projectId if it is present to the ListNetworks API call

@svanharmelen
Copy link
Contributor

Thanks for the PR @pdube!

Before I merge this, could you give me a TF config that doesn't work without this PR? As I believe this use case is already fixed in a more general way in the go-cloudstack package: https://github.com/xanzy/go-cloudstack/blob/master/cloudstack/NetworkService.go#L1013-L1014

So I suspect we don't actually need this additional code in here...

@hany
Copy link
Contributor

hany commented Apr 11, 2016

@svanharmelen I believe the issue here is some CS implementations require that a valid project ID always be passed in to network calls. As such, passing in -1 may not actually work in this case. This is actually similar to #6106.

@svanharmelen
Copy link
Contributor

If that is the case, could you share a TF config and CS version that have this behaviour?

Because if this is something generic, I believe we should fix it in the go-cloudstack package by adding helper functions that are project aware or by extending the existing helper functions.

But if this is the only API call having this behaviour that's not worth it of course...

@hany
Copy link
Contributor

hany commented Apr 12, 2016

Hey @svanharmelen, below is a sample TF config that is causing issues. This has been tested against CS 4.4 I believe (@pdube should be able to confirm). With this config without this PR, the API will return an empty set for the network tier ID, and therefore not get recorded in the TF state file (also the output will output nothing).

variable "api_key" {
}

variable "secret_key" {
}

variable "api_url" {
}

variable "zone" {
}

variable "project" {
  default = "some_project_id"
}

variable "vpc_name" {
  default = "test"
}

variable "vpc_cidr" {
  default = "10.48.0.0/22"
}

provider "cloudstack" {
  api_url    = "${var.api_url}"
  api_key    = "${var.api_key}"
  secret_key = "${var.secret_key}"
}

variable "vpc_offering" {
  default = "Default VPC offering"
}
variable "network_offering" {
  default = "Standard Tier"
}

resource "cloudstack_vpc" "main_vpc" {
  name         = "${var.vpc_name}"
  cidr         = "${var.vpc_cidr}"
  vpc_offering = "${var.vpc_offering}"
  zone         = "${var.zone}"
  project      = "${var.project}"
}

resource "cloudstack_network_acl" "main_acl" {
  name  = "proxy_acl"
  vpc   = "${cloudstack_vpc.main_vpc.id}"
}

resource "cloudstack_network_acl_rule" "main_acl" {
  aclid = "${cloudstack_network_acl.main_acl.id}"

  rule {
    action       = "allow"
    source_cidr  = "0.0.0.0/0"
    protocol     = "all"
    traffic_type = "egress"
  }
}

resource "cloudstack_network" "tier" {
  name             = "proxy"
  cidr             = "10.48.0.0/24"
  network_offering = "${var.network_offering}"
  vpc              = "${cloudstack_vpc.main_vpc.id}"
  zone             = "${var.zone}"
  aclid            = "${cloudstack_network_acl.main_acl.id}"
  project          = "${var.project}"
}

output "network_id" {
  value = "${cloudstack_network.tier.id}"
}

@pdube
Copy link
Author

pdube commented Apr 12, 2016

@svanharmelen We have identified 2 at this point. The network and the port forwarding rules as seen here #6106

@svanharmelen
Copy link
Contributor

@pdube @hany sorry for the delay... I worked away some other CloudStack issues, but now there's only this one left. So will pick this one up tomorrow so all CloudStack issues and PR's are gone before the next release...

@svanharmelen
Copy link
Contributor

@pdube could you share an example that shows me the problem? I tried to reproduce this using a slightly simplefied version of the above example, but failed to get the same result:

Config:

variable "api_url" {}
variable "api_key" {}
variable "secret_key" {}

provider "cloudstack" {
    api_url = "${var.api_url}"
    api_key = "${var.api_key}"
    secret_key = "${var.secret_key}"
}

resource "cloudstack_vpc" "foobar" {
    name = "terraform-vpc"
    cidr = "172.16.0.0/16"
    vpc_offering = "Default VPC offering"
    project = "terraform-test"
    zone = "BETA-SBP-DC-1"
}

resource "cloudstack_network" "foo" {
    name = "terraform-network"
    cidr = "172.16.0.0/24"
    network_offering = "NiciraNvpVpcNetworkWithLb"
    vpc_id = "${cloudstack_vpc.foobar.id}"
    project = "terraform-test"
    zone = "${cloudstack_vpc.foobar.zone}"
}

output "network_id" {
    value = "${cloudstack_network.foo.id}"
}

Result:
image

@pdube
Copy link
Author

pdube commented Apr 20, 2016

Hey @svanharmelen!
This problem occurs, when a domain-admin user's credentials is used to create resources for a sub domain's project. So to recreate it, you can create a sub domain, then create a project in that sub domain. Then create resources in that project with your initial credentials. Thanks for your response

@svanharmelen
Copy link
Contributor

@pdube @hany are you able to build the code from this PR #6282 and run your tests? I expect this to fix your problem and at the same time add a solid base for adding more project related stuff in the future.

Please let me know asap if this fixes it for you, so I can merge it before the new release is cut... Thx!

@pdube
Copy link
Author

pdube commented Apr 21, 2016

@svanharmelen Ok I'll test it out! Thanks. From a bird's eye view, it looks good.

@svanharmelen
Copy link
Contributor

@pdube did you find time to test this already? I did some additional work to make sure all other resources that support projects are now also using the updated go-cloudstack package correctly.

All acceptance tests and my local testing don't show any problems with the new code, but I don't use projects as much, and in the same way, as you guys are doing. So if you could make a fresh build of PR #6282 (amended the last changes just now) and battle test some project heavy configs with it, that would be great!

Thx!

@svanharmelen
Copy link
Contributor

@pdube feel free to reopen this one if needed, and please still do run those tests and let me know the results so we know if all your use cases are fixed now...

I just gone ahead and merged #6282 since it's pretty likely that 0.6.15 will be released today and all (acceptance) tests are passing without problems. So even if we need to tweak some additional stuff to get this specific issue solved (which I don't expect, but you never know), it's good to have the latest improvements released already...

@pdube
Copy link
Author

pdube commented Apr 22, 2016

@svanharmelen I was a bit bogged down yesterday, I'll try to build and test it today. Thanks for the work around this. It is really appreciated!

@pdube
Copy link
Author

pdube commented Apr 22, 2016

@svanharmelen I built it and have been testing it and it is working very well. I have built a few environments with most of the available CS resources (templates, networks, vpcs, instances, port forwarding rules, network ACLs, network ACL rules, IP addresses, and SSH keys) and everything is working as expected. Great work! +1

@svanharmelen
Copy link
Contributor

Cool! Thanks for testing and for the feedback! We made the provider a little better again 😉

@pdube
Copy link
Author

pdube commented Apr 22, 2016

:)

@ghost
Copy link

ghost commented Apr 26, 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 26, 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