ACI - added secure environment variables

[neilpeterson]

This PR adds Azure Container Instances secure environment variables.

Fundamentally the only difference between an environment variable and a secured variable is the value property name (Value vs. SecureValue), however for ease of use I am treating them as separate properties (seen in the below sample).

resource "azurerm_container_group" "both" {
  name                = "both"
  location            = "${azurerm_resource_group.vote-resource-group.location}"
  resource_group_name = "${azurerm_resource_group.vote-resource-group.name}"
  ip_address_type     = "public"
  os_type             = "linux"

  container {
    name   = "both"
    image  = "neilpeterson/nepetersv1"
    cpu    = "0.5"
    memory = "1.5"
    port   = "80"

    environment_variables {
      "NON_SECURE"  = "This is exposed in the portal and logs etc..."
    }

    secure_environment_variables {
      "SECURE"  = "This is not exposed in the portal and logs etc..."
    }

  }
}

[neilpeterson]

This PR replaces #1874

I am still working through the feedback and will give indication when this is ready for review.

[neilpeterson]

Outstanding Issues from corrupt PR:

#1874 (comment)

#1874 (comment)

[metacpp]

@metacpp in the old PR, you ask to rev the ACI SDK version. It seems this version introduces some changes which will require a refactor of the Terraform provider. Do you want the refactor + my additional functionality added in one PR, or would it be ok for me to add the functionality first, and then follow up with a PR to refactor the provider to the new SDK version?

#1874 (comment)

@neilpeterson please go ahead with current SDK version.

[metacpp]

@metacpp can you point me towards the docs so that I can update. I am unable to locate them.

@neilpeterson Please add description for secure_environment_variables at:
https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/website/docs/r/container_group.html.markdown

[metacpp]

Please add the missed documentation change in the PR.

[metacpp]

Do we need to update go-autorest/logger here?

[metacpp]

Do we need to change it?

[neilpeterson]

I was unable to successfully run make vendor-status without this change.

[neilpeterson]

@metacpp thanks a bunch. I had checked the build and everything looked good. My apologies for these issues.

[metacpp]

@neilpeterson @jeffreyCline Please resolve the comment in PR.

[metacpp]

We may need to align with Azure documentation:
https://docs.microsoft.com/en-us/azure/container-instances/container-instances-environment-variables#secure-values

[WodansSon]

I disagree, as this implies a higher level of security that isn't there since the values are persisted in the HCL file plan test. Since it is not encrypted I would feel better if we would expose it as sensitive and in the code pass it as secureValue as required by the API.

[paultyng]

I think there was a misunderstanding that this wasn't surfacing something the API here internally. While these variable are not "secure" in Terraform, they are more secure in the container group and that is the name used in the API, so we should probably match it.

[WodansSon]

Agreed, I have reverted back to using the secure name instead of sensitive.

[WodansSon]

[metacpp]

LGTM but please do more testing.

[WodansSon]

Agreed, more testing is needed and more test cases need to be added, but this is a functional resource for the happy path scenario.

[WodansSon]

@katbyte Appreciate the updates to the lookup, code LGTM

[katbyte]

tests pass:

[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 👉 hashibot-feedback@hashicorp.com. Thanks!