search

sensu / sensu-go

Simple. Scalable. Multi-cloud monitoring.
https://sensu.io
MIT License
1.03k stars 174 forks source link

Inconsistent UX for resource types in sensuctl commands (ex. dump,prune,list) #3981

Open nikkictl opened 4 years ago

nikkictl commented 4 years ago

The sensuctl dump command has had a few iterations in UX since its inception. While more use cases are accounted for and the UX has certainly improved, we still have some inconsistencies with what we consider valid ways to represent a resource type. In this issue, I will provide a brief history of the design, overview of the current behavior, and proposed solution to the problems presented.

History

Originally, the command was a wrapper around sensuctl list where only short name resource types were supported. For example sensuctl dump check,entity,role-binding,api-key would basically run sensuctl check list && sensuctl entity list && sensuctl role-binding list && sensuctl api-key list. This approach limited users in how expressive they could be to define a resource type, and had other developmental issues such as hard coded resource names.

Some improvements to this design included @echlebek's approach to generically and dynamically generate resource types by their fully qualified API names. For example sensuctl dump core/v2.Check was equivalent to the short name check and directly correlated to the Go API package. This was especially advantageous to solidifying our resource type system, but while the approach intended to be backwards compatible, it actually introduced unintentional inconsistencies, for example, support for RBAC short names (ex. sensuctl dump checks,entities,rolebindings,apikeys) on top of sensuctl command nouns (ex. sensuctl dump check,entity,role-binding,api-key).

Current Behavior

After some refactors to the types system and resource interfaces, sensuctl dump exhibited the following behaviors, which have multiple ways to express a resource type inconsistently across resources.

Dump checks

✅ sensuctl dump check ✅ sensuctl dump checks ✅ sensuctl dump core/v2.Check

Dump entities

✅ sensuctl dump entity ✅ sensuctl dump entities ✅ sensuctl dump core/v2.Entity

Dump role bindings

❌ sensuctl dump rolebinding ✅ sensuctl dump rolebindings ✅ sensuctl dump role-binding ✅ sensuctl dump role-bindings ✅ sensuctl dump core/v2.RoleBinding

Dump api keys

❌ sensuctl dump apikey ✅ sensuctl dump apikeys ❌ sensuctl dump api-key ❌ sensuctl dump api-keys ❌ sensuctl dump core/v2.ApiKey ✅ sensuctl dump core/v2.APIKey

Dump lifted providers (ex. secrets/v1.Provider and authentication/v2.Provider) see https://github.com/sensu/sensu-enterprise-go/issues/1216 for more information

❌ sensuctl dump secrets/v1.VaultProvider (accepted arg, but output is incorrect) ❌ sensuctl dump secrets/v1.Env (accepted arg, but output is incorrect) ✅ sensuctl dump secrets/v1.Provider

Describe-type only lists fully qualified name and short name (RBAC name)

$ sensuctl describe-type all
      Fully Qualified Name           Short Name           API Version             Type          Namespaced  
 ────────────────────────────── ───────────────────── ─────────────────── ──────────────────── ──────────── 
  authentication/v2.Provider                           authentication/v2   Provider             false       
  licensing/v2.LicenseFile                             licensing/v2        LicenseFile          false       
  store/v1.PostgresConfig                              store/v1            PostgresConfig       false       
  federation/v1.EtcdReplicator                         federation/v1       EtcdReplicator       false       
  secrets/v1.Secret                                    secrets/v1          Secret               true        
  secrets/v1.Provider                                  secrets/v1          Provider             false       
  searches/v1.Search                                   searches/v1         Search               true        
  web/v1.GlobalConfig                                  web/v1              GlobalConfig         false       
  core/v2.Namespace              namespaces            core/v2             Namespace            false       
  core/v2.ClusterRole            clusterroles          core/v2             ClusterRole          false       
  core/v2.ClusterRoleBinding     clusterrolebindings   core/v2             ClusterRoleBinding   false       
  core/v2.User                   users                 core/v2             User                 false       
  core/v2.APIKey                 apikeys               core/v2             APIKey               false       
  core/v2.TessenConfig           tessen                core/v2             TessenConfig         false       
  core/v2.Asset                  assets                core/v2             Asset                true        
  core/v2.CheckConfig            checks                core/v2             CheckConfig          true        
  core/v2.Entity                 entities              core/v2             Entity               true        
  core/v2.Event                  events                core/v2             Event                true        
  core/v2.EventFilter            filters               core/v2             EventFilter          true        
  core/v2.Handler                handlers              core/v2             Handler              true        
  core/v2.Hook                   hooks                 core/v2             Hook                 true        
  core/v2.Mutator                mutators              core/v2             Mutator              true        
  core/v2.Role                   roles                 core/v2             Role                 true        
  core/v2.RoleBinding            rolebindings          core/v2             RoleBinding          true        
  core/v2.Silenced               silenced              core/v2             Silenced             true

Expected Behavior

Possible Solution

Implementing stricter validation around acceptable resource type arguments (in sensuctl dump/prune/describe-type etc) could be argued as a breaking change. Limiting nouns to the 2 options listed in sensuctl describe-type would ultimately end support for kebab-cased nouns that are supported in sensuctl [RESOURCE] list, such as role-binding. However, since kebab-cased nouns are not consistently enforced (ex. sensuctl dump api-key), it could also be argued that kebab-case only happened to previously work out of coincidence.

IMO, we should implement strict enforcements for resource type definitions, otherwise users may end up with unintended results. The logic reuse of resource resolution in sensuctl prune makes those assumptions much more dangerous. See https://github.com/sensu/sensu-go/compare/release/6.0...bugfix/sensuctl-dump for POC and https://github.com/sensu/sensu-go/pull/3982 for WIP PR.

Steps to Reproduce (for bugs)

  1. Observe the output of sensuctl describe-type all
  2. Successfully resolve a resource type that's not listed in sensuctl describe-type all, ex. sensuctl describe-type secrets/v1.VaultProvider
  3. Run the sensuctl dump commands listed above to observe conflicting behaviors
  4. Run sensuctl [RESOURCE] list on some of the nouns listed above to observe conflicting behaviors

Context

There has been a lot of general confusion from developers, advocates, and users around the proper noun use in the sensuctl CLI. The sensuctl dump command is the preferred method of backup/restore for Sensu, therefore resource definitions should be extremely explicit. The sensuctl prune command is still considered alpha/experimental, but is a destructive command, and therefore resource definitions should be extremely explicit.

Your Environment

nikkictl commented 4 years ago

A few notes after some discussion:

  1. Limiting the ways to express a resource type is ideal but would be considered a breaking change.
  2. Until we're ready for another major version bump, we'll update documentation to use the resource types defined in sensuctl describe-type all, as it contains the more reliable ways to express a resource type https://github.com/sensu/sensu-docs/issues/2692.
  3. There are known issues around short names for resource types and we could add support for all of them, but that could also just convolute the problem more.
  4. We'll continue to patch bugs around this UX in the most generic way possible, but document additional issues related to these inconsistencies.
calebhailey commented 4 years ago

This is an amazing issue description! Thanks @nikkictl!

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.