Adding Ports To Container Groups #1930

nathanjsweet · 2018-09-14T16:57:51Z

Adding ports field to allow for multiple port exposures
Refactor tests

(fixes #1662)

1. Adding ports field to allow for multiple port exposures 2. Refactore tests

katbyte · 2018-09-19T23:47:27Z

It looks like this is still a WIP? as tests and documentation is missing.

There is an existing branch I never finished here

One thing to note is protocol is applies to all containers so it can be pulled out into the top level, simplifying the logic.

nathanjsweet · 2018-09-20T15:51:21Z

I did change the tests. It doesn't look like ports where being tested before. Should we update the tests altogether?.

katbyte · 2018-09-20T16:12:44Z

@nathanjsweet,

We should have 1 test with the old property so we know it still works, and another one with multiple ports.

Also the documentation should to be updated.

azurerm/resource_arm_container_group.go

katbyte · 2018-09-21T17:34:45Z

azurerm/resource_arm_container_group.go

+							ForceNew: true,
+							Elem: &schema.Schema{
+								Type:         schema.TypeInt,
+								ValidateFunc: validation.IntBetween(1, 65535),


There is an existing validation function validate.PortNumber that could be used here

I can't find it.

It is in the validate package

katbyte · 2018-10-16T00:45:31Z

Hi @nathanjsweet,

Just wondering if you are intending to continue working on this? 🙂

…vider-azurerm

nathanjsweet · 2018-10-16T10:45:18Z

@katbyte I updated the code to merge port and protocol (which is what Azure does). If and when the tests pass, let me know if you'd like me to update the documentation. The next thing I would like to do is add the private IP type so that people can launch network inaccessible containers.

nathanjsweet · 2018-10-16T11:23:04Z

@katbyte Here is the error I'm getting:

--- FAIL: TestProvider (0.03s)
	provider_test.go:28: err: 1 error(s) occurred:
		
		* resource azurerm_container_group: port: ConflictsWith references unknown attribute (ports)

It seems like I'm using the ConflictsWith attribute incorrectly. Would you mind taking a look and making a suggestion?

katbyte · 2018-10-25T19:28:41Z

Hi @nathanjsweet,

Sorry for the delay, its been a busy time with v1.17 last week and hashiconf this week. I'll try and look at it next week, however could you merge in the latest master and see if you can fix the build?

The error: * resource azurerm_container_group: port: ConflictsWith references unknown attribute (ports)

I believe it may be fixed by changing the conflicts with string to container.0.ports

…vider-azurerm

nathanjsweet · 2018-10-25T19:42:15Z

I believe it may be fixed by changing the conflicts with string to container.0.ports

What if it isn't the 0th container?

katbyte · 2018-10-29T02:22:04Z

@nathanjsweet,

It will still work because no matter how many elements are in the other property, the first will always exist and trigger a conflict.

katbyte

@nathanjsweet,

I've gone through the PR and left comments inline, i'm happy to see your testing the port -> ports upgrade path! My main concern is that the tests should only be checking the state and not the response. I don't think the additional checks in the exists function are required.

The code is also failing to compile, it looks like you forgot to vendor in some packages?

azurerm/resource_arm_container_group.go

katbyte · 2018-10-29T02:31:59Z

azurerm/resource_arm_container_group.go

+						},
+
+						"ports": {
+							Type:          schema.TypeList,


We should probably use a TypeSet here as order doesn't matter? at we shouldn't allow duplicates.

azurerm/resource_arm_container_group_test.go

katbyte · 2018-10-29T02:33:22Z

azurerm/resource_arm_container_group_test.go

 )

+var emptyCheck = func(cg containerinstance.ContainerGroup) error { return nil }


Not required

azurerm/resource_arm_container_group_test.go

katbyte · 2018-10-29T02:36:33Z

azurerm/resource_arm_container_group_test.go

@@ -595,8 +766,7 @@ func testCheckAzureRMContainerGroupExists(name string) resource.TestCheckFunc {
 			}
 			return fmt.Errorf("Bad: Get on containerGroupsClient: %+v", err)
 		}
-
-		return nil
+		return checkResp(resp)


We should only be checking the resource exists here, anything more should be via the state.

azurerm/resource_arm_container_group.go

nathanjsweet · 2018-11-28T16:19:05Z

@katbyte I don't know where or how to do the documentation.

katbyte

@nathanjsweet,

All that needs to be done for the documentation is remove port and add ports from this file.

I have also re ran the tests and now the fail with:


------- Stdout: -------
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentials
=== PAUSE TestAccAzureRMContainerGroup_imageRegistryCredentials
=== CONT  TestAccAzureRMContainerGroup_imageRegistryCredentials
--- FAIL: TestAccAzureRMContainerGroup_imageRegistryCredentials (66.78s)
	testing.go:538: Step 0 error: Error applying: 1 error(s) occurred:
		
		* azurerm_container_group.test: 1 error(s) occurred:
		
		* azurerm_container_group.test: containerinstance.ContainerGroupsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UnreferencedIpAddressPorts" Message="Following ports '5443' in the 'ipAddress' are not used by any container in container group 'acctestcontainergroup-8589748340137379081'."
FAIL

katbyte

@nathanjsweet,

The tests as still failing with the following:

------- Stdout: -------
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentials
=== PAUSE TestAccAzureRMContainerGroup_imageRegistryCredentials
=== CONT  TestAccAzureRMContainerGroup_imageRegistryCredentials
--- FAIL: TestAccAzureRMContainerGroup_imageRegistryCredentials (63.39s)
	testing.go:538: Step 0 error: Error applying: 1 error(s) occurred:
		
		* azurerm_container_group.test: 1 error(s) occurred:
		
		* azurerm_container_group.test: containerinstance.ContainerGroupsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UnreferencedIpAddressPorts" Message="Following ports '5443' in the 'ipAddress' are not used by any container in container group 'acctestcontainergroup-7738923082888827735'."
FAIL

katbyte · 2018-11-29T17:03:28Z

azurerm/resource_arm_container_group.go

+										Optional:         true,
+										ForceNew:         true,
+										DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+										Default:          string("TCP"),


Suggested change

Default: string("TCP"),

Default: string(containerinstance.TCP),

katbyte · 2018-11-29T17:08:59Z

azurerm/resource_arm_container_group_test.go

@@ -81,6 +83,9 @@ func TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate(t *testing.T) {
 					resource.TestCheckResourceAttr(resourceName, "image_registry_credential.0.server", "hub.docker.com"),
 					resource.TestCheckResourceAttr(resourceName, "image_registry_credential.0.username", "updatedusername"),
 					resource.TestCheckResourceAttr(resourceName, "image_registry_credential.0.password", "updatedpassword"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.0.port", "80"),


This will fail because with a set you can no longer use .0. notation.

So what can I do?

The best way is to simply check the count via #. You can calculate the hash and access it that way however that isn't required.

katbyte · 2018-11-29T17:09:16Z

azurerm/resource_arm_container_group_test.go

@@ -143,6 +149,11 @@ func TestAccAzureRMContainerGroup_linuxBasicUpdate(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMContainerGroupExists(resourceName),
 					resource.TestCheckResourceAttr(resourceName, "container.#", "2"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.#", "2"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.0.port", "80"),


This will fail because with a set you can no longer use .0. notation.

katbyte · 2018-11-29T17:09:21Z

azurerm/resource_arm_container_group_test.go

@@ -165,6 +176,9 @@ func TestAccAzureRMContainerGroup_linuxComplete(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMContainerGroupExists(resourceName),
 					resource.TestCheckResourceAttr(resourceName, "container.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.0.port", "80"),


This will fail because with a set you can no longer use .0. notation.

katbyte · 2018-11-29T17:09:26Z

azurerm/resource_arm_container_group_test.go

@@ -216,6 +230,11 @@ func TestAccAzureRMContainerGroup_windowsBasic(t *testing.T) {
 					testCheckAzureRMContainerGroupExists(resourceName),
 					resource.TestCheckResourceAttr(resourceName, "container.#", "1"),
 					resource.TestCheckResourceAttr(resourceName, "os_type", "Windows"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.#", "2"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.0.port", "80"),


This will fail because with a set you can no longer use .0. notation.

katbyte · 2018-11-29T17:09:38Z

azurerm/resource_arm_container_group_test.go

@@ -243,6 +262,9 @@ func TestAccAzureRMContainerGroup_windowsComplete(t *testing.T) {
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMContainerGroupExists(resourceName),
 					resource.TestCheckResourceAttr(resourceName, "container.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "container.0.ports.0.port", "80"),


This will fail because with a set you can no longer use .0. notation.

katbyte · 2018-11-29T17:11:16Z

website/docs/r/container_group.html.markdown

+
+* `port` - (Required) The port number the container will expose.
+
+* `protocol` - (Optional) The network protocol ("tcp"/"udp") the port will use.


We should specify what the default is:

Suggested change

* `protocol` - (Optional) The network protocol ("tcp"/"udp") the port will use.

* `protocol` - (Optional) The network protocol associated with port. Possible values are `TCP`, `UDP` and the default is `TCP`.

katbyte · 2018-12-20T19:06:20Z

@nathanjsweet 👋

Just wondering if you are still working on this 🙂

necosta · 2019-01-08T10:37:19Z

azurerm/resource_arm_container_group.go

@@ -436,6 +469,25 @@ func expandContainerGroupContainers(d *schema.ResourceData) (*[]containerinstanc
 			containerGroupPorts = append(containerGroupPorts, containerGroupPort)
 		}

+		if v, ok := data["ports"]; ok {
+			s := v.(*schema.Set)


As I also need to deploy a container instance with multiple open ports, I cherry-picked the change in this pull request to a forked repo: https://github.com/TechhubLisbon/terraform-provider-azurerm/tree/patched_master

I only had one issue: panic: interface conversion: interface {} is *schema.Set, not []interface {}

I fixed this by replacing this line with s := v.([]interface{})

Commit: tblxio@dc95c9e

Everything else looked good!

…zurerm

katbyte · 2019-01-11T06:59:10Z

Hi @nathanjsweet,

I hope you don't mind but I have made some updates to your code to get it to pass so i can get it merged today/tomorrow.

katbyte · 2019-01-11T07:08:25Z

Tests pass:

==> Fixing source code with gofmt...
gofmt -s -w ./azurerm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMContainerGroup -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentials
=== PAUSE TestAccAzureRMContainerGroup_imageRegistryCredentials
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate
=== PAUSE TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate
=== RUN   TestAccAzureRMContainerGroup_linuxBasic
=== PAUSE TestAccAzureRMContainerGroup_linuxBasic
=== RUN   TestAccAzureRMContainerGroup_requiresImport
--- SKIP: TestAccAzureRMContainerGroup_requiresImport (0.00s)
    resource_arm_container_group_test.go:128: Skipping since resources aren't required to be imported
=== RUN   TestAccAzureRMContainerGroup_linuxBasicUpdate
=== PAUSE TestAccAzureRMContainerGroup_linuxBasicUpdate
=== RUN   TestAccAzureRMContainerGroup_linuxComplete
=== PAUSE TestAccAzureRMContainerGroup_linuxComplete
=== RUN   TestAccAzureRMContainerGroup_windowsBasic
=== PAUSE TestAccAzureRMContainerGroup_windowsBasic
=== RUN   TestAccAzureRMContainerGroup_windowsComplete
=== PAUSE TestAccAzureRMContainerGroup_windowsComplete
=== CONT  TestAccAzureRMContainerGroup_imageRegistryCredentials
=== CONT  TestAccAzureRMContainerGroup_linuxComplete
=== CONT  TestAccAzureRMContainerGroup_linuxBasic
=== CONT  TestAccAzureRMContainerGroup_windowsComplete
=== CONT  TestAccAzureRMContainerGroup_linuxBasicUpdate
=== CONT  TestAccAzureRMContainerGroup_windowsBasic
=== CONT  TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate
--- PASS: TestAccAzureRMContainerGroup_imageRegistryCredentials (82.65s)
--- PASS: TestAccAzureRMContainerGroup_windowsBasic (83.04s)
--- PASS: TestAccAzureRMContainerGroup_linuxBasic (83.58s)
--- PASS: TestAccAzureRMContainerGroup_windowsComplete (84.46s)
--- PASS: TestAccAzureRMContainerGroup_linuxBasicUpdate (104.66s)
--- PASS: TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate (107.03s)
--- PASS: TestAccAzureRMContainerGroup_linuxComplete (146.48s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	147.623s

ghost · 2019-03-05T14:07:58Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

Adding Ports To Container Groups

6e57154

1. Adding ports field to allow for multiple port exposures 2. Refactore tests

ghost added the size/M label Sep 14, 2018

tombuildsstuff added service/container-group enhancement labels Sep 19, 2018

katbyte requested changes Sep 21, 2018

View reviewed changes

nathanjsweet added 2 commits October 16, 2018 03:57

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

b182715

…vider-azurerm

merging with master, adding more robust tests, also deprecating protocol

d0886d7

ghost added size/XL and removed size/M labels Oct 16, 2018

nathanjsweet added 2 commits October 16, 2018 05:55

use port number validation, remove parts of test that are conflicting

b2c6c2e

fixing bad ports reference

dba2ecc

katbyte self-assigned this Oct 25, 2018

katbyte added this to the 1.19.0 milestone Oct 25, 2018

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

1ea9489

…vider-azurerm

conflict with the specific element

8f55e16

katbyte requested changes Oct 29, 2018

View reviewed changes

metacpp self-assigned this Oct 30, 2018

katbyte added the waiting-response label Nov 6, 2018

tombuildsstuff unassigned metacpp Nov 7, 2018

tombuildsstuff modified the milestones: 1.19.0, 1.20.0 Nov 15, 2018

ghost removed the waiting-response label Nov 28, 2018

fix set list issue

7a1debf

katbyte requested changes Nov 28, 2018

View reviewed changes

katbyte added the waiting-response label Nov 28, 2018

adding documentation, fixing tests

1b22262

ghost added size/L and removed size/M labels Nov 29, 2018

katbyte requested changes Nov 29, 2018

View reviewed changes

tombuildsstuff modified the milestones: 1.20.0, 1.21.0 Dec 6, 2018

Merge branch 'master' into master

48f0d65

ghost added the documentation label Jan 5, 2019

update default to use constant

a0727c3

necosta reviewed Jan 8, 2019

View reviewed changes

nathanjsweet and others added 4 commits January 9, 2019 15:00

ports are sets

55b840b

Merge branch 'master' of github.com:nathanjsweet/terraform-provider-a…

c584062

…zurerm

Merge branch 'master' into njs-container_portstest

93a5d1e

fixed container group ports

6997434

katbyte approved these changes Jan 11, 2019

View reviewed changes

katbyte added 2 commits January 10, 2019 23:20

make fmt goimport

04b5bf3

add some validation

f80e912

katbyte removed the waiting-response label Jan 11, 2019

katbyte merged commit 6889f3e into hashicorp:master Jan 11, 2019

katbyte added a commit that referenced this pull request Jan 11, 2019

Update CHANGELOG.md to include #1930

8d3ceee

ghost locked and limited conversation to collaborators Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Ports To Container Groups #1930

Adding Ports To Container Groups #1930

nathanjsweet commented Sep 14, 2018 •

edited by katbyte

Loading

katbyte commented Sep 19, 2018

nathanjsweet commented Sep 20, 2018

katbyte commented Sep 20, 2018

katbyte Sep 21, 2018

nathanjsweet Oct 16, 2018

katbyte Oct 16, 2018

katbyte commented Oct 16, 2018

nathanjsweet commented Oct 16, 2018

nathanjsweet commented Oct 16, 2018

katbyte commented Oct 25, 2018

nathanjsweet commented Oct 25, 2018

katbyte commented Oct 29, 2018

katbyte left a comment

katbyte Oct 29, 2018

katbyte Oct 29, 2018

katbyte Oct 29, 2018

nathanjsweet commented Nov 28, 2018

katbyte left a comment

katbyte left a comment

katbyte Nov 29, 2018

katbyte Nov 29, 2018

nathanjsweet Dec 27, 2018

katbyte Jan 6, 2019

katbyte Nov 29, 2018

katbyte Nov 29, 2018

katbyte Nov 29, 2018

katbyte Nov 29, 2018

katbyte Nov 29, 2018

katbyte commented Dec 20, 2018

necosta Jan 8, 2019

katbyte commented Jan 11, 2019

katbyte commented Jan 11, 2019

ghost commented Mar 5, 2019

		)

		var emptyCheck = func(cg containerinstance.ContainerGroup) error { return nil }

	Default: string("TCP"),
	Default: string(containerinstance.TCP),


		* `port` - (Required) The port number the container will expose.

		* `protocol` - (Optional) The network protocol ("tcp"/"udp") the port will use.

Adding Ports To Container Groups #1930

Adding Ports To Container Groups #1930

Conversation

nathanjsweet commented Sep 14, 2018 • edited by katbyte Loading

katbyte commented Sep 19, 2018

nathanjsweet commented Sep 20, 2018

katbyte commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Oct 16, 2018

nathanjsweet commented Oct 16, 2018

nathanjsweet commented Oct 16, 2018

katbyte commented Oct 25, 2018

nathanjsweet commented Oct 25, 2018

katbyte commented Oct 29, 2018

katbyte 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

nathanjsweet commented Nov 28, 2018

katbyte left a comment

Choose a reason for hiding this comment

katbyte 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

Choose a reason for hiding this comment

katbyte commented Dec 20, 2018

Choose a reason for hiding this comment

katbyte commented Jan 11, 2019

katbyte commented Jan 11, 2019

ghost commented Mar 5, 2019

nathanjsweet commented Sep 14, 2018 •

edited by katbyte

Loading