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

TestCore with KV v2 doesn't generate metadata #8440

Closed
glindstedt opened this issue Mar 2, 2020 · 12 comments
Closed

TestCore with KV v2 doesn't generate metadata #8440

glindstedt opened this issue Mar 2, 2020 · 12 comments
Labels

Comments

@glindstedt
Copy link

Describe the bug

While using the TestCore in the testing module to write integration tests, and mounting a KV v2 key-value store, it seems like it does not behave the same as a proper vault deployment, unless I'm missing some fundamental part of the setup process. Normally when creating a secret through the HTTP api under kv/data/test a matching metadata entry would get created under kv/metadata/test, however this does not seem to happen when using TestCore.

To Reproduce

This test displays the issue:

package test

import (
	"testing"

	"github.com/hashicorp/vault/api"
	"github.com/hashicorp/vault/http"
	"github.com/hashicorp/vault/vault"
)

func Test(t *testing.T) {
	core, _, rootToken := vault.TestCoreUnsealed(t)
	_, addr := http.TestServer(t, core)
	c, _ := api.NewClient(api.DefaultConfig())
	c.SetAddress(addr)
	c.SetToken(rootToken)

	// Create KV V2 mount
	c.Sys().Mount("kv", &api.MountInput{
		Type: "kv",
		Options: map[string]string{
			"version": "2",
		},
	})

	c.Logical().Write("kv/data/test", map[string]interface{}{
		"data": map[string]interface{}{
			"hello": "world",
		},
	})

	// Read secret data
        // Output: map[hello:world]
	secret, _ := c.Logical().Read("kv/data/test")
	t.Error(secret.Data["data"])

	// Read secret metadata
        // Output: <nil> <nil>
	secretMeta, err := c.Logical().Read("kv/metadata/test")
	t.Error(secretMeta, err)
}

Expected behavior

That the appropriate metadata entry would be created with the new secret.

Environment:

  • Go 1.14
  • vault api 1.3.2
@catsby catsby added bug Used to indicate a potential bug secret/kv labels Mar 4, 2020
@catsby
Copy link
Contributor

catsby commented Mar 4, 2020

Hello -

TestCoreUnsealed doesn't use the regular KV backend, instead it uses a PassthroughBackend to essentially mock the kv behavior. TestCoreUnsealed leads to TestCoreWithSeal:

vault/vault/testing.go

Lines 125 to 133 in 6c45080

func TestCoreWithSeal(t testing.T, testSeal Seal, enableRaw bool) *Core {
conf := &CoreConfig{
Seal: testSeal,
EnableUI: false,
EnableRaw: enableRaw,
BuiltinRegistry: NewMockBuiltinRegistry(),
}
return TestCoreWithSealAndUI(t, conf)
}

There, a CoreConfig is created that does not define any LogicalBackends. By the time the new core is created in NewCore, a default, passthrough kv is added:

vault/vault/core.go

Lines 881 to 888 in 17361f2

logicalBackends := make(map[string]logical.Factory)
for k, f := range conf.LogicalBackends {
logicalBackends[k] = f
}
_, ok := logicalBackends["kv"]
if !ok {
logicalBackends["kv"] = PassthroughBackendFactory
}

Contrast that with something like testVaultServerUnseal:

// testVaultServerUnseal creates a test vault cluster and returns a configured
// API client, list of unseal keys (as strings), and a closer function.
func testVaultServerUnseal(tb testing.TB) (*api.Client, []string, func()) {
tb.Helper()
return testVaultServerCoreConfig(tb, &vault.CoreConfig{
DisableMlock: true,
DisableCache: true,
Logger: defaultVaultLogger,
CredentialBackends: defaultVaultCredentialBackends,
AuditBackends: defaultVaultAuditBackends,
LogicalBackends: defaultVaultLogicalBackends,
BuiltinRegistry: builtinplugins.Registry,
})
}

That setups an unsealed vault cluster configured with a normal KV backend defined here:

defaultVaultLogicalBackends = map[string]logical.Factory{
"generic-leased": vault.LeasedPassthroughBackendFactory,
"pki": pki.Factory,
"ssh": ssh.Factory,
"transit": transit.Factory,
"kv": kv.Factory,
}

That core config eventually gets used in vault.NewTestCluster:

func NewTestCluster(t testing.T, base *CoreConfig, opts *TestClusterOptions) *TestCluster {

Granted that testVaultServerUnseal is a private method, you could look to some of our other tests that use NewTestCluster, for example the pki backend here:

var b *backend
factory := func(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) {
be, err := Factory(ctx, conf)
if err == nil {
b = be.(*backend)
}
return be, err
}
coreConfig := &vault.CoreConfig{
LogicalBackends: map[string]logical.Factory{
"pki": factory,
},
}

The factory function is defined inline there, but for KV you'd want to reference the KV factory defined in that plugin:

https://github.com/hashicorp/vault-plugin-secrets-kv/blob/3541c91333def6240e4e2640adc3c9e7bf13f409/backend.go#L73-L91

Unfortunately in the end TestCoreUnsealed isn't documented to have this limitation 😦 I admit I had to do some digging and asking around to discover what was (not) happening here. For testing Vault backends or integrations, I would use NewTestCluster.

Hopefully this addresses your questions!

@glindstedt
Copy link
Author

@catsby Thank you for the really thorough answer!

I've successfully refactored my tests to use NewTestCluster instead and it works like a charm. Really appreciate that you showed all the code snippets, it gave insight into how things fit together and helped with the migration.

I assume you might want to keep the issue around to maybe get some documentation about the limitation in TestCoreUnsealed, but in my case at least the issue is resolved.

@catsby
Copy link
Contributor

catsby commented Mar 5, 2020

Thanks for following up, @glindstedt. I'll leave this open as you suggest, I'm not honestly sure why TestCoreUnsealed functions the way it does. Generally though, NewTestCluster seems to be the better option, so I'm glad you were able to get that working.

@mwodrich
Copy link

mwodrich commented Mar 26, 2020

I figure since I just ran headlong into this issue myself that I’d like to thank you both for documenting and explaining this issue, and that I should stand up and be counted as another person this affects.

I was writing tests to verify that I was working appropriately with versioned KV v2 data when I found that the responses from the TestCoreUnsealed server contained my data, but were structurally unlike the documented responses.

In terms of alternatives to TestCoreUnsealed, upon discovering testVaultServerUnseal in the code, I was excited when I saw how straightforward it was to use testVaultServerUnseal in the command tests and then immediately crestfallen when I realized it was only defined in the test file and not exported. I think it would be very cool if that could become accessible to folks writing tests since it so elegantly provides the needed test server.

I thought I’d be able to get by with TestCoreUnsealed, but with this limitation that’s definitely not the case. +1 to encouraging that its limitations should be better documented. For my part, I’ll also transition my tests over to NewTestCluster along the lines you’ve described.

Thanks again!

@catsby
Copy link
Contributor

catsby commented Mar 26, 2020

Thanks for the input @mwodrich , I'm sorry for your troubles. This and other comments have been taken to heart and internally we're discussing and working on improvements to our current tests and tools/methods to improve future testing. I don't have a timeline so please bear with us, but we are working on it 😄

@innovia
Copy link

innovia commented Apr 18, 2020

@catsby Thank you for the really thorough answer!

I've successfully refactored my tests to use NewTestCluster instead and it works like a charm. Really appreciate that you showed all the code snippets, it gave insight into how things fit together and helped with the migration.

I assume you might want to keep the issue around to maybe get some documentation about the limitation in TestCoreUnsealed, but in my case at least the issue is resolved.

@glindstedt can you please provide a sample code, im having a hard time figuring this out

@glindstedt
Copy link
Author

@innovia Here's the helper function we're using to start the test cluster and add a KV v2 mount:

import (
	kv "github.com/hashicorp/vault-plugin-secrets-kv"
	"github.com/hashicorp/vault/api"
	"github.com/hashicorp/vault/sdk/logical"
	vaulthttp "github.com/hashicorp/vault/http"
	hashivault "github.com/hashicorp/vault/vault"
)

func createVaultTestCluster(t *testing.T) *hashivault.TestCluster {
	t.Helper()

	coreConfig := &hashivault.CoreConfig{
		LogicalBackends: map[string]logical.Factory{
			"kv": kv.Factory,
		},
	}
	cluster := hashivault.NewTestCluster(t, coreConfig, &hashivault.TestClusterOptions{
		HandlerFunc: vaulthttp.Handler,
	})
	cluster.Start()

	// Create KV V2 mount
	if err := cluster.Cores[0].Client.Sys().Mount("kv", &api.MountInput{
		Type: "kv",
		Options: map[string]string{
			"version": "2",
		},
	}); err != nil {
		t.Fatal(err)
	}

	return cluster
}

func TestSomething(t *testing.T) {
    cluster := createVaultTestCluster(t)
    defer cluster.Cleanup()
    rootVaultClient := cluster.Cores[0].Client
    // do your testing
}

Hope it helps!

@innovia
Copy link

innovia commented Apr 20, 2020

Thanks a lot! @glindstedt this is extremely helpful!

Its working fine, my mistake for misplacing defer and closing the connection right before i vene started tests.

@innovia
Copy link

innovia commented Apr 20, 2020

Wanted to add the proper way of setting up a v2 secret

KV version 2 expects the values to be in a Data map: https://www.vaultproject.io/api/secret/kv/kv-v2.html#data

secret["data"] = map[string]interface{}{
    "value": pass,
}```

and the path to write the secret must have the `/data` prefix after the secret kv name
`secrets/v2/data/plain/secrets/path/app`

phanyzewski added a commit to teamsnap/vault-key that referenced this issue Apr 28, 2020
Prefer NeseTestCluster over TestCoreUnsealed for v2.
hashicorp/vault#8440
@HridoyRoy HridoyRoy added docs pod/bridge and removed bug Used to indicate a potential bug secret/kv labels Feb 18, 2021
@HridoyRoy HridoyRoy self-assigned this Feb 18, 2021
@HridoyRoy
Copy link
Contributor

Hi folks,
Seems like this issue has been resolved and just needs some extra documentation around it. Is that right?

@aphorise
Copy link
Contributor

aphorise commented Sep 3, 2022

@glindstedt any further updates here? - is issue still applicable or anything further pending?

@heatherezell
Copy link
Contributor

Due to issue age and quiescence, I'm going to go ahead and close this now. Please feel free to re-open as needed. Thanks!

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

No branches or pull requests

7 participants