Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Design, refactor, and document CRUD semantics/behavior for auth server API #29234

Closed
ibeckermayer opened this issue Jul 17, 2023 · 2 comments
Closed
Assignees
Labels
refactoring robustness Resistance to crashes and reliability tctl tctl - Teleport admin tool

Comments

@ibeckermayer
Copy link
Contributor

Currently the auth.ClientI has awkward and sometimes inconsistent naming conventions with undocumented semantics and return values, e.g.:

awkward

UpdateX (e.g. UpdateUser) -- most of these blindly overwrite some resource except we require that anything was there in the first place. This isn't really a useful semantic, typically you want to do one of the following:

  • create a new object or error if it already exists ("create", http POST)
  • create or overwrite an object ("upsert", http PUT)
  • update particular fields of an existing object or error if it dne ("update", http PATCH)

inconsistent

An exception to the above UpdateX example is UpdateRemoteCluster (perhaps others?) which does implement http PATCH semantics.

Additionally, naming is sometimes inconsistent between methods, for example we have CreateX and SetX (or perhaps SetX should be compared to UpsertX, the semantics are unclear).

undocumented

At the time of writing, we have this logic in the tctl create codepath which seems to assume that a "create" on an existing resource will return a trace.AlreadyExists error, however it's unclear whether all of ClientI's CreateX methods do so (it would make sense if they did), or whether that needs to be implemented manually elsewhere.

Standardizing and documenting (and perhaps even adding integration tests for) such behavior should improve stability over the long run.

@rosstimothy
Copy link
Contributor

Does RFD 153 cover the request here?

@ibeckermayer
Copy link
Contributor Author

Does RFD 153 cover the request here?

Indeed it did

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring robustness Resistance to crashes and reliability tctl tctl - Teleport admin tool
Projects
None yet
Development

No branches or pull requests

2 participants