-
Notifications
You must be signed in to change notification settings - Fork 16
k8s/controller: always map "default" Consul namespace to empty string #483
Conversation
bbf7494
to
0a7bc44
Compare
// Re-enable namespace mirroring | ||
ctx, err = e2e.SetNamespaceMirroring(true)(ctx, nil) | ||
require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was surprised this would be necessary, as I expected SetUpStack
to create a new context for each test, but it appears to be reused - I'm not sure if that's intentional/common, or if that should be changed and this cleanup could be removed?
Without this cleanup, the subsequent TestGatewayBasic
e2e fails when running against Consul Enterprise, as it checks for the presence of a Consul service registered in an expected namespace.
If keeping this cleanup logic makes more sense, I'm wondering if e2e.SetConsulNamespace(nil)
should also be called here to randomize the Consul namespace again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run into this issue before when touching the e2e tests as well. I'm not sure what the right solution is because I definitely think spinning up a new kind cluster for each test would make these tests take way longer, but the current set up make it difficult to do any kind of namespace manipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for hopping on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch itself looks fine to me
Co-authored-by: Andrew Stucki <[email protected]>
Changes proposed in this PR:
Always map the
"default"
Consul namespace to the empty string in the memory backend store, for compatibility with Consul OSS and the logic to parse the namespace of a Gateway from the SPIFFE URI, used when reading a Gateway from a store backend.This Consul namespace mapper function is already called when reconciling a Gateway to a backend store, but the corresponding lookup fails with Consul Enterprise when
--consul-destination-namespace
is set to"default"
because of the special case handling in theparseURI
helper.No migration handling should be needed because this is all within the ephemeral memory store for now, but this does need an e2e test for a service in the
"default"
namespace on Consul Enterprise.As a temporary workaround, this edge case can be avoided by configuring
consulNamespaces.mirroringK8S: true
or specfiying""
or any value other than"default"
forconsulNamespaces.consulDestinationNamespace
in the Helm chart.How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: