feature: add command for exporting service to peer or partition #15654
Conversation
github-actions
bot
added
pr/dependencies
command/peering/export/export.go
Outdated
| if !serviceExists { | ||
| cfg.Services = append(cfg.Services, api.ExportedService{ | ||
| Name: c.serviceName, | ||
| Namespace: c.serviceNamespace, |
There was a problem hiding this comment.
Notably, we can assign a namespace here even when using OSS. Discussed in the issue w/ @DanStough that we will want to update the config-entry validation on the server to return an error such as enterprise-only feature in a future PR.
There was a problem hiding this comment.
One more question for you @DanStough: should we be making API calls to query for the service being exported to ensure that it exists, or should we depend on the server to validate that, or is that up to the user?
|
@nathancoleman , w.r.t. checking if the service exists, I would say no. Generally, design things in the CLI to support eventual consistency, so you don't need to worry about the order in which these things are created. It would be cool to have a |
|
This looks awesome! One note is this would look almost identical if we made a command for exporting to partitions. I wonder if this should be a new top-level command, and we can create a followup tasks to support both |
|
@DanStough the question of whether this single command supports both peers and partitions or whether we have this command and another parition-specific command elsewhere in the CLI is something I'm expecting an answer from Jared on before we merge |
20sr20
force-pushed
the
export-peering-cli
branch
from
85eee9a
to
43ef742
Compare
nathancoleman
force-pushed
the
export-peering-cli
branch
from
a99d040
to
0f68ffe
Compare
| t.Run("initial config entry should be created w/ partitions", func(t *testing.T) { | ||
|
|
||
| ui := cli.NewMockUi() | ||
| cmd := New(ui) | ||
|
|
||
| args := []string{ | ||
| "-http-addr=" + a.HTTPAddr(), | ||
| "-name=testservice", | ||
| "-consumer-partitions=a,b", | ||
| } | ||
|
|
||
| code := cmd.Run(args) | ||
| require.Equal(t, 0, code) | ||
| require.Contains(t, ui.OutputWriter.String(), "Successfully exported service") | ||
| }) |
There was a problem hiding this comment.
In a perfect world, we would move the partition test cases to another file export_test_ent.go, but since it looks like we don't have any server-side validation for this, I'm fine leaving it as it is.
| return 1 | ||
| } | ||
|
|
||
| partitionNames, err := c.getPartitionNames() |
There was a problem hiding this comment.
What happens if partitions are specified in OSS?
There was a problem hiding this comment.
This follows the same theme as other enterprise features in this PR. The command with partitions specified will go through even for OSS users, same as if you use $ consul config write today (since this is really all that's happening behind the scenes).
Since the established pattern is to allow Enterprise features in the OSS CLI, the correct thing to do in the longer term IMO is to add server validation for this config entry that returns an error in this case.
| ok, _, err := client.ConfigEntries().CAS(cfg, cfg.GetModifyIndex(), nil) | ||
| if err != nil { | ||
| c.UI.Error(fmt.Sprintf("Error writing config entry: %s", err)) | ||
| return 1 | ||
| } else if !ok { | ||
| c.UI.Error(fmt.Sprintf("Config entry was changed during update. Please try again")) | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
Nice due diligence in handling this case :)
| if c.partitionNames != "" { | ||
| partitionNames = strings.Split(c.partitionNames, ",") | ||
| for _, partitionName := range partitionNames { | ||
| if partitionName == "" { |
There was a problem hiding this comment.
Is there any other validation of peer or partition names beyond "is it empty" that the backend does?
I guess the possible harm is that someone could write a config entry for a non-existent partition, or one with a mistyped/garbled name?
That said, we output a string like Successfully exported service %q to peers %q afterwards, so that would hopefully show whether something was wrong.
So maybe additional validation isn't important (unless it was really easy to do).
There was a problem hiding this comment.
@DanStough and I talked at one point about adding validation in the backend for this as a separate PR.
Today, you can write a config entry that exports to a non-existent peer or partition using consul config write - this command is no different since it uses the same config entry mechanism. Based on that, I consider adding more validation on the server side to be outside the scope of this PR. It would be good to get on someone's radar though. Thoughts?
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
nathancoleman
changed the title
|
@nathancoleman Out of curiosity are there docs changes for this features as well? I currently do not see docs for this feature here: https://developer.hashicorp.com/consul/commands/services |
Description
Describe why you're making this change, in plain English.
Testing & Reproduction steps
Links
Include any links here that might be helpful for people reviewing your PR (Tickets, GH issues, API docs, external benchmarks, tools docs, etc). If there are none, feel free to delete this section.
Please be mindful not to leak any customer or confidential information. HashiCorp employees may want to use our internal URL shortener to obfuscate links.
PR Checklist