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.

Sign up for GitHub

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

OpenAPI: Separate ListOperation from ReadOperation #21723

Merged
merged 10 commits into from
Jul 13, 2023
Merged

OpenAPI: Separate ListOperation from ReadOperation #21723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e6718ff
OpenAPI: Separate ListOperation from ReadOperation
maxb Jul 10, 2023
408de42
Set further DisplayAttrs in auth/github plugin
maxb Jul 10, 2023
1185ef8
Apply requested changes to operation IDs
maxb Jul 11, 2023
898bc36
Adjust logic to prevent any possibility of generating OpenAPI paths w…
maxb Jul 11, 2023
aeb4289
changelog
maxb Jul 11, 2023
6bfe9e8
Merge branch 'main' into full-openapi-list-separation
maxb Jul 11, 2023
d037477
Fix TestSudoPaths to pass again... which snowballed a bit...
maxb Jul 11, 2023
01cb30b
Fix other tests
maxb Jul 11, 2023
1d4680a
More test fixing
maxb Jul 11, 2023
51f7a03
Undo scope creep - back away from fixing sudo paths not shown as such…
maxb Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 24 additions & 16 deletions api/plugin_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ const (
// path matches that path or not (useful specifically for the paths that
// contain templated fields.)
var sudoPaths = map[string]*regexp.Regexp{
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
"/auth/token/accessors": regexp.MustCompile(`^/auth/token/accessors/?$`),
// TODO /auth/token/revoke-orphan requires sudo but isn't represented as such in the OpenAPI spec
"/pki/root": regexp.MustCompile(`^/pki/root$`),
"/pki/root/sign-self-issued": regexp.MustCompile(`^/pki/root/sign-self-issued$`),
"/sys/audit": regexp.MustCompile(`^/sys/audit$`),
Expand All @@ -52,28 +53,33 @@ var sudoPaths = map[string]*regexp.Regexp{
"/sys/config/cors": regexp.MustCompile(`^/sys/config/cors$`),
"/sys/config/ui/headers": regexp.MustCompile(`^/sys/config/ui/headers/?$`),
"/sys/config/ui/headers/{header}": regexp.MustCompile(`^/sys/config/ui/headers/.+$`),
"/sys/leases": regexp.MustCompile(`^/sys/leases$`),
"/sys/leases/lookup/": regexp.MustCompile(`^/sys/leases/lookup/?$`),
"/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`),
"/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`),
"/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`),
"/sys/plugins/catalog/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[^/]+$`),
"/sys/plugins/catalog/{type}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+$`),
"/sys/plugins/catalog/{type}/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+/[^/]+$`),
"/sys/raw": regexp.MustCompile(`^/sys/raw$`),
"/sys/raw/{path}": regexp.MustCompile(`^/sys/raw/.+$`),
"/sys/remount": regexp.MustCompile(`^/sys/remount$`),
"/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`),
"/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`),
"/sys/rotate": regexp.MustCompile(`^/sys/rotate$`),
"/sys/internal/inspect/router/{tag}": regexp.MustCompile(`^/sys/internal/inspect/router/.+$`),
"/sys/leases": regexp.MustCompile(`^/sys/leases$`),
// This entry is a bit wrong... sys/leases/lookup does NOT require sudo. But sys/leases/lookup/ with a trailing
// slash DOES require sudo. But the part of the Vault CLI that uses this logic doesn't pass operation-appropriate
// trailing slashes, it always strips them off, so we end up giving the wrong answer for one of these.
"/sys/leases/lookup": regexp.MustCompile(`^/sys/leases/lookup/?$`),
"/sys/leases/lookup/{prefix}": regexp.MustCompile(`^/sys/leases/lookup/.+$`),
"/sys/leases/revoke-force/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-force/.+$`),
"/sys/leases/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/leases/revoke-prefix/.+$`),
"/sys/plugins/catalog/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[^/]+$`),
"/sys/plugins/catalog/{type}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+$`),
"/sys/plugins/catalog/{type}/{name}": regexp.MustCompile(`^/sys/plugins/catalog/[\w-]+/[^/]+$`),
"/sys/raw": regexp.MustCompile(`^/sys/raw$`),
"/sys/raw/{path}": regexp.MustCompile(`^/sys/raw/.+$`),
"/sys/remount": regexp.MustCompile(`^/sys/remount$`),
"/sys/revoke-force/{prefix}": regexp.MustCompile(`^/sys/revoke-force/.+$`),
"/sys/revoke-prefix/{prefix}": regexp.MustCompile(`^/sys/revoke-prefix/.+$`),
"/sys/rotate": regexp.MustCompile(`^/sys/rotate$`),
// TODO /sys/seal requires sudo but isn't represented as such in the OpenAPI spec
// TODO /sys/step-down requires sudo but isn't represented as such in the OpenAPI spec

// enterprise-only paths
"/sys/replication/dr/primary/secondary-token": regexp.MustCompile(`^/sys/replication/dr/primary/secondary-token$`),
"/sys/replication/performance/primary/secondary-token": regexp.MustCompile(`^/sys/replication/performance/primary/secondary-token$`),
"/sys/replication/primary/secondary-token": regexp.MustCompile(`^/sys/replication/primary/secondary-token$`),
"/sys/replication/reindex": regexp.MustCompile(`^/sys/replication/reindex$`),
"/sys/storage/raft/snapshot-auto/config/": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`),
"/sys/storage/raft/snapshot-auto/config": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/?$`),
"/sys/storage/raft/snapshot-auto/config/{name}": regexp.MustCompile(`^/sys/storage/raft/snapshot-auto/config/[^/]+$`),
}

Expand Down Expand Up @@ -252,6 +258,8 @@ func SudoPaths() map[string]*regexp.Regexp {
// Determine whether the given path requires the sudo capability.
// Note that this uses hardcoded static path information, so will return incorrect results for paths in namespaces,
// or for secret engines mounted at non-default paths.
// Expects to receive a path with an initial slash, but no trailing slashes, as the Vault CLI (the only known and
// expected user of this function) sanitizes its paths that way.
func IsSudoPath(path string) bool {
// Return early if the path is any of the non-templated sudo paths.
if _, ok := sudoPaths[path]; ok {
Expand Down
32 changes: 31 additions & 1 deletion builtin/credential/github/backend.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"net/url"

"github.com/google/go-github/github"
cleanhttp "github.com/hashicorp/go-cleanhttp"
"github.com/hashicorp/go-cleanhttp"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/logical"
"golang.org/x/oauth2"
Expand Down Expand Up @@ -43,6 +43,21 @@ func Backend() *backend {
OperationPrefix: operationPrefixGithub,
OperationSuffix: "team-mapping",
}
teamMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{
logical.ListOperation: &framework.PathOperation{
Callback: teamMapPaths[0].Callbacks[logical.ListOperation],
Summary: teamMapPaths[0].HelpSynopsis,
},
logical.ReadOperation: &framework.PathOperation{
Callback: teamMapPaths[0].Callbacks[logical.ReadOperation],
Summary: teamMapPaths[0].HelpSynopsis,
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "list",
OperationSuffix: "teams2", // The ReadOperation is redundant with the ListOperation
},
},
}
teamMapPaths[0].Callbacks = nil

b.UserMap = &framework.PolicyMap{
PathMap: framework.PathMap{
Expand All @@ -61,6 +76,21 @@ func Backend() *backend {
OperationPrefix: operationPrefixGithub,
OperationSuffix: "user-mapping",
}
userMapPaths[0].Operations = map[logical.Operation]framework.OperationHandler{
logical.ListOperation: &framework.PathOperation{
Callback: userMapPaths[0].Callbacks[logical.ListOperation],
Summary: userMapPaths[0].HelpSynopsis,
},
logical.ReadOperation: &framework.PathOperation{
Callback: userMapPaths[0].Callbacks[logical.ReadOperation],
Summary: userMapPaths[0].HelpSynopsis,
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "list",
OperationSuffix: "users2", // The ReadOperation is redundant with the ListOperation
},
},
}
userMapPaths[0].Callbacks = nil

allPaths := append(teamMapPaths, userMapPaths...)
b.Backend = &framework.Backend{
Expand Down
15 changes: 8 additions & 7 deletions builtin/logical/pki/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6767,8 +6767,8 @@ func TestProperAuthing(t *testing.T) {
"cert/unified-delta-crl": shouldBeUnauthedReadList,
"cert/unified-delta-crl/raw": shouldBeUnauthedReadList,
"cert/unified-delta-crl/raw/pem": shouldBeUnauthedReadList,
"certs": shouldBeAuthed,
"certs/revoked": shouldBeAuthed,
"certs/": shouldBeAuthed,
"certs/revoked/": shouldBeAuthed,
"certs/revocation-queue": shouldBeAuthed,
"certs/unified-revoked": shouldBeAuthed,
"config/acme": shouldBeAuthed,
Expand Down Expand Up @@ -6817,7 +6817,7 @@ func TestProperAuthing(t *testing.T) {
"issuer/default/sign-verbatim": shouldBeAuthed,
"issuer/default/sign-verbatim/test": shouldBeAuthed,
"issuer/default/sign/test": shouldBeAuthed,
"issuers": shouldBeUnauthedReadList,
"issuers/": shouldBeUnauthedReadList,
"issuers/generate/intermediate/exported": shouldBeAuthed,
"issuers/generate/intermediate/internal": shouldBeAuthed,
"issuers/generate/intermediate/existing": shouldBeAuthed,
Expand All @@ -6829,7 +6829,7 @@ func TestProperAuthing(t *testing.T) {
"issuers/import/cert": shouldBeAuthed,
"issuers/import/bundle": shouldBeAuthed,
"key/default": shouldBeAuthed,
"keys": shouldBeAuthed,
"keys/": shouldBeAuthed,
"keys/generate/internal": shouldBeAuthed,
"keys/generate/exported": shouldBeAuthed,
"keys/generate/kms": shouldBeAuthed,
Expand All @@ -6839,7 +6839,7 @@ func TestProperAuthing(t *testing.T) {
"revoke": shouldBeAuthed,
"revoke-with-key": shouldBeAuthed,
"roles/test": shouldBeAuthed,
"roles": shouldBeAuthed,
"roles/": shouldBeAuthed,
"root": shouldBeAuthed,
"root/generate/exported": shouldBeAuthed,
"root/generate/internal": shouldBeAuthed,
Expand All @@ -6864,7 +6864,7 @@ func TestProperAuthing(t *testing.T) {
"unified-crl/delta/pem": shouldBeUnauthedReadList,
"unified-ocsp": shouldBeUnauthedWriteOnly,
"unified-ocsp/dGVzdAo=": shouldBeUnauthedReadList,
"eab": shouldBeAuthed,
"eab/": shouldBeAuthed,
"eab/" + eabKid: shouldBeAuthed,
}

Expand Down Expand Up @@ -6953,7 +6953,8 @@ func TestProperAuthing(t *testing.T) {

handler, present := paths[raw_path]
if !present {
t.Fatalf("OpenAPI reports PKI mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path)
t.Fatalf("OpenAPI reports PKI mount contains %v -> %v but was not tested to be authed or not authed.",
openapi_path, raw_path)
}

openapi_data := raw_data.(map[string]interface{})
Expand Down
5 changes: 3 additions & 2 deletions builtin/logical/ssh/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2765,7 +2765,7 @@ func TestProperAuthing(t *testing.T) {
"public_key": shouldBeUnauthedReadList,
"roles/test-ca": shouldBeAuthed,
"roles/test-otp": shouldBeAuthed,
"roles": shouldBeAuthed,
"roles/": shouldBeAuthed,
"sign/test-ca": shouldBeAuthed,
"tidy/dynamic-keys": shouldBeAuthed,
"verify": shouldBeUnauthedWriteOnly,
Expand Down Expand Up @@ -2809,7 +2809,8 @@ func TestProperAuthing(t *testing.T) {

handler, present := paths[raw_path]
if !present {
t.Fatalf("OpenAPI reports SSH mount contains %v->%v but was not tested to be authed or authed.", openapi_path, raw_path)
t.Fatalf("OpenAPI reports SSH mount contains %v -> %v but was not tested to be authed or not authed.",
openapi_path, raw_path)
}

openapi_data := raw_data.(map[string]interface{})
Expand Down
3 changes: 3 additions & 0 deletions changelog/21723.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
openapi: List operations are now given first-class representation in the OpenAPI document, rather than sometimes being overlaid with a read operation at the same path
```
103 changes: 76 additions & 27 deletions sdk/framework/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,12 @@ type OASLicense struct {
}

type OASPathItem struct {
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
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"`
DisplayNavigation bool `json:"x-vault-displayNavigation,omitempty" mapstructure:"x-vault-displayNavigation"`
DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
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"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewer: DisplayNavigation removed as wholly unused.

Copy link
Member

@banks banks Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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?

Copy link
Contributor Author

@maxb maxb Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maxb maxb Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maxb maxb Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Get *OASOperation `json:"get,omitempty"`
Post *OASOperation `json:"post,omitempty"`
Expand Down Expand Up @@ -309,6 +308,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *

// Process each supported operation by building up an Operation object
// with descriptions, properties and examples from the framework.Path data.
var listOperation *OASOperation
for opType, opHandler := range operations {
props := opHandler.Properties()
if props.Unpublished || forceUnpublished {
Expand All @@ -324,11 +324,6 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// If both List and Read are defined, only process Read.
if opType == logical.ListOperation && operations[logical.ReadOperation] != nil {
continue
}

op := NewOASOperation()

operationID := constructOperationID(
Expand Down Expand Up @@ -408,24 +403,17 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// LIST is represented as GET with a `list` query parameter.
// LIST is represented as GET with a `list` query parameter. Code later on in this function will assign
// list operations to a path with an extra trailing slash, ensuring they do not collide with read
// operations.
if opType == logical.ListOperation {
// Only accepts List (due to the above skipping of ListOperations that also have ReadOperations)
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Must be set to `true`",
Required: true,
In: "query",
Schema: &OASSchema{Type: "string", Enum: []interface{}{"true"}},
})
} else if opType == logical.ReadOperation && operations[logical.ListOperation] != nil {
// Accepts both Read and List
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Return a list if `true`",
In: "query",
Schema: &OASSchema{Type: "string"},
})
}

// Add tags based on backend type
Expand Down Expand Up @@ -521,18 +509,79 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
switch opType {
case logical.CreateOperation, logical.UpdateOperation:
pi.Post = op
case logical.ReadOperation, logical.ListOperation:
case logical.ReadOperation:
pi.Get = op
case logical.DeleteOperation:
pi.Delete = op
case logical.ListOperation:
listOperation = op
}
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn("OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins", "path", openAPIPath)
// The conventions enforced by the Vault HTTP routing code make it impossible to match a path with a trailing
// slash to anything other than a ListOperation. Catch mistakes in path definition, to enforce that if both of
// the two following blocks of code (non-list, and list) write an OpenAPI path to the output document, then the
// first one will definitely not have a trailing slash.
originalPathHasTrailingSlash := strings.HasSuffix(path, "/")
if originalPathHasTrailingSlash && (pi.Get != nil || pi.Post != nil || pi.Delete != nil) {
backend.Logger().Warn(
"OpenAPI spec generation: discarding impossible-to-invoke non-list operations from path with "+
"required trailing slash; this is a bug in the backend code", "path", path)
pi.Get = nil
pi.Post = nil
pi.Delete = nil
}

// Write the regular, non-list, OpenAPI path to the OpenAPI document, UNLESS we generated a ListOperation, and
// NO OTHER operation types. In that fairly common case (there are lots of list-only endpoints), we avoid
// writing a redundant OpenAPI path for (e.g.) "auth/token/accessors" with no operations, only to then write
// one for "auth/token/accessors/" immediately below.
//
// On the other hand, we do still write the OpenAPI path here if we generated ZERO operation types - this serves
// to provide documentation to a human that an endpoint exists, even if it has no invokable OpenAPI operations.
// Examples of this include kv-v2's ".*" endpoint (regex cannot be translated to OpenAPI parameters), and the
// auth/oci/login endpoint (implements ResolveRoleOperation only, only callable from inside Vault).
if listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil {
openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &pi
}

// If there is a ListOperation, write it to a separate OpenAPI path in the document.
if listOperation != nil {
// Append a slash here to disambiguate from the path written immediately above.
// However, if the path already contains a trailing slash, we want to avoid doubling it, and it is
// guaranteed (through the interaction of logic in the last two blocks) that the block immediately above
// will NOT have written a path to the OpenAPI document.
if !originalPathHasTrailingSlash {
path += "/"
}

listPathItem := OASPathItem{
Description: pi.Description,
Parameters: pi.Parameters,
DisplayAttrs: pi.DisplayAttrs,

// Since the path may now have an extra slash on the end, we need to recalculate the special path
// matches, as the sudo or unauthenticated status may be changed as a result!
Sudo: specialPathMatch(path, sudoPaths),
Unauthenticated: specialPathMatch(path, unauthPaths),

Get: listOperation,
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &listPathItem
}
doc.Paths[openAPIPath] = &pi
}

return nil
Expand Down
Loading