Cloudstack security group #9103

mcanevet · 2016-09-28T13:21:59Z

No description provided.

mcanevet · 2016-09-28T15:02:11Z

svanharmelen

Thanks for the PR @mcanevet! Looks pretty decent, but there are a few things that we should try to improve before we merge this one.

Next to the inline comments I think we should also make this resource be able to update the security groups. The CloudStack API allows us to do so, so why not offer that to our users as well?

But if needed I can add the update part, so don't worry about that for now. And for the other comments, please let me know if you need help with anything or if you think otherwise!

Thanks!

svanharmelen · 2016-09-28T19:21:11Z

builtin/providers/cloudstack/resource_cloudstack_security_group.go

+				ForceNew: true,
+			},
+
+			"ingress_rules": &schema.Schema{


Wouldn't it be easier to call this just rules and then have a parameter in the schema called traffic_type?

Have a look at this one as an example: https://github.com/hashicorp/terraform/blob/master/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go

This way we can have both ingress and egress rules making sure people can configure what they need.

svanharmelen · 2016-09-28T19:23:22Z

builtin/providers/cloudstack/resource_cloudstack_security_group.go

+				Optional: true,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"cidr": &schema.Schema{


cidr should be called cidr_list to stay consistent throughout the CloudStack provider.

svanharmelen · 2016-09-28T19:25:30Z

builtin/providers/cloudstack/resource_cloudstack_security_group.go

+							ForceNew: true,
+						},
+
+						"start_port": &schema.Schema{


start_port and end_port could be done a bit nicer and more inline with the other firewall related resources, like so: https://github.com/hashicorp/terraform/blob/master/builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go#L71-L76

I understand this will make the code in the resource a little bit more complex/advanced, but it offers a much better UX and it then stay consistent.

svanharmelen · 2016-09-28T19:28:09Z

builtin/providers/cloudstack/resource_cloudstack_security_group.go

+
+		param.SetSecuritygroupid(d.Id())
+
+		if cidr, ok := rule["cidr"]; ok {


You don't have to run all these tests (if ... ; ok) here as they are required parameters. So they are guaranteed to be here.

mcanevet · 2016-09-29T09:31:37Z

@svanharmelen I think I addressed all your comments.
Could you please review again?

mcanevet · 2016-10-03T08:41:27Z

@svanharmelen I still have to find out how to specify a SG as source or destination of the rule : xanzy/go-cloudstack#64

svanharmelen · 2016-10-03T17:04:01Z

I'm a bit short on time, but I'll try to have a look before the end of the week...

mcanevet · 2016-10-04T08:05:17Z

@svanharmelen I can make SetUsersecuritygrouplist(map[string]string) work. I get this error message:

Invalid user group specified, fields 'group' and 'account' cannot be null, please specify groups in the form:  userGroupList[0].group=XXX&userGroupList[0].account=YYY

I'm wondering if SetUsersecuritygrouplist() shouldn't take a [0]map[string]string as argument.

mcanevet · 2016-10-20T12:43:58Z

Requires xanzy/go-cloudstack#65

mcanevet · 2016-10-20T12:49:06Z

@svanharmelen could you please review this?

svanharmelen · 2016-10-20T14:10:11Z

@mcanevet I've been a little overloaded, but have reserved time for this tomorrow. Will update you as soon as I had a look...

mcanevet · 2016-10-24T11:25:29Z

@svanharmelen I updated this PR. It now works with latest go-cloudstack

svanharmelen · 2016-10-25T08:06:04Z

@mcanevet I'm just about to merge this PR, but there are 2 remaining points of attention.

There are no acceptance tests in the PR
There is no documentation in the PR

If you have issues writing the acceptance tests, that's no problem. In that case I will add them in a separate PR right after merging this one. But for the docs, I would very much like you to write (at least the initial) docs to describe how this resource works and what parameters it uses.

Writing the docs is really easy and not much work. If you start by making a copy of the cloudstack_firewall resource and then refactor that into the cloudstack_security_group resource, it should be a walk in the park 😉

Once the docs are added, I'll merge this PR and (if needed) add any tests and tweaks in an additional PR.

Thanks...

mcanevet · 2016-10-25T12:18:45Z

@svanharmelen Documentation added. I never wrote acceptance tests for Terraform, so could you please do it for me for now? I'll try to learn how to do it for next time.
Thanks

svanharmelen · 2016-10-26T07:29:25Z

@mcanevet again thanks for your PR and your patience 😉

I still think this PR needs some work to make to up to spec with the other CloudStack resources, but I'll add the missing stuff in a separate PR later today so that by the end of the day we have a shiny new resource which will be in the next TF release 🎉

Thanks again!

[origin/master] Cloudstack security group (#9103) d030b62

svanharmelen · 2016-10-26T08:28:14Z

@mcanevet are you available anywhere online to a have a quick chat? IRC maybe (#terraform-tool)? Or an open Slack or Gitter channel that I can join?

I'm refactoring some stuff and would like to better understand how the usersecuritygrouplist works. Not in TF, but in CS. I don't use security groups at all, so I have a hard time grasping how this is used and what it functionally does.

svanharmelen · 2016-10-26T08:39:18Z

@mcanevet I see there is a Gitter channel as well: https://gitter.im/hashicorp-terraform/Lobby

svanharmelen · 2016-10-26T20:26:29Z

@mcanevet not sure if you're getting updates from gitter, but I send you some updates about the progress...

mcanevet · 2016-10-27T05:56:50Z

@svanharmelen yes I received an email from gitter with your comments. I'll try to take some time today to test your modifications

* Add cloudstack_security_group resource * Update github.com/xanzy/go-cloudstack/cloudstack * Add support for security_group * Add documentation for cloudstack_security_group

ghost · 2020-04-21T02:01:28Z

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.

stack72 added enhancement provider/cloudstack new-resource labels Sep 28, 2016

mcanevet force-pushed the cloudstack_security_group branch from af29ade to ca5f595 Compare September 28, 2016 14:56

mcanevet force-pushed the cloudstack_security_group branch from ca5f595 to 215bdf5 Compare September 28, 2016 15:50

svanharmelen suggested changes Sep 28, 2016

View reviewed changes

mcanevet force-pushed the cloudstack_security_group branch 8 times, most recently from 082c6fa to b0d0b3a Compare September 29, 2016 09:29

mcanevet force-pushed the cloudstack_security_group branch from 019d40e to 1e6792e Compare October 20, 2016 12:47

mcanevet force-pushed the cloudstack_security_group branch from 1e6792e to 9c747f4 Compare October 20, 2016 13:33

mcanevet force-pushed the cloudstack_security_group branch from 9c747f4 to 7fe731f Compare October 24, 2016 11:20

mcanevet added 3 commits October 25, 2016 13:53

Update github.com/xanzy/go-cloudstack/cloudstack

c41248d

Add cloudstack_security_group resource

50d2bf2

Add support for security_group

70d4c1a

Add documentation for cloudstack_security_group

f80fb6d

mcanevet force-pushed the cloudstack_security_group branch from 7fe731f to f80fb6d Compare October 25, 2016 12:16

svanharmelen merged commit d030b62 into hashicorp:master Oct 26, 2016

terraformbot pushed a commit that referenced this pull request Oct 26, 2016

Terraform Acceptance Tests; Suite: powerdns

d090e85

[origin/master] Cloudstack security group (#9103) d030b62

mcanevet deleted the cloudstack_security_group branch October 26, 2016 07:38

ghost locked and limited conversation to collaborators Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloudstack security group #9103

Cloudstack security group #9103

mcanevet commented Sep 28, 2016

mcanevet commented Sep 28, 2016

svanharmelen left a comment

svanharmelen Sep 28, 2016

mcanevet Sep 29, 2016

svanharmelen Sep 28, 2016

mcanevet Sep 29, 2016

svanharmelen Sep 28, 2016

mcanevet Sep 29, 2016

svanharmelen Sep 28, 2016

mcanevet Sep 29, 2016

mcanevet commented Sep 29, 2016

mcanevet commented Oct 3, 2016 •

edited

Loading

svanharmelen commented Oct 3, 2016

mcanevet commented Oct 4, 2016 •

edited

Loading

mcanevet commented Oct 20, 2016

mcanevet commented Oct 20, 2016

svanharmelen commented Oct 20, 2016

mcanevet commented Oct 24, 2016

svanharmelen commented Oct 25, 2016

mcanevet commented Oct 25, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

mcanevet commented Oct 27, 2016

ghost commented Apr 21, 2020


		param.SetSecuritygroupid(d.Id())

		if cidr, ok := rule["cidr"]; ok {

Cloudstack security group #9103

Cloudstack security group #9103

Conversation

mcanevet commented Sep 28, 2016

mcanevet commented Sep 28, 2016

svanharmelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcanevet commented Sep 29, 2016

mcanevet commented Oct 3, 2016 • edited Loading

svanharmelen commented Oct 3, 2016

mcanevet commented Oct 4, 2016 • edited Loading

mcanevet commented Oct 20, 2016

mcanevet commented Oct 20, 2016

svanharmelen commented Oct 20, 2016

mcanevet commented Oct 24, 2016

svanharmelen commented Oct 25, 2016

mcanevet commented Oct 25, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

svanharmelen commented Oct 26, 2016

mcanevet commented Oct 27, 2016

ghost commented Apr 21, 2020

mcanevet commented Oct 3, 2016 •

edited

Loading

mcanevet commented Oct 4, 2016 •

edited

Loading