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

Azure keyvault secret datasource #89

Closed
wants to merge 1 commit into from
Closed

Azure keyvault secret datasource #89

wants to merge 1 commit into from

Conversation

NicolasFloquet
Copy link

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

@NicolasFloquet NicolasFloquet requested a review from a team as a code owner May 18, 2021 15:36
@hashicorp-cla
Copy link

hashicorp-cla commented May 18, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@nywilken nywilken May 21, 2021

Choose a reason for hiding this comment

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

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)
}

Copy link
Author

Choose a reason for hiding this comment

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

Done

@NicolasFloquet
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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 NicolasFloquet changed the title [WIP] Azure keyvault secret datasource Azure keyvault secret datasource May 20, 2021
@NicolasFloquet
Copy link
Author

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 :)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
sidebar_title: Overview
nav_title: Overview

Copy link
Author

Choose a reason for hiding this comment

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

Done

type=string
}
variable "client_id" {
type=string
Copy link
Contributor

Choose a reason for hiding this comment

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

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

type=string
}
variable "client_secret" {
type=string
Copy link
Contributor

Choose a reason for hiding this comment

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

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

type=string
}
variable "tenant_id" {
type=string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
type=string
type=string
default = "${env("AZURE_TENANT_ID")}"

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +18 to +20
client_id = "${var.client_id}"
client_secret = "${var.client_secret}"
tenant_id = "${var.tenant_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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` }}"
    },

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 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")}"
}```

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

@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.


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"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nywilken nywilken May 22, 2021

Choose a reason for hiding this comment

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

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.

ContentType string
Tags map[string]*string

client keyvault.BaseClient
Copy link
Contributor

Choose a reason for hiding this comment

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

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

type=string
}
variable "tenant_id" {
type=string
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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
Copy link
Contributor

@nywilken nywilken May 21, 2021

Choose a reason for hiding this comment

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

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)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Azure Keyvault Datasource
4 participants