[Enhancement] Consul ESM to support Admin Partitions #281
Conversation
|
Just tested this against
With 100% success |
|
@Esity Thanks for testing the draft, I have made few cleanups after you have tested it. These cleanups were required to make the test passing. Do you mind testing this version as-well? |
There was a problem hiding this comment.
Thanks so much for making these changes! This is looking great, just a couple of comments on whether we got all the cases and some tests we could add.
For each .go file that changed, would it make sense to add a test with non-default partitions? For example in leader.go, you added partitions to commitOps(). I think it could make sense to then update the test for that function or add a new test that just tests that function with non-default partitions in the inputs. Similarly, for example in agent.go, you added partitions to the getHealthChecks function, so you could add to the test func TestAgent_getHealthChecks some cases with non-default partitions.
The way I reviewed was I was looking for every instance of api. or client. and checked if partition was added.
@ndhanushkodi Since this is OSS project hence, we may not be able to add API related test with partition since it is enterprise only feature. |
So for a similar example, consul-k8s is also an open source repo, but for example here it tests the behavior with non-default namespaces. It doesn't have to be a separate build but similar to how you're passing a non-default partition to this unit test here, you can also do something similar for tests for functions that you updated. I would say for the scope of this PR it would make sense to add non-default partitions to tests that already exist, such as |
|
@ndhanushkodi Added more admin partition tests with mock server. |
| } | ||
|
|
||
| // HasPartition checks if the partition is valid and calls the callback with the partition if it has any. | ||
| func (a *Agent) HasPartition(callback func(partition string)) { |
There was a problem hiding this comment.
this reads a little strange to me, is this checking if a partition is valid then it should be returning a bool to indicate if it's valid (and should probably be named ValidPartition) though this isn't checking if the partition is valid, it's just checking if the partition on the agent is non-emtpy and non-default, I'd probably get rid of this function entirely and re-write where it's used to just assign the output of PartitionOrEmtpy
| cases := []struct { | ||
| name, partition, expected string | ||
| }{ | ||
| {"No partition", "", ""}, |
There was a problem hiding this comment.
can we use the struct keys here so it's a bit clearer what each value means?
| if tc.partition == "" { | ||
| agent = testAgent(t, func(c *Config) { | ||
| c.HTTPAddr = ts.URL | ||
| c.InstanceID = "test-agent" | ||
| }) | ||
| } else { | ||
| agent = testAgent(t, func(c *Config) { | ||
| c.HTTPAddr = ts.URL | ||
| c.InstanceID = "test-agent" | ||
| c.Partition = tc.partition | ||
| }) | ||
| } |
There was a problem hiding this comment.
so the zero value for a string here is "" so we can just always assign the partition value because in the first case tc.partition == "" the Partition field on the config struct will just be "" so we don't need the conditional check here
| if tc.partition == "" { | |
| agent = testAgent(t, func(c *Config) { | |
| c.HTTPAddr = ts.URL | |
| c.InstanceID = "test-agent" | |
| }) | |
| } else { | |
| agent = testAgent(t, func(c *Config) { | |
| c.HTTPAddr = ts.URL | |
| c.InstanceID = "test-agent" | |
| c.Partition = tc.partition | |
| }) | |
| } | |
| agent = testAgent(t, func(c *Config) { | |
| c.HTTPAddr = ts.URL | |
| c.InstanceID = "test-agent" | |
| c.Partition = tc.partition | |
| }) |
| case strings.Contains(uri, "health/service"): | ||
| assert.Equal(t, tc.partition, r.URL.Query().Get(partitionQueryParamKey)) | ||
| fmt.Fprint(w, healthserviceJSON) | ||
| } |
There was a problem hiding this comment.
minor, but could we add a default case here that errors if an unexpected endpoint is called so we can be sure that we're catching all the cases?
|
@jm96441n Even the empty partition fails with OSS/CE version of consul. |
Background
Many customers are looking forward for ESM to support Admin partition.
Changes proposed
There is need for additional configuration in order to support Admin partition.
Note: ESM configured with a particular partition needs to connect to its respective
Consul Client Agentwith same partition configured.