Add Nomad secret backend #923

jasonodonnell · 2020-11-30T20:29:19Z

This PR adds support for the Nomad secret engine. The following endpoints are supported.

"/nomad/config/access" as a resource
"/nomad/config/lease" as a resource
"/nomad/role/<role name>" as a resource
"/nomad/creds/<role name>" as a data source

Currently automated acceptance testing isn't integrated with CircleCI because to run Nomad in a container, it needs to run as privileged. When we update the CI for other integrated testing (such as AD) this should be added and is being tracked internally.

To manually test this, you can use the following script: https://gist.github.com/jasonodonnell/1a2cfaaa376592c170ad5fa2eab7e423. This script assumes you have vault and nomad installed.

To run the acceptance tests, here's a setup script you can run from the project root: https://gist.github.com/jasonodonnell/e40a9b63eb8e9cd3556d221fd2e16494. This script assumes you have vault and nomad installed.

If you want to test this via terraform binary, here's an example resource:

variable "TOKEN" {}

provider "vault" {
  address = "http://0.0.0.0:8200"
  token = "root"
}

resource "vault_nomad_secret_backend" "config" {
    backend = "nomad"
    address = "http://0.0.0.0:4646"
    token = var.TOKEN
    max_ttl = 60
    ttl = 30
}

resource "vault_nomad_secret_role" "test" {
    backend = vault_nomad_secret_backend.config.backend
    role = "test"
    type = "client"
    policies = ["readonly"]
}

data "vault_nomad_access_token" "token" {
  backend = vault_nomad_secret_backend.config.backend
  role    = vault_nomad_secret_role.test.role
  depends_on = [vault_nomad_secret_role.test]
}

output "secret" {
  value = data.vault_nomad_access_token.token.secret_id
}

output "accessor" {
  value = data.vault_nomad_access_token.token.accessor_id
}

Related test output

[~/Git/terraform-provider-vault] make testacc TESTARGS='-run TestAccDataSourceNomadAccess*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccDataSourceNomadAccess* -timeout 120m
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/codegen	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/generated	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/util	(cached) [no tests to run]
=== RUN   TestAccDataSourceNomadAccessCredentialsClientBasic
--- PASS: TestAccDataSourceNomadAccessCredentialsClientBasic (0.73s)
=== RUN   TestAccDataSourceNomadAccessCredentialsManagementBasic
--- PASS: TestAccDataSourceNomadAccessCredentialsManagementBasic (0.71s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	1.940s

[~/Git/terraform-provider-vault] make testacc TESTARGS='-run TestAccNomadSecret*'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccNomadSecret* -timeout 120m
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/codegen	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/generated	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/decode	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/datasources/transform/encode	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/alphabet	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/role	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/template	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/generated/resources/transform/transformation	(cached) [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-vault/util	(cached) [no tests to run]
=== RUN   TestAccNomadSecretLease
--- PASS: TestAccNomadSecretLease (0.21s)
=== RUN   TestAccNomadSecretBackendRoleClientBasic
--- PASS: TestAccNomadSecretBackendRoleClientBasic (0.14s)
=== RUN   TestAccNomadSecretBackendRoleManagementBasic
--- PASS: TestAccNomadSecretBackendRoleManagementBasic (0.14s)
=== RUN   TestAccNomadSecretBackendRoleImport
--- PASS: TestAccNomadSecretBackendRoleImport (0.16s)
PASS
ok  	github.com/hashicorp/terraform-provider-vault/vault	1.118s

Closes #831.

calvn · 2020-11-30T23:34:55Z

vault/provider.go

@@ -466,6 +470,18 @@ var (
 			Resource:      ldapAuthBackendGroupResource(),
 			PathInventory: []string{"/auth/ldap/groups/{name}"},
 		},
+		"vault_nomad_secret_backend": {
+			Resource:      nomadSecretAccessBackendResource(),
+			PathInventory: []string{"/nomad"},


Should /nomad/config/access be part of PathInventory here?

@calvn Good question! Throughout the provider the mount and the config are the same resource. We derive the full path nomad/config/access from the backend. Since we derive it, /nomad is the default.

nomad/config/lease is slightly different so I didn't add it to this resource. We could but it felt different enough to have a separate resource.

@calvn Having thought about combining nomad/config/lease with the other configs, I decided to keep them as two separate resources. This is simply to reduce the amount of operations create/update/read need to do for all three paths (/nomad, /nomad/config/access and /nomad/config/lease).

Is PathInventory used for routing? Or is is just for documentation purposes?

To Calvin's question this function does seem to cover the /nomad/config/access endpoint?

I can try in the morning having both but pointing to the same resource handler, maybe that won't cause issues.

Update: both can't be present because the resource name is the key and you can't duplicate it.

I changed the PathInventory to be /nomad/config/access. This did not have any regressions on the resources and is probably a better self-documenting change.

PathInventory is a list, so it could also be this:

"vault_nomad_secret_backend": { Resource: nomadSecretAccessBackendResource(), PathInventory: []string{ "/nomad", "/nomad/config/access", },

But I think setting it to just "/nomad/config/access" makes sense 👍

Oh derp, yeah, that did work! Good call!

/nomad, /nomad/config/access and /nomad/config/lease are merged into a single resource vault_nomad_secret_backend.

tvoran

The manual and acceptance tests worked for me locally, not seeing any blockers on the first pass here.

vault/resource_nomad_secret_backend.go

vault/resource_nomad_secret_role.go

tvoran · 2020-12-02T01:13:25Z

vault/provider.go

@@ -466,6 +470,18 @@ var (
 			Resource:      ldapAuthBackendGroupResource(),
 			PathInventory: []string{"/auth/ldap/groups/{name}"},
 		},
+		"vault_nomad_secret_backend": {
+			Resource:      nomadSecretAccessBackendResource(),
+			PathInventory: []string{"/nomad"},


Is PathInventory used for routing? Or is is just for documentation purposes?

To Calvin's question this function does seem to cover the /nomad/config/access endpoint?

Co-authored-by: Theron Voran <[email protected]>

vault/provider.go

catsby · 2020-12-02T23:28:05Z

vault/data_source_nomad_credentials.go

+		return fmt.Errorf("secret_id is not set in response")
+	}
+
+	d.SetId(accessorID)


Why not use path here? Granted, we aren't using the ID at all that I can see

Each call to this endpoint generates a new token, so we want the ID to be the unique identifier of that specific generated token. Path would be too generic I think?

catsby · 2020-12-02T23:34:49Z

vault/resource_nomad_secret_backend.go

+		return nil
+	}
+
+	if val, ok := resp.Data["address"]; ok {


I think these values should be read and update the state regardless if they are set in the response. If we use this resource to set them, and then out-of-band the backend is destroyed and recreated at the same address but without these values, we'll fail to detect the drift here.

This is Required: true, so you would get an error if you didn't set it. Thoughts?

catsby · 2020-12-02T23:40:33Z

vault/resource_nomad_secret_backend.go

+	tune := api.MountConfigInput{}
+	data := map[string]interface{}{}
+
+	if defaultTTL != 0 {


I believe this prevents us from initially setting a default lease to something that is not the default system lease, and then removing it, e.g. we can't set it to something non-standard and then try to later adapt the system defaults implicitly by removing our non-standard value.

Example:

System default is 5m

We use default_lease_ttl_seconds to set it to 6m instead

Later we decide we want to use the system defaults, so we remove our default_lease_ttl_seconds

No change is detected here, because the new value would be the zero value, right?

No update happens, and the default least ttl remains at 6m

Is my assumption correct here?

Agreed, fixed.

vault/resource_nomad_secret_backend_test.go

vault/resource_nomad_secret_backend.go

* Add Nomad secret backend * Update vault/resource_nomad_secret_backend.go Co-authored-by: Theron Voran <[email protected]> * Update vault/resource_nomad_secret_role.go Co-authored-by: Theron Voran <[email protected]> * Change PathInventory to config/access * Add more paths to inventory * Merge lease config with backend resource * Fix updating mount parameters * Remove required flag from token/address * Remove commented code Co-authored-by: Theron Voran <[email protected]>

ghost added size/XXL documentation labels Nov 30, 2020

Add Nomad secret backend

d9e8686

jasonodonnell force-pushed the nomad-backend branch from 1e79c31 to d9e8686 Compare November 30, 2020 20:43

jasonodonnell requested review from catsby, tomhjp and tvoran November 30, 2020 20:44

calvn reviewed Nov 30, 2020

View reviewed changes

tvoran reviewed Dec 2, 2020

View reviewed changes

jasonodonnell and others added 3 commits December 1, 2020 20:41

Update vault/resource_nomad_secret_backend.go

98b38ee

Co-authored-by: Theron Voran <[email protected]>

Update vault/resource_nomad_secret_role.go

8a817a1

Co-authored-by: Theron Voran <[email protected]>

Change PathInventory to config/access

2ac09f1

tvoran approved these changes Dec 2, 2020

View reviewed changes

Add more paths to inventory

496d0d5

catsby reviewed Dec 2, 2020

View reviewed changes

jasonodonnell added 3 commits December 3, 2020 13:45

Merge lease config with backend resource

247f849

Fix updating mount parameters

3d0c71e

Remove required flag from token/address

9a611db

tvoran reviewed Dec 10, 2020

View reviewed changes

vault/resource_nomad_secret_backend_test.go Show resolved Hide resolved

vault/resource_nomad_secret_backend.go Outdated Show resolved Hide resolved

Remove commented code

537b2d5

jasonodonnell merged commit 00b8dd3 into master Dec 11, 2020

jasonodonnell deleted the nomad-backend branch December 11, 2020 14:46

swenson mentioned this pull request Aug 3, 2022

Feature Request: Support the Nomad secrets backend #640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Nomad secret backend #923

Add Nomad secret backend #923

jasonodonnell commented Nov 30, 2020 •

edited

Loading

calvn Nov 30, 2020

jasonodonnell Nov 30, 2020

jasonodonnell Nov 30, 2020

jasonodonnell Dec 1, 2020

tvoran Dec 2, 2020

jasonodonnell Dec 2, 2020 •

edited

Loading

jasonodonnell Dec 2, 2020

tvoran Dec 2, 2020

jasonodonnell Dec 2, 2020

jasonodonnell Dec 3, 2020

tvoran left a comment

tvoran Dec 2, 2020

catsby Dec 2, 2020

jasonodonnell Dec 3, 2020

catsby Dec 2, 2020

jasonodonnell Dec 3, 2020

catsby Dec 2, 2020

jasonodonnell Dec 3, 2020

Add Nomad secret backend #923

Add Nomad secret backend #923

Conversation

jasonodonnell commented Nov 30, 2020 • edited Loading

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

jasonodonnell Dec 2, 2020 • edited Loading

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

tvoran left a 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

jasonodonnell commented Nov 30, 2020 •

edited

Loading

jasonodonnell Dec 2, 2020 •

edited

Loading