OpenAPI: Separate ListOperation from ReadOperation #21723

maxb · 2023-07-10T17:19:03Z

Historically, since Vault's ReadOperation and ListOperation both map to the HTTP GET method, their representation in the generated OpenAPI has been a bit confusing.

This was partially mitigated some time ago, by making the list query parameter express whether it was required or optional - but only in a way useful to human readers - the human had to know, for example, that the schema of the response body would change depending on whether list was selected.

Now that there is an effort underway to automatically generate API clients from the OpenAPI spec, we have a need to fix this more comprehensively. Fortunately, we do have a means to do so - since Vault has opinionated treatment of trailing slashes, linked to operations being list or not, we can use an added trailing slash on the URL path to separate list operations in the OpenAPI spec.

This PR implements that, and then fixes various OpenAPI DisplayAttrs consequential to this change.

See also hashicorp/vault-client-go#174, a bug which will be fixed by this work.

Historically, since Vault's ReadOperation and ListOperation both map to the HTTP GET method, their representation in the generated OpenAPI has been a bit confusing. This was partially mitigated some time ago, by making the `list` query parameter express whether it was required or optional - but only in a way useful to human readers - the human had to know, for example, that the schema of the response body would change depending on whether `list` was selected. Now that there is an effort underway to automatically generate API clients from the OpenAPI spec, we have a need to fix this more comprehensively. Fortunately, we do have a means to do so - since Vault has opinionated treatment of trailing slashes, linked to operations being list or not, we can use an added trailing slash on the URL path to separate list operations in the OpenAPI spec. This PR implements that, and then fixes an operation ID which becomes duplicated, with this change applied. See also hashicorp/vault-client-go#174, a bug which will be fixed by this work.

maxb · 2023-07-10T17:22:12Z

The consequences of this PR on vault-client-go may be viewed at hashicorp/vault-client-go#190

maxb · 2023-07-10T17:23:00Z

sdk/framework/openapi.go

+	Sudo            bool               `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"`
+	Unauthenticated bool               `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"`
+	CreateSupported bool               `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"`
+	DisplayAttrs    *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`


Note to reviewer: DisplayNavigation removed as wholly unused.

🤔 If I understand correctly, removing a public field of a public struct in our SDK is a breaking change in terms of binary compatibility and so breaks SemVer and therefore Go mod's rules.

I don't know the ecosystem well enough to estimate whether this would actually impact any real downstream code, but for example it could cause downstream code to fail to build if they are referring to this field. In general in other HashiCorp public modules we are relatively conservative about this kind of thing.

I'd be happy to be advised by any one who knows the code better whether there is some exceptional circumstance that makes it seem OK in this case, but my instinct would be to not remove this and avoid the issue entirely. I certainly don't think it's worth making a v2 of the whole sdk module to change this but that would be the "correct" way to do it I think.

Am I missing some reason this is fine?

You are correct it is a compatibility break. (Though given that Go doesn't generally ship libraries as binaries, but as source code, it's more a source compatibility issue than a binary one.)

However, the version of the SDK is v0.x.x, and the Go versioning rules explicitly declare that version zero is way of declaring a module has yet to implement any compatibility guarantees as it is still in development. Therefore, by keeping the SDK version at v0.x.x, the implicit message is that rights are reserved to break compatibility as desired.

(In contrast, the API package is v1.x.x implying compatibility will be maintained.)

I'd need to do a quick bit of research to confirm, but I think the SDK's lack of compatibility guarantees has been exercised in the past - e.g. in recent changes to decouple the Vault version number from the SDK.

Practically speaking, the field is located in a type which is only used to perform JSON serialization of OpenAPI specifications, internally to the SDK. It is very likely that it has zero uses in the wider ecosystem.

Therefore whilst it is a compatibility break, there are both technical and pragmatic reasons to believe that it is fine to do so.

However it is only a bit of cleanup in passing, and I would be happy to remove that change from this PR, if you prefer it that way.

I'll defer to @averche on whether it's preferable to leave it alone but I'd not noticed the v0.* tag which I think tips the whole thing towards being defensible in my mind 😄. Thanks for helping me understand Max!

I agree with @maxb on the rationale behind breaking the backwards compatibility and I think it's OK to remove this 👍

Just to indulge my curiosity about the compatibility issue, I ran previous versions of the SDK through an API diff tool:

Incompatible changes between v0.2.0 and v0.2.1

./framework.TypeBool: value changed from 3 to 4

./framework.TypeCommaIntSlice: value changed from 13 to 14

./framework.TypeCommaStringSlice: value changed from 9 to 10

./framework.TypeDurationSecond: value changed from 5 to 6

./framework.TypeFloat: value changed from 15 to 16

./framework.TypeHeader: value changed from 14 to 15

./framework.TypeKVPairs: value changed from 12 to 13

./framework.TypeLowerCaseString: value changed from 10 to 11

./framework.TypeMap: value changed from 4 to 5

./framework.TypeNameString: value changed from 11 to 12

./framework.TypeSignedDurationSecond: value changed from 6 to 7

./framework.TypeSlice: value changed from 7 to 8

./framework.TypeStringSlice: value changed from 8 to 9

./framework.TypeTime: value changed from 16 to 17

Incompatible changes between v0.2.1 and v0.3.0

package github.com/hashicorp/vault/sdk/testing/stepwise: removed

package github.com/hashicorp/vault/sdk/testing/stepwise/environments/docker: removed

package github.com/hashicorp/vault/sdk/helper/awsutil: removed

./database/dbplugin.DatabaseServer.mustEmbedUnimplementedDatabaseServer: added unexported method

./database/dbplugin.RegisterDatabaseServer: changed from func(*google.golang.org/grpc.Server, DatabaseServer) to func(google.golang.org/grpc.ServiceRegistrar, DatabaseServer)

./database/dbplugin/v5/proto.DatabaseServer.mustEmbedUnimplementedDatabaseServer: added unexported method

./database/dbplugin/v5/proto.RegisterDatabaseServer: changed from func(*google.golang.org/grpc.Server, DatabaseServer) to func(google.golang.org/grpc.ServiceRegistrar, DatabaseServer)

./logical.HTTPRawCacheControl: removed

./plugin/pb.BackendServer.mustEmbedUnimplementedBackendServer: added unexported method

./plugin/pb.ProtoConnectionToLogicalConnection: changed from func(*Connection) *github.com/hashicorp/vault/sdk/logical.Connection to func(*Connection) (*github.com/hashicorp/vault/sdk/logical.Connection, error)

./plugin/pb.RegisterBackendServer: changed from func(*google.golang.org/grpc.Server, BackendServer) to func(google.golang.org/grpc.ServiceRegistrar, BackendServer)

./plugin/pb.RegisterStorageServer: changed from func(*google.golang.org/grpc.Server, StorageServer) to func(google.golang.org/grpc.ServiceRegistrar, StorageServer)

./plugin/pb.RegisterSystemViewServer: changed from func(*google.golang.org/grpc.Server, SystemViewServer) to func(google.golang.org/grpc.ServiceRegistrar, SystemViewServer)

./plugin/pb.StorageServer.mustEmbedUnimplementedStorageServer: added unexported method

./plugin/pb.SystemViewServer.mustEmbedUnimplementedSystemViewServer: added unexported method

Incompatible changes between v0.3.0 and v0.4.0

./helper/pluginutil.RunnerUtil.NewPluginClient: added

./database/dbplugin/v5.DatabasePluginClient.Mutex: removed

./database/dbplugin/v5.GRPCDatabasePlugin: old is comparable, new is not

./database/dbplugin/v5.NewPluginClient: changed from func(context.Context, github.com/hashicorp/vault/sdk/helper/pluginutil.RunnerUtil, *github.com/hashicorp/vault/sdk/helper/pluginutil.PluginRunner, github.com/hashicorp/go-hclog.Logger, bool) (Database, error) to func(context.Context, github.com/hashicorp/vault/sdk/helper/pluginutil.RunnerUtil, github.com/hashicorp/vault/sdk/helper/pluginutil.PluginClientConfig) (Database, error)

sync.(*Mutex).Lock, method set of *DatabasePluginClient: removed

sync.(*Mutex).TryLock, method set of *DatabasePluginClient: removed

sync.(*Mutex).Unlock, method set of *DatabasePluginClient: removed

./logical.SystemView.NewPluginClient: added

./helper/certutil.ValidateSignatureLength: changed from func(int) error to func(string, int) error

Incompatible changes between v0.4.0 and v0.4.1

Incompatible changes between v0.4.1 and v0.5.0

./helper/useragent.PluginString: changed from func(*github.com/hashicorp/vault/sdk/logical.PluginEnvironment, string) string to func(*github.com/hashicorp/vault/sdk/logical.PluginEnvironment, string, ...string) string

./helper/useragent.String: changed from func() string to func(...string) string

Incompatible changes between v0.5.0 and v0.5.1

Incompatible changes between v0.5.1 and v0.5.2

./helper/keysutil.ParsePKCS8Ed25519PrivateKey: removed

Incompatible changes between v0.5.2 and v0.5.3

Incompatible changes between v0.5.3 and v0.6.0

./helper/pluginutil.Looker.LookupPluginVersion: added

./helper/pluginutil.MultiplexingSupported: changed from func(context.Context, google.golang.org/grpc.ClientConnInterface) (bool, error) to func(context.Context, google.golang.org/grpc.ClientConnInterface, string) (bool, error)

./helper/pluginutil.PluginCACertPEMEnv: changed from var to const

./helper/pluginutil.PluginClient.Reload: added

./helper/pluginutil.PluginMetadataModeEnv: changed from var to const

./helper/pluginutil.PluginMlockEnabled: changed from var to const

./helper/pluginutil.PluginUnwrapTokenEnv: changed from var to const

./helper/pluginutil.PluginVaultVersionEnv: changed from var to const

./plugin.BackendPluginClient.Mutex: removed

sync.(*Mutex).Lock, method set of *BackendPluginClient: removed

sync.(*Mutex).TryLock, method set of *BackendPluginClient: removed

sync.(*Mutex).Unlock, method set of *BackendPluginClient: removed

./logical.ManagedKeySystemView.WithManagedEncryptingKeyByName: added

./logical.ManagedKeySystemView.WithManagedEncryptingKeyByUUID: added

./logical.SystemView.ListVersionedPlugins: added

./logical.SystemView.LookupPluginVersion: added

./database/dbplugin.PluginFactory: changed from func(context.Context, string, github.com/hashicorp/vault/sdk/helper/pluginutil.LookRunnerUtil, github.com/hashicorp/go-hclog.Logger) (Database, error) to func(context.Context, string, string, github.com/hashicorp/vault/sdk/helper/pluginutil.LookRunnerUtil, github.com/hashicorp/go-hclog.Logger) (Database, error)

Incompatible changes between v0.6.0 and v0.6.1

./helper/keysutil.(*Policy).EncryptWithFactory: changed from func(int, []byte, []byte, string, AEADFactory) (string, error) to func(int, []byte, []byte, string, ...interface{}) (string, error)

Incompatible changes between v0.6.1 and v0.6.2

./helper/consts.VaultAllowPendingRemovalMountsEnv: removed

./framework.NewOASDocument: changed from func() *OASDocument to func(string) *OASDocument

./framework.Response: old is comparable, new is not

./logical.ManagedKeySystemView.WithManagedMACKeyByName: added

./logical.ManagedKeySystemView.WithManagedMACKeyByUUID: added

./logical.StaticSystemView.VaultVersion: removed

./logical.SystemView.VaultVersion: added

./helper/pluginutil.RunnerUtil.VaultVersion: added

Incompatible changes between v0.6.2 and v0.7.0

Incompatible changes between v0.7.0 and v0.8.0

./helper/keysutil.Policy.ManagedKeyName: removed

./logical.ManagedEncryptingKey.Decrypt: added

./logical.ManagedEncryptingKey.Encrypt: added

./logical.ManagedEncryptingKey.GetAEAD: removed

./logical.ManagedKeyRandomSource.GetRandomBytes: changed from func(context.Context, int) ([]byte, error) to func(int) ([]byte, error)

./logical.SystemView.ClusterID: added

./plugin/pb.SystemViewClient.ClusterInfo: added

package github.com/hashicorp/vault/sdk/version: removed

Incompatible changes between v0.8.0 and v0.8.1

./logical.(*EventData).GetMetadata: changed from func() []byte to func() *google.golang.org/protobuf/types/known/structpb.Struct

./logical.EventData.Metadata: changed from []byte to *google.golang.org/protobuf/types/known/structpb.Struct

Incompatible changes between v0.8.1 and v0.9.0

./logical.(*EventData).ID: removed

./logical.(*EventReceived).GetTimestamp: removed

./logical.EventReceived.Timestamp: removed

Incompatible changes between v0.9.0 and v0.9.1

./logical.Externaler: removed

./helper/ldaputil.LDAP.Dial: removed

./helper/ldaputil.LDAP.DialTLS: removed

./helper/ldaputil.LDAP.DialURL: added

./plugin.(*BackendPluginClient).IsExternal: removed

Incompatible changes between v0.9.1 and HEAD

./framework.OASPathItem.DisplayNavigation: removed

./helper/testcluster.WaitForReplicationStatus: changed from func(context.Context, *github.com/hashicorp/vault/api.Client, bool, func(map[string]interface{}) bool) error to func(context.Context, *github.com/hashicorp/vault/api.Client, bool, func(map[string]interface{}) error) error

It turns out the supposedly compatible API has fudged things a bit in the past, too:

Incompatible changes between v1.0.4 and v1.1.0

(*Sys).ConfigureCORS: changed from func(*CORSRequest) (*CORSResponse, error) to func(*CORSRequest) error

(*Sys).DisableCORS: changed from func() (*CORSResponse, error) to func() error

CORSRequest.AllowedOrigins: changed from string to []string

CORSRequest: old is comparable, new is not

CORSResponse.AllowedOrigins: changed from string to []string

CORSResponse: old is comparable, new is not

Incompatible changes between v1.5.0 and v1.6.0

(*OutputStringError).CurlString: changed from func() string to func() (string, error)

(*Sys).Monitor: changed from func(context.Context, string) (chan string, error) to func(context.Context, string, string) (chan string, error)

AutopilotServer.Meta: removed

TLSConfig: old is comparable, new is not

Incompatible changes between v1.7.2 and v1.8.0

PluginMetadataModeEnv: changed from var to const

PluginUnwrapTokenEnv: changed from var to const

Incompatible changes between v1.8.0 and v1.8.1

MountInput.PluginVersion: removed

Incompatible changes between v1.8.2 and v1.8.3

SealStatusResponse: old is comparable, new is not

Incompatible changes between v1.9.0 and v1.9.1

OutputPolicyError: old is comparable, new is not

To mask out more duplicate read/list functionality, now being separately generated to OpenAPI client libraries as a result of this change.

averche

This looks great and will fix the KVv1/KVv2 List issues in the generated libraries, which is fantastic!

I have a few nitty comments, otherwise LGTM.

sdk/framework/openapi.go

vault/logical_system_paths.go

builtin/credential/github/backend.go

I'm not totally convinced its worth the extra lines of code, but equally, I don't have strong feelings about it, so I'll just make the change.

…ith doubled final slashes Even in the edge case of improper use of regex patterns and operations.

averche

Thanks again for the fixes and the detailed explanation!

Before merging, It looks like some of the OpenAPI-related and external tests (e.g. TestProperAuthing, TestSudoPaths) are currently failing. I'm guessing the tests expect the old OpenAPI output and need to be adjusted accordingly. Could you please take a look.

maxb · 2023-07-11T21:13:59Z

Before merging, It looks like some of the OpenAPI-related and external tests (e.g. TestProperAuthing, TestSudoPaths) are currently failing.

Whoops! Unfortunately there are multiple checks that always fail on community PRs, so I've got used to ignoring the red crosses :-/

Once I looked hard at it, I found it was missing several sudo paths, which led to additional bug fixing elsewhere. I might need to pull some parts of this change out into a separate PR for ease of review...

… in OpenAPI, at least within this PR Just add TODO comments for now.

averche

This LGTM, thanks again for putting it together! We might want to hold off on merging this until after #21772 is merged (to get rid of the TODOs).

maxb · 2023-07-13T06:25:43Z

It's your call, but personally I'd really prefer that this be merged now: yes, this PR adds some TODO comments, but only documenting issues that already exist on the main branch - not things that are on-topic to fix in this PR. If I'd just declined to mention the missing sudo paths at all, this PR would have no interaction with the other PR at all.

There are going to be merge conflicts between my open PRs for sure, due to Go code formatting rearranging indentation, so merging this one now gives me an opportunity to resolve those - you won't be able to merge both PRs in quick succession.

If the problem is an internal policy on not adding TODO comments to the codebase, I can just delete the TODO comments from this PR entirely - you can see my other PR already tracks fixing the issue.

maxb · 2023-07-13T07:22:43Z

Also it would be nice to not tie the progression of this PR to finding someone happy to sign off on the fixes to the somewhat-obscure sudo code in the other PR.

cipherboy

From a cryptosec perspective, this looks good. :-) Thanks @maxb !

maxb mentioned this pull request Jul 10, 2023

Preview results of Vault PR 21723 hashicorp/vault-client-go#190

Closed

maxb commented Jul 10, 2023

View reviewed changes

maxb mentioned this pull request Jul 10, 2023

list of subkeys with KvV2ReadMetadata is not working hashicorp/vault-client-go#174

Closed

Set further DisplayAttrs in auth/github plugin

408de42

To mask out more duplicate read/list functionality, now being separately generated to OpenAPI client libraries as a result of this change.

maxb requested a review from a team July 10, 2023 17:49

averche self-requested a review July 10, 2023 19:17

averche reviewed Jul 10, 2023

View reviewed changes

sdk/framework/openapi.go Outdated Show resolved Hide resolved

vault/logical_system_paths.go Outdated Show resolved Hide resolved

builtin/credential/github/backend.go Show resolved Hide resolved

builtin/credential/github/backend.go Show resolved Hide resolved

maxb added 3 commits July 11, 2023 07:33

Apply requested changes to operation IDs

1185ef8

I'm not totally convinced its worth the extra lines of code, but equally, I don't have strong feelings about it, so I'll just make the change.

Adjust logic to prevent any possibility of generating OpenAPI paths w…

898bc36

…ith doubled final slashes Even in the edge case of improper use of regex patterns and operations.

changelog

aeb4289

averche reviewed Jul 11, 2023

View reviewed changes

averche added the core/openapi label Jul 11, 2023

averche added this to the 1.15 milestone Jul 11, 2023

averche added backport/1.13.x labels Jul 11, 2023

Merge branch 'main' into full-openapi-list-separation

6bfe9e8

maxb added 2 commits July 11, 2023 23:23

Fix TestSudoPaths to pass again... which snowballed a bit...

d037477

Once I looked hard at it, I found it was missing several sudo paths, which led to additional bug fixing elsewhere. I might need to pull some parts of this change out into a separate PR for ease of review...

Fix other tests

01cb30b

maxb requested a review from a team July 11, 2023 22:48

maxb added 2 commits July 12, 2023 00:17

More test fixing

1d4680a

Undo scope creep - back away from fixing sudo paths not shown as such…

51f7a03

… in OpenAPI, at least within this PR Just add TODO comments for now.

averche approved these changes Jul 12, 2023

View reviewed changes

cipherboy approved these changes Jul 13, 2023

View reviewed changes

averche merged commit e47dd9d into hashicorp:main Jul 13, 2023
86 of 88 checks passed

maxb deleted the full-openapi-list-separation branch July 13, 2023 17:49

Monkeychip mentioned this pull request Oct 2, 2023

Bug fix for double slash in LIST endpoint #23446

Merged

hc-github-team-secure-vault-core mentioned this pull request Oct 2, 2023

Backport of Bug fix for double slash in LIST endpoint into release/1.15.x #23450

Merged

hashishaw mentioned this pull request Nov 15, 2023

Web Interface API vs Web inteface CLI list differences #7970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenAPI: Separate ListOperation from ReadOperation #21723

OpenAPI: Separate ListOperation from ReadOperation #21723

maxb commented Jul 10, 2023 •

edited

Loading

maxb commented Jul 10, 2023

maxb Jul 10, 2023

banks Jul 13, 2023 •

edited

Loading

maxb Jul 13, 2023 •

edited

Loading

banks Jul 13, 2023

averche Jul 13, 2023

maxb Jul 14, 2023 •

edited

Loading

maxb Jul 14, 2023 •

edited

Loading

averche left a comment

averche left a comment •

edited

Loading

maxb commented Jul 11, 2023

averche left a comment

maxb commented Jul 13, 2023

maxb commented Jul 13, 2023

cipherboy left a comment

OpenAPI: Separate ListOperation from ReadOperation #21723

OpenAPI: Separate ListOperation from ReadOperation #21723

Conversation

maxb commented Jul 10, 2023 • edited Loading

maxb commented Jul 10, 2023

maxb Jul 10, 2023

Choose a reason for hiding this comment

banks Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

maxb Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

banks Jul 13, 2023

Choose a reason for hiding this comment

averche Jul 13, 2023

Choose a reason for hiding this comment

maxb Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Incompatible changes between v0.2.0 and v0.2.1

Incompatible changes between v0.2.1 and v0.3.0

Incompatible changes between v0.3.0 and v0.4.0

Incompatible changes between v0.4.0 and v0.4.1

Incompatible changes between v0.4.1 and v0.5.0

Incompatible changes between v0.5.0 and v0.5.1

Incompatible changes between v0.5.1 and v0.5.2

Incompatible changes between v0.5.2 and v0.5.3

Incompatible changes between v0.5.3 and v0.6.0

Incompatible changes between v0.6.0 and v0.6.1

Incompatible changes between v0.6.1 and v0.6.2

Incompatible changes between v0.6.2 and v0.7.0

Incompatible changes between v0.7.0 and v0.8.0

Incompatible changes between v0.8.0 and v0.8.1

Incompatible changes between v0.8.1 and v0.9.0

Incompatible changes between v0.9.0 and v0.9.1

Incompatible changes between v0.9.1 and HEAD

maxb Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Incompatible changes between v1.0.4 and v1.1.0

Incompatible changes between v1.5.0 and v1.6.0

Incompatible changes between v1.7.2 and v1.8.0

Incompatible changes between v1.8.0 and v1.8.1

Incompatible changes between v1.8.2 and v1.8.3

Incompatible changes between v1.9.0 and v1.9.1

averche left a comment

Choose a reason for hiding this comment

averche left a comment • edited Loading

Choose a reason for hiding this comment

maxb commented Jul 11, 2023

averche left a comment

Choose a reason for hiding this comment

maxb commented Jul 13, 2023

maxb commented Jul 13, 2023

cipherboy left a comment

Choose a reason for hiding this comment

maxb commented Jul 10, 2023 •

edited

Loading

banks Jul 13, 2023 •

edited

Loading

maxb Jul 13, 2023 •

edited

Loading

maxb Jul 14, 2023 •

edited

Loading

maxb Jul 14, 2023 •

edited

Loading

averche left a comment •

edited

Loading