Add Probe support for Azure Container Instances

[neilpeterson]

Issue - #2916

I've taken my best stab on style. If I've missed any standards etc. happy to update.

[katbyte]

Hi @neilpeterson,

Thank you for the PR! I've left some comments inline but overall this is off to a good start 🙂 once they have been addressed this should be good to merge !

[neilpeterson]

Thanks a bunch, @katbyte I will address each and also the linting issues picked up by Travis CI.

[neilpeterson]

@katbyte I've addressed all feedback with the following notes:

• Because the two probe types share a scheme, use a helper function to populate properties (link). I am happy to do this assuming we can preserve the end user experience. Do you have a sample of where a similar helper function was used that I can draw from?
• Validate and add portal defailts. From what I can tell the Azure APIs are not enforcing default values nor a range of expected values (link). This fetaure is also not avaliable .via the Azure portal. I have added default taken from this Kuberntes document. I have contacted some MSFT folk to find out what the expectation is here.

[katbyte]

@neilpeterson,

Here is an example of a scheme function:

func SchemaDevTestVirtualMachineInboundNatRule() *schema.Schema {
	return &schema.Schema{
		Type:     schema.TypeSet,
		Optional: true,
		// since these aren't returned from the API
		ForceNew: true,
		Elem: &schema.Resource{
			Schema: map[string]*schema.Schema{
				"protocol": {
					Type:     schema.TypeString,
					Required: true,
					ValidateFunc: validation.StringInSlice([]string{
						string(dtl.TCP),
						string(dtl.UDP),
					}, false),
				},

				"backend_port": {
					Type:         schema.TypeInt,
					Required:     true,
					ForceNew:     true,
					ValidateFunc: validate.PortNumber,
				},

				"frontend_port": {
					Type:     schema.TypeInt,
					Computed: true,
				},
			},
		},
	}
}

As for the validation if there are no easy win there don't bother yourself too much trying to find some, its not a blocker to merge without 🙂

[neilpeterson]

@katbyte thanks a bunch. I've added the helper function and removed the defaults to be more in line with the API. When we sort the Azure end I will come back and add default values back in.

With that, I believe this is ready for another review. Thanks for your help.

[katbyte]

Thanks for the revisions @neilpeterson !

This is looking pretty good now, a couple minor comments left and it should be good to merge 🙂

[katbyte]

Could we change this ti false? we are moving towards case sensitivity for properties like this

[neilpeterson]

@katbyte I am struggling a bit with this one. The Azure API always returns Http or Https. When setting ignoreCase to false tests on this value begin to fail.

My assumption is that these tests should work.

    liveness_probe {
      httpget {
        path   = "/"
        port   = 443
        scheme = "Http"
      }
      inital_delay_seconds = 1
      period_seconds       = 1
      failure_threashold   = 1
      sucess_threashold    = 1
      timeout_seconds      = 1
    }

and then:

resource.TestCheckResourceAttr(resourceName,"container.0.liveness_probe.0.httpget.0.scheme", "Http"),

However, it seems I am hitting this, which returns:

testing.go:538: Step 0 error: config is invalid: azurerm_container_group.test: expected container.0.liveness_probe.0.httpget.0.scheme to be one of [http https], got Http

I am still investigating but if you have any advice, I would appreciate it :).

Thanks

[tombuildsstuff]

turns out this is a bug in the Swagger/SDK - where the constants are defined in lower-case, when the API returns them in Title Case; we can work around this for the moment by making this:

ValidateFunc: validation.StringInSlice([]string{
  // TODO: switch to using Constants once the casings fixed
  "Http",
  "Https",
}, false),

[neilpeterson]

Thanks, Tom. I figured something was going on here and had considered this approach. Appreciate the assist and validation.

[katbyte]

(misclick above)

[tombuildsstuff]

hey @neilpeterson

Thanks for pushing those changes - I've taken a look through and left some minor comments inline, but this otherwise LGTM; since most of the comments are fairly minor I'm going to push a few commits to fix these so that we can get this merged - I hope you don't mind?

Thanks!

[tombuildsstuff]

turns out this is a bug in the Swagger/SDK - where the constants are defined in lower-case, when the API returns them in Title Case; we can work around this for the moment by making this:

ValidateFunc: validation.StringInSlice([]string{
  // TODO: switch to using Constants once the casings fixed
  "Http",
  "Https",
}, false),

[tombuildsstuff]

typo here:

Suggested change

	"failure_threashold": {
	"failure_threshold": {

dismissing since changes have been pushed

[neilpeterson]

@tombuildsstuff, thanks a bunch. Changes all make sense, really appreciate the assistance in wrapping this up.

[tombuildsstuff]

Tests pass:

[ghost]

This has been released in version 1.25.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
	version = "~> 1.25.0"
}
# ... other configuration ...

[ghost]

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!