openapi: Improve operationId/request/response naming strategy

[averche]

Background

This PR changes the way we construct the operationId as well as the request & response names in the generated openapi.json document. operationIds are translated directly into method names in the two new OpenAPI-based client libraries:

• hashicorp/vault-client-go
• hashicorp/vault-client-dotnet

Old naming strategy

Today the approach is to naively construct operationId values from the API paths. Unfortunately, the resultant methods names are not very human-friendly. Here are a couple examples:

// notice roleRoleRole stutter and the redundant 'Auth' in the method name
client.Auth.PostAuthApproleRoleRoleName(...)

// this is the function generated for fetching KVv2 secrets (probably the most popular Vault use-case)
// but it sounds like it's getting a secret path instead!
client.Secrets.GetSecretDataPath(...)

Additionally, the request & response names had a different naming logic, which resulted in some naming collisions (#18578).

New naming strategy

The new naming strategy is to take advantage of the existing optional DisplayAttributes at Path and PathOperation level. I'm introducing a couple new DisplayAttributes fields:

vault/sdk/framework/path.go

Lines 227 to 243 in 72c4acd

	// OperationPrefix is a hyphenated lower-case string used to construct
	// OpenAPI OperationID. It is typically the name of the plugin.
	OperationPrefix string `json:"operationPrefix,omitempty"`
	// OperationPrefix is a hyphenated lower-case string used to construct
	// OpenAPI OperationID. It is typically an action to be performed
	// (e.g. "generate", "sign", "login", etc.). If not specified, the verb
	// defaults to `logical.Operation.String()` (e.g. "read", "delete", etc.).
	OperationVerb string `json:"operationVerb,omitempty"`
	// OperationPrefix is a hyphenated lower-case string used to construct
	// OpenAPI OperationID. It is typically the name of the resource on which
	// the action is performed (e.g. "role", "credentials", etc.). A pipe (|)
	// separator can be used to list different suffixes for various permutations
	// of the `Path.Pattern` regular expression. If not specified, the suffix
	// defaults to the `Path.Pattern` split by dashes.
	OperationSuffix string `json:"operationSuffix,omitempty"`

These fields will be used to help construct the operation ID / request / response names if defined. The fields will now be generated as hyphenated-lowercase-strings for better compatibility with various language generators.

For examples of how the new naming strategy works, please take a look at the new test:

vault/sdk/framework/openapi_test.go

Lines 571 to 744 in 72c4acd

	func TestOpenAPI_constructOperationID(t *testing.T) {
	tests := map[string]struct {
	path string
	pathIndex int
	pathAttributes *DisplayAttributes
	operation logical.Operation
	operationAttributes *DisplayAttributes
	defaultPrefix string
	expected string
	}{
	"empty": {
	path: "",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.Operation(""),
	operationAttributes: nil,
	defaultPrefix: "",
	expected: "",
	},
	"simple-read": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.ReadOperation,
	operationAttributes: nil,
	defaultPrefix: "test",
	expected: "test-read-path-to-thing",
	},
	"simple-write": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: nil,
	defaultPrefix: "test",
	expected: "test-write-path-to-thing",
	},
	"operation-verb": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationVerb: "do-something"},
	operation: logical.UpdateOperation,
	operationAttributes: nil,
	defaultPrefix: "test",
	expected: "do-something",
	},
	"operation-verb-override": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationVerb: "do-something"},
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationVerb: "do-something-else"},
	defaultPrefix: "test",
	expected: "do-something-else",
	},
	"operation-prefix": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix"},
	operation: logical.UpdateOperation,
	operationAttributes: nil,
	defaultPrefix: "test",
	expected: "my-prefix-write-path-to-thing",
	},
	"operation-prefix-override": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix"},
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix"},
	defaultPrefix: "test",
	expected: "better-prefix-write-path-to-thing",
	},
	"operation-prefix-and-suffix": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix"},
	operation: logical.UpdateOperation,
	operationAttributes: nil,
	defaultPrefix: "test",
	expected: "my-prefix-write-my-suffix",
	},
	"operation-prefix-and-suffix-override": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix"},
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix"},
	defaultPrefix: "test",
	expected: "better-prefix-write-better-suffix",
	},
	"operation-prefix-verb-suffix": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix", OperationVerb: "Create"},
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix"},
	defaultPrefix: "test",
	expected: "better-prefix-create-better-suffix",
	},
	"operation-prefix-verb-suffix-override": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: &DisplayAttributes{OperationPrefix: "my-prefix", OperationSuffix: "my-suffix", OperationVerb: "Create"},
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "better-suffix", OperationVerb: "Login"},
	defaultPrefix: "test",
	expected: "better-prefix-login-better-suffix",
	},
	"operation-prefix-verb": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationVerb: "Login"},
	defaultPrefix: "test",
	expected: "better-prefix-login",
	},
	"operation-verb-suffix": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationVerb: "Login", OperationSuffix: "better-suffix"},
	defaultPrefix: "test",
	expected: "login-better-suffix",
	},
	"pipe-delimited-suffix-0": {
	path: "path/to/thing",
	pathIndex: 0,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
	defaultPrefix: "test",
	expected: "better-prefix-write-suffix0",
	},
	"pipe-delimited-suffix-1": {
	path: "path/to/thing",
	pathIndex: 1,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
	defaultPrefix: "test",
	expected: "better-prefix-write-suffix1",
	},
	"pipe-delimited-suffix-2-fallback": {
	path: "path/to/thing",
	pathIndex: 2,
	pathAttributes: nil,
	operation: logical.UpdateOperation,
	operationAttributes: &DisplayAttributes{OperationPrefix: "better-prefix", OperationSuffix: "suffix0|suffix1"},
	defaultPrefix: "test",
	expected: "better-prefix-write-path-to-thing",
	},
	}
	for name, test := range tests {
	name, test := name, test
	t.Run(name, func(t *testing.T) {
	t.Parallel()
	actual := constructOperationID(
	test.path,
	test.pathIndex,
	test.pathAttributes,
	test.operation,
	test.operationAttributes,
	test.defaultPrefix,
	)
	if actual != test.expected {
	t.Fatalf("expected: %s; got: %s", test.expected, actual)
	}
	})
	}
	}

Related work

This PR is a new approach based on some valid concerns brought forward by @maxb in a related #18321.

The changes to define display attributes for specific plugins will be done in upcoming related PRs:

• openapi: Add display attributes for approle #19363
• openapi: Add display attributes for AWS #19366
• openapi: Add display attributes for cert auth #19383
• openapi: Add display attributes for GitHub auth #19384
• openapi: Add display attributes for LDAP auth #19389
• openapi: Add display attributes for Okta auth #19391
• openapi: Add display attributes for Radius auth #19392
• openapi: Add display attributes for token/ #19399
• openapi: Add display attributes for userpass #19411
• openapi: Add display attributes for Consul #19413
• openapi: Add display attributes for database plugin #19414
• openapi: Add display attributes for Nomad plugin #19415
• openapi: Add display attributes for PKI plugin #19422

[AnPucel]

I looked at the test, but could we get examples in the description of what the new OperationId's will map to that are actual paths right now? E.g. PostAuthApproleRoleRoleName will now look like... ?

[averche]

After this PR is merged, the names are not going to be in an ideal form. For instance, PostAuthApproleRoleRoleName will turn into approle-write-role-role-name (ApproleWriteRoleRoleName in code).

The follow-up PR(s) will populate the DisplayAttributes for AppRole and other plugins, so the final state will be close to what we have in the client libraries (app-role-write-role).

[maxb]

For instance, PostAuthApproleRoleRoleName will turn into ApproleWriteRoleRoleName.

There are quite a few cases where there is an auth method and a secrets engine of the same name (e.g AWS, Azure, Kubernetes, LDAP). For this reason, I think it might be better to retain the Auth in the default prefix of auth methods - otherwise operations of these auth methods and secrets engines will mingle, and in the worse case, possibly even clash.

[averche]

There are quite a few cases where there is an auth method and a secrets engine of the same name (e.g AWS, Azure, Kubernetes, LDAP). For this reason, I think it might be better to retain the Auth in the default prefix of auth methods - otherwise operations of these auth methods and secrets engines will mingle, and in the worse case, possibly even clash.

My plan is to address such ambiguities in related PR's by adding "Auth" into the OperationPrefix where necessary. Example: #19366. I will merge all PR's together once they are all reviewed & approved.