Update to command line messages for tctl create commands

[hegurhurgurk]

Updated the create commands to make their output more uniform. Also updated the upsert verb to use more accurate verbs.

[zmb3]

Thanks for the contribution, but I'm not convinced this extra logic is something we need at this point.

[ibeckermayer]

Thanks for the contribution, but I'm not convinced this extra logic is something we need at this point.

@zmb3 I put @hegurhurgurk up to this, the reason for the extra logic is to make it so that for all resources, if the resource already exists, users must call tctl create -f resource.yaml in order to overwrite them.

Before these changes, some resources required -f to be overwritten, while others didn't, with no particular rhyme or reason.

[zmb3]

Before these changes, some resources required -f to be overwritten, while others didn't, with no particular rhyme or reason.

Sure, but now we may break existing scripts or automation.

Additionally, calling get before an upsert as an attempt to detect whether the item was created or updated doesn't seem right. (It makes the operation take twice as long since there are now 2 round trips, and you can't be sure the resource wasn't inserted in between the get and the upsert.)

[ibeckermayer]

Sure, but now we may break existing scripts or automation.

In that case how about we avoid any backports and keep this as a change in a major release (breaking API changes).

Additionally, calling get before an upsert as an attempt to detect whether the item was created or updated doesn't seem right. (It makes the operation take twice as long since there are now 2 round trips, and you can't be sure the resource wasn't inserted in between the get and the upsert.)

Valid point, the counterpoint being that this is how we're already handling this behavior for many of the resources in tctl create, e.g. for tctl create user and tctl create role.

https://github.com/gravitational/teleport/blob/9c55a33e0143f1e8bd6063872e139705a2e47941/tool/tctl/common/resource_command.go#L441

https://github.com/gravitational/teleport/blob/9c55a33e0143f1e8bd6063872e139705a2e47941/tool/tctl/common/resource_command.go#L388

Two round trips is obviously worse than one, but I think data integrity (making sure the user is being explicit by making them use -f) and API consistency (all else equal, that behavior is the same for all resources) is a more important goal than performance for these typically-one-off command line operations.

I don't know if there's any way to solve for that potential race condition given our current system without adding some sort of distributed locking mechanism on resources.

Ultimately I think we should

1. Make sure all of our CreateX methods return trace.AlreadyExists if the resource already exists (relates to https://github.com/gravitational/teleport/issues/29234)
2. Make tctl create X only call CreateX methods, and fail if the resource already exists.
3. Create a separate tctl upsert X method which only calls UpsertX methods (also relates to https://github.com/gravitational/teleport/issues/29234)

However I still think this "quick and dirty" fix to enforce consistency is worthwhile for the time being.

[ibeckermayer]

Buddy PR: https://github.com/gravitational/teleport/pull/33043

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

:x: ibeckermayer
:x: hegurhurgurk
_{You have signed the CLA already but the status is still pending? Let us recheck it.}