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

Container Groups (ACI) - added secure environment variables #1874

Closed
wants to merge 0 commits into from
Closed

Container Groups (ACI) - added secure environment variables #1874

wants to merge 0 commits into from

Conversation

neilpeterson
Copy link
Contributor

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..."
    }

  }
}

I have limited experience with Go, and this is my first PR to this project. Any critique / coaching would be appreciated.

@ghost ghost added the size/XXL label Sep 5, 2018
@tombuildsstuff tombuildsstuff self-assigned this Sep 6, 2018
@ghost ghost added the size/XXL label Sep 7, 2018
@metacpp metacpp added this to the Soon milestone Sep 13, 2018
@neilpeterson
Copy link
Contributor Author

@metacpp @tombuildsstuff - I see that this PR has gone conflicted. I going to hold off on fixing the conflict until some interest / traction / feedback has been given to the PR. Let me know and I will fix it up.

Thanks

@@ -14,7 +14,7 @@ import (
"github.com/Azure/azure-sdk-for-go/services/automation/mgmt/2015-10-31/automation"
"github.com/Azure/azure-sdk-for-go/services/cdn/mgmt/2017-10-12/cdn"
"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-06-01/compute"
"github.com/Azure/azure-sdk-for-go/services/containerinstance/mgmt/2018-04-01/containerinstance"
"github.com/Azure/azure-sdk-for-go/services/containerinstance/mgmt/2018-06-01/containerinstance"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the 2018-09-01 in v21.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metacpp, per our email conversation, I will keep this one at 2018-06-01 due to significant refactor in 2018-09-01. Once the secure environment variables PR has been merged, I will create a new PR to refactor the provider to use ACI 2018-09-01.

Optional: true,
ForceNew: true,
Sensitive: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this newly added schema, can you also update the documentation for ACI resource?

}

for _, v := range *secevar {
*evar = append(*evar, v)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocation for too many times, change it to:

evar = &append(*evar, *secevar...)

@@ -564,10 +571,24 @@ func expandContainerGroupContainers(d *schema.ResourceData) (*[]containerinstanc
containerGroupPorts = append(containerGroupPorts, containerGroupPort)
}

// Set both secure and non-secure environment variables
var evar *[]containerinstance.EnvironmentVariable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

envVars and secEnvVars

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@metacpp metacpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neilpeterson Please address the comments in the PR.

SecureValue: utils.String(v.(string)),
}

output = append(output, ev)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the re-allocation happening too frequently?

Copy link
Contributor Author

@neilpeterson neilpeterson Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@metacpp - I am struggling with this one, most likely a misunderstanding on my part about how this should work in Go.

Basically an environment variable and secure environment variable are the same type, with the only difference being the name of the value property (Value vs. SecureValue). That said, rather then having a single Terraform argument for environment_variables with a flag for secure, I think it works better to have two, one for environment_variables and another for secure_environment_variables. This makes for a nicer Terraform configuration.

So, for each argument, I pass the list of environment variables to the expand function.

// Expand environment variables, produces a slice of env (envVars)
if v, ok := data["environment_variables"]; ok {
  envVars = expandContainerEnvironmentVariables(v, false)
}

// Expand environment variables, produces a slice of env (secEnvVars)
if v, ok := data["secure_environment_variables"]; ok {
  secEnvVars = expandContainerEnvironmentVariables(v, true)
}

The expand function is called at max twice and returns a []containerinstance.EnvironmentVariable for each type of environment variable.

func expandContainerEnvironmentVariables(input interface{}, secure bool) *[]containerinstance.EnvironmentVariable {

	envVars := input.(map[string]interface{})
	output := make([]containerinstance.EnvironmentVariable, 0)

	// For each secure environment variable, build the instance and add to output (slice)
	if secure == true {
		for k, v := range envVars {
			ev := containerinstance.EnvironmentVariable{
				Name:        utils.String(k),
				SecureValue: utils.String(v.(string)),
			}
			output = append(output, ev)
		}

	} else {
		for k, v := range envVars {
			ev := containerinstance.EnvironmentVariable{
				Name:  utils.String(k),
				Value: utils.String(v.(string)),
			}
			output = append(output, ev)
		}
	}
	return &output
}

These are then combined and set.

// Combine envVars and secEnvVars
*envVars = append(*envVars, *secEnvVars...)

container.EnvironmentVariables = envVars

Value: utils.String(v.(string)),
}

output = append(output, ev)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't re-allocation happening too frequently?

@@ -469,6 +475,12 @@ resource "azurerm_container_group" "test" {
"foo" = "bar"
"foo1" = "bar1"
}

secure_environment_variables {
"foo" = "bar"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"secureFoo" = "secureBar"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -536,6 +548,11 @@ resource "azurerm_container_group" "test" {
"foo1" = "bar1"
}

secure_environment_variables {
"foo" = "bar"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ghost ghost added size/XL and removed size/XXL labels Oct 4, 2018
@metacpp
Copy link
Contributor

metacpp commented Oct 4, 2018

@neilpeterson I did some cleanup work on your branch and hope you don't mind.

@metacpp metacpp closed this Oct 4, 2018
@ghost ghost added size/XS and removed size/XL labels Oct 4, 2018
@tombuildsstuff tombuildsstuff modified the milestones: Soon, Being Sorted Oct 25, 2018
@ghost
Copy link

ghost commented Mar 6, 2019

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!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
@ghost ghost removed the waiting-response label Mar 6, 2019
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