Azure keyvault secret datasource #89

NicolasFloquet · 2021-05-18T15:36:49Z

Hello,

So this is a first implementation of a datasource for keyvault secrets.
Configuration requires a keyvault name (I used "keyvault_id" to stay consistent with terraform's azure-arm), and a secret name. Datasource use the same authentication parameters than the builder:
Example:

data "azure-keyvault-secret" "secret" {
  name            = "my-secret"
  keyvault_id     = "my-keyvault"
  client_id       = "${var.client_id}"
  subscription_id = "${var.subscription_id}"
  client_secret   = "${var.client_secret}"
}

Output is also similar to what you get with terraform:

id (string) - The Key Vault Secret ID.
value (string) - The value of the Key Vault Secret.
content_type (string) - The content type for the Key Vault Secret.
tags (map[string]*string) - Any tags assigned to this resource.

Closes #83

hashicorp-cla · 2021-05-18T15:36:52Z

All committers have signed the CLA.

sylviamoss

This is looking nice! I miss the docs though. I see you generated the partials but they are not being used anywhere.

sylviamoss · 2021-05-18T15:45:18Z

datasource/keyvaultsecret/data.go

+	return (&DatasourceOutput{}).FlatMapstructure().HCL2Spec()
+}
+
+// We need to stub packersdk ui "say" function. UI is not available from this package, I guess this is the best we can do for now


this sounds like something we need to think about 🤔 nice workaround though!

I agree with @sylviamoss that we need to think about the use of UI within datasources. But IMHO I don't think it is needed for this datasource.

The use of the say function is solely for displaying the debug like messages from the GetServicePrincipalTokens function which can add some additional noise to the output since datasources execute before an actual build.

For this case I would suggestion going with the logSay function but with a few small tweaks. Which will only show up when running a Packer build with the debug logs enabled PACKER_LOG=1 packer build template.pkr.hcl.

func logSay(s string) { log.Println("[DEBUG] packer-datasource-azure-keyvaultsecret:", s) }

NicolasFloquet · 2021-05-18T15:47:51Z

Hello,
This is my first contribution, so I have a few questions & comments.

First of all, to avoid unnecessary code duplication for authentication, I reused some code from the builder (namely, GetServicePrincipalTokens). Yet, this methods relies on packer sdk UI "say" function, that is not available from the execute handler of the datasource. I stubbed the function with a log.Println, but I feel this is not ideal. Is there any way or plan to access UI functions from datasources?

You will also notice that I have not implemented any acceptance test for the datasource yet. I'm actually pretty confused about how it should work. I tried to look at the amazon plugin, and the acceptance seemingly requires credentials to pass. Can I get some guidance on how to implement this test?

Thank you!

NicolasFloquet · 2021-05-19T09:43:20Z

This is looking nice! I miss the docs though. I see you generated the partials but they are not being used anywhere.

Just added added a documentation page shamelessly inspired by the amazon one :)

sylviamoss · 2021-05-19T11:58:04Z

Hey @NicolasFloquet, thanks for working on this. Again, this is looking nice!

You're right, the UI is not available to the Datasource and it should be. This will require some thinking to find the best approach to do it. The first thing it comes to my mind is that we are going to change the Datasource interface to let Execute() get the UI as an argument. There are some ways to do it without breaking the existing interface. I will create an issue to track this and speak to the rest of the team about it. For now, I think your workaround should be good.

For the acceptance test, you can pass credentials via environment variables and document that these are needed in order to run the test. The amazon ones assume the credentials are set as env variables. Does that help you?

Feel free to keep making any question you need 😃

NicolasFloquet · 2021-05-20T16:26:40Z

Hello @sylviamoss,
I've added the documentation and the acceptance test.
For simplicity I chose to rely on env variables for the test. Following variables are required:
PKR_VAR_keyvault_id
PKR_VAR_client_id
PKR_VAR_client_secret
PKR_VAR_tenant_id

I think my pull request is ready for review now :)

sylviamoss · 2021-05-21T14:04:21Z

docs/datasources/index.mdx

+  The Azure plugin is able to fetch data from Azure. To achieve this, the plugin comes with
+  data sources to retrieve keyvault secrets.
+page_title: Azure - Data Sources
+sidebar_title: Overview


Sidebar_title works but is old, we are using nav_title now.

Suggested change

sidebar_title: Overview

nav_title: Overview

sylviamoss · 2021-05-21T14:10:58Z

datasource/keyvaultsecret/test-fixtures/template.pkr.hcl

+    type=string
+}
+variable "client_id" {
+    type=string


I think these can default to the environment, is easier to have them set already than setting the PKR_VAR_client_id. What do you think?

Suggested change

type=string

type=string

default = "${env("AZURE_CLIENT_ID")}"

sylviamoss · 2021-05-21T14:11:19Z

datasource/keyvaultsecret/test-fixtures/template.pkr.hcl

+    type=string
+}
+variable "client_secret" {
+    type=string


Suggested change

type=string

type=string

default = "${env("AZURE_CLIENT_SECRET")}"

sylviamoss · 2021-05-21T14:11:44Z

datasource/keyvaultsecret/test-fixtures/template.pkr.hcl

+    type=string
+}
+variable "tenant_id" {
+    type=string


Suggested change

type=string

type=string

default = "${env("AZURE_TENANT_ID")}"

Is TENANT_ID still needed?

It has been removed from the other acceptance test as the ARM_SUBSCRIPTION_ID appears to be enough. But I'm not sure from the context of this change.

Actually at first I wanted to stay with subscription Id, but the acceptance test uses azure-sdk-for-go for secret creation/deletion, and as far as I know the SDK only works with tenant Id (https://docs.microsoft.com/en-us/azure/developer/go/azure-sdk-authorization#use-environment-based-authentication).
For simplicity purpose I also used tenant id in the template.

Ah okay I see. That is why os.Setenv("AZURE_AD_RESOURCE", "https://vault.azure.net") is also needed. Thanks for the response here TIL.

sylviamoss · 2021-05-21T14:13:40Z

datasource/keyvaultsecret/test-fixtures/template.pkr.hcl

+  client_id       = "${var.client_id}"
+  client_secret   = "${var.client_secret}"
+  tenant_id       = "${var.tenant_id}"


You can also just add a comment to this file telling that the AZURE_* envs are necessary to be set. If so you can remove the variables and these three lines, as well as their reference in getAuthorizer()

I'm assuming azure consumes the env variables by default like AWS does. I think it does because I run my azure builds like this 🤔

I tried Azure builder with only AZURE_* environment variable and it didn't work, it actually falls back to managed identity authentication, which obviously cannot work on my computer.
I'm genuinely curious to know how it works for you!
Having the same behavior than AWS plugin would be great indeed.

The builder is setup to use managed identity if the user does not explicitly set subscription_id, client_id, and client_secret. Which is not configured automatically when using the AZURE_* environment variables. We run our builds and tests with the following variables:

"variables": { "client_id": "{{ env `ARM_CLIENT_ID` }}", "client_secret": "{{ env `ARM_CLIENT_SECRET` }}", "object_id": "{{ env `ARM_OBJECT_ID` }}", "subscription_id": "{{ env `ARM_SUBSCRIPTION_ID` }}", "tenant_id": "{{ env `ARM_TENANT_ID` }}", "resource_group_name": "{{ env `ARM_RESOURCE_GROUP_NAME` }}", "storage_account": "{{ env `ARM_STORAGE_ACCOUNT` }}" },

Okay, this is way clearer! I'll push a change match this setup then :)

👍 For the record this is an example in JSON. An HCL2 equivalent would be something like

variable "client_id" { type = string default = "${env("ARM_CLIENT_ID")}" } variable "client_secret" { type = string default = "${env("ARM_CLIENT_SECRET")}" } variable "object_id" { type = string default = "${env("ARM_OBJECT_ID")}" } variable "resource_group_name" { type = string default = "${env("ARM_RESOURCE_GROUP_NAME")}" } variable "storage_account" { type = string default = "${env("ARM_STORAGE_ACCOUNT")}" } variable "subscription_id" { type = string default = "${env("ARM_SUBSCRIPTION_ID")}" } variable "tenant_id" { type = string default = "${env("ARM_TENANT_ID")}" }```

nywilken

@NicolasFloquet I agree with @sylviamoss this looks is looking good. I left a few additional suggestions and my thoughts on the logSay function workaround. I would like to pull this down and test as well. But at a glance it looks good.

nywilken · 2021-05-21T14:10:06Z

datasource/keyvaultsecret/data_acc_test.go

+
+func getAuthorizer() (autorest.Authorizer, error) {
+	os.Setenv("AZURE_AD_RESOURCE", "https://vault.azure.net")
+	os.Setenv("AZURE_TENANT_ID", os.Getenv("PKR_VAR_tenant_id"))


To simplify the acceptance testing setup the expected environment variables should be the same as expected by the current builder acceptance tests. This way you don't have to set anything, maybe other than the AZURE_AD_RESOURCE which looks like it can be a const for the test cases.

packer-plugin-azure/builder/azure/arm/builder_acc_test.go

Line 6 in 5980611

// * ARM_CLIENT_ID

After your last comment about the AZURE_* variables I understand why you have this setup. Instead of PKR_VAR_* I recommend using the environment variables already required for the the acceptance tests. For example

os.Setenv("AZURE_TENANT_ID", os.Getenv("ARM_TENANT_ID")) os.Setenv("AZURE_CLIENT_ID", os.Getenv("ARM_CLIENT_ID")) ...

I also recommend that a comment be added to the top of this file indicating which env variables are needed for the tests.

nywilken · 2021-05-21T14:12:33Z

datasource/keyvaultsecret/data_acc_test.go

+	ContentType string
+	Tags        map[string]*string
+
+	client keyvault.BaseClient


nit: Run go fmt against the files to format the source.

nywilken · 2021-05-21T14:16:51Z

datasource/keyvaultsecret/test-fixtures/template.pkr.hcl

+    type=string
+}
+variable "tenant_id" {
+    type=string


Is TENANT_ID still needed?

It has been removed from the other acceptance test as the ARM_SUBSCRIPTION_ID appears to be enough. But I'm not sure from the context of this change.

nywilken · 2021-05-21T14:27:21Z

datasource/keyvaultsecret/data.go

+	return (&DatasourceOutput{}).FlatMapstructure().HCL2Spec()
+}
+
+// We need to stub packersdk ui "say" function. UI is not available from this package, I guess this is the best we can do for now


I agree with @sylviamoss that we need to think about the use of UI within datasources. But IMHO I don't think it is needed for this datasource.

The use of the say function is solely for displaying the debug like messages from the GetServicePrincipalTokens function which can add some additional noise to the output since datasources execute before an actual build.

For this case I would suggestion going with the logSay function but with a few small tweaks. Which will only show up when running a Packer build with the debug logs enabled PACKER_LOG=1 packer build template.pkr.hcl.

func logSay(s string) { log.Println("[DEBUG] packer-datasource-azure-keyvaultsecret:", s) }

NicolasFloquet requested a review from a team as a code owner May 18, 2021 15:36

sylviamoss reviewed May 18, 2021

View reviewed changes

sylviamoss mentioned this pull request May 19, 2021

Make UI available in Datasources hashicorp/packer#11034

Open

NicolasFloquet changed the title ~~[WIP] Azure keyvault secret datasource~~ Azure keyvault secret datasource May 20, 2021

sylviamoss reviewed May 21, 2021

View reviewed changes

nywilken reviewed May 21, 2021

View reviewed changes

First implementation for azure keyvault datasource

ea11cc2

NicolasFloquet closed this Jul 27, 2021

fsforza mentioned this pull request Sep 25, 2023

Azure Keyvault Datasource #83

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure keyvault secret datasource #89

Azure keyvault secret datasource #89

NicolasFloquet commented May 18, 2021

hashicorp-cla commented May 18, 2021 •

edited

sylviamoss left a comment

sylviamoss May 18, 2021

nywilken May 21, 2021 •

edited

NicolasFloquet May 22, 2021

NicolasFloquet commented May 18, 2021

NicolasFloquet commented May 19, 2021

sylviamoss commented May 19, 2021

NicolasFloquet commented May 20, 2021

sylviamoss May 21, 2021

NicolasFloquet May 22, 2021

sylviamoss May 21, 2021

sylviamoss May 21, 2021

sylviamoss May 21, 2021

nywilken May 21, 2021

NicolasFloquet May 22, 2021

nywilken May 22, 2021

sylviamoss May 21, 2021

sylviamoss May 21, 2021

NicolasFloquet May 22, 2021

nywilken May 22, 2021

NicolasFloquet May 22, 2021

nywilken May 22, 2021

nywilken left a comment

nywilken May 21, 2021

nywilken May 22, 2021 •

edited

nywilken May 21, 2021

nywilken May 21, 2021

nywilken May 21, 2021 •

edited

	type=string
	type=string
	default = "${env("AZURE_CLIENT_SECRET")}"

Azure keyvault secret datasource #89

Azure keyvault secret datasource #89

Conversation

NicolasFloquet commented May 18, 2021

hashicorp-cla commented May 18, 2021 • edited

sylviamoss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken May 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasFloquet commented May 18, 2021

NicolasFloquet commented May 19, 2021

sylviamoss commented May 19, 2021

NicolasFloquet commented May 20, 2021

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

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

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken May 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken May 21, 2021 • edited

Choose a reason for hiding this comment

hashicorp-cla commented May 18, 2021 •

edited

nywilken May 21, 2021 •

edited

nywilken May 22, 2021 •

edited

nywilken May 21, 2021 •

edited