vault: detect namespace change in config reload

[tgross]

The namespace field was not included in the equality check between old and new
Vault configurations, which meant that a Vault config change that only changed
the namespace would not be detected as a change and the clients would not be
reloaded.

Also, the comparison for boolean fields such as enabled and
allow_unauthenticated was on the pointer and not the value of that pointer,
which results in spurious reloads in real config reload that is easily missed in
typical test scenarios.

Includes a minor refactor of the order of fields for Copy and Merge to match
the struct fields in hopes it makes it harder to make this mistake in the
future, as well as additional test coverage.

---

This comes out of #14233 but doesn't fix it as far as I can tell.

[lgfa29]

Minor comments on the test, but LGTM!

[lgfa29]

Is newConfig supposed to be different from agentConfig? It think you would want to test if newConfig was applied after a Reload?

[tgross]

Oh good call. And I should be checking agent.GetConfig().Vault here too to get the updated config.

[tgross]

Well as it turns out, reloading the config doesn't set the server config we keep in the server struct. That still holds on to the old version of the config as far as I can tell, because we're never supposed to read from it at runtime. That seems like a bug, tbh, but unrelated and there's a lot of threads to pull on that.

We do overwrite the VaultConfig that we write into the server's VaultClient, but I can't get to the one in the agent in this test. I've added an accessor method so that we can access it in the nomad/server_test.go test at least.

[tgross]

Done in 9692586

[shoenig]

i was thinking, maybe we could have pointer.Compare similar to (and replacing) helper.CompareTimePtrs - caveat being it should only be used on basic types (not on structs with also pointer fields)

[tgross]

Good idea, lemme take a crack at that.

[shoenig]

caveat being it should only be used on basic types

what am I thinking, we can enforce that with a type parameter constraint!

[shoenig]

pretty sure this is effectively what we'd want

// Compare returns whether a and b contain the same value.
func Compare[A constraints.Ordered](a, b *A) bool {
// but also check nil
	return *a == *b
}```

[schmichael]

Yeah, something like this? https://go.dev/play/p/nE1aQVTzsuz

[shoenig]

the problem is I want to prevent comparisons like this:

https://go.dev/play/p/Lh26c-YWmBd

using constraints.Ordered as a constraint prevents comparing pointers of anything but underlying Numeric/String types which are safe

[tgross]

hrm, I seem to be stuck there.

type primitivePointer interface {
	*bool | *string | *int // etc, we can add the rest here
}

// Compare is an equality check for pointers that point to primitives.
func Compare[A primitivePointer](a, b A) bool {
	if a == nil || b == nil {
		return a == b
	}

	return *a == *b
}

Gives me:

# github.com/hashicorp/nomad/helper/pointer
helper/pointer/pointer.go:28:10: invalid operation: pointers of a (variable of type A constrained by primitivePointer) must have identical base types
helper/pointer/pointer.go:28:16: invalid operation: pointers of b (variable of type A constrained by primitivePointer) must have identical base types

For now I'm going to merge the bugfix and then we'll come up with some nicer comparison functions in a separate PR.

[tgross]

Ah, I posted that without seeing the other solutions. In any case, probably better for another PR.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.