-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
events: Add full api_path
; rename Send
#22487
Conversation
This preserves the existing `path` entry in the metadata for backwards compatibility. The `secret_path` is the path to the secret in Vault. The `operation` is what operation is being performed, which may be plugin-dependent. This depends on hashicorp/vault#22487 being merged and bumped in `go.mod` before this will compile (since it references new constants in the SDK, though this could be removed).
Build Results: |
CI Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left a few comments but nothing urgent
vault/eventbus/bus.go
Outdated
data.Metadata.Fields[logical.EventMetadataSecretPath] == nil { | ||
return data | ||
} | ||
data.Metadata.Fields[logical.EventMetadataSecretPath] = structpb.NewStringValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't feel like it matters a huge amount either way, but is it maybe a safer/better pattern to append to the event rather than mutate it? e.g. if the plugin reports an event with relative_path
, we add secret_path
with the mount path in front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I am okay either way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not against it I am slightly inclined towards plugins setting relative_api_path
and then Vault using that to populate api_path
as it feels less confusing for plugin devs - that way the data is always accurate all the way through the pipeline. Whereas if we only use api_path
the plugin is kind of filling in a half-truth until it gets corrected inside Vault.
vault/eventbus/bus_test.go
Outdated
|
||
// TestSecretPathIsPrependedWithMount tests that "secret_path", if present in the | ||
// metadata, is prepended with the plugin's mount. | ||
func TestSecretPathIsPrependedWithMount(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
I just realised, in the PR description example, the secret_path doesn't look right: |
sdk/logical/events.go
Outdated
// common event metadata keys | ||
const ( | ||
EventMetadataSecretPath = "secret_path" | ||
EventMetadataOperation = "operation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to enforce some rules on this field because it should be tightly scoped to one of a few valid values. Could we define a type EventOperation string
and enumerate all the valid options for this? And then validate them as we receive events in the core's event bus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to leave metadata to be more free-form, so that plugins can define and use whatever fields they want. (Hence why we're using a generic structpb
and not a fixed protobuf message
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My motivating use-case for this comment was the RFC discussion where our consumers need to know whether the secret data has changed and they therefore need to re-fetch it. I don't think this field satisfies that requirement unless we have a schema that they can rely on for interpreting events. I'm open to other solutions too though; off the top of my head:
- Do we envisage emitting events for non-write events? If not, perhaps consumers should always assume underlying data change? Although I would worry a little bit about boxing ourselves in by committing to that implicit assumption.
- A different, more tightly scoped field name like a boolean "data_changed", or "updated", or "refetch" etc.?
@tomhjp re: the KV expects you to prepend This is one of the reasons I left I guess a better question would be, is the point of |
(Discussed with @tomhjp elsewhere, and it seems better to use |
secret_path
; rename Sendapi_path
; rename Send
8a3e88a
to
2a28102
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor ongoing discussion points, but still no blockers as I'm happy to follow up on those in a separate PR if preferred.
sdk/logical/events.go
Outdated
// EventMetadataApiPath is used in event metadata to show the API path that can be used to fetch any underlying | ||
// data. For example, the KV plugin would set this to `data/mysecret`. The event system will automatically prepend | ||
// the plugin mount to this path, if present, so it would be come `secret/data/mysecret`, for example. | ||
// If this is an auth plugin event, this will additionally be prepended with `auth/`. | ||
EventMetadataApiPath = "api_path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Go naming conventions
// EventMetadataApiPath is used in event metadata to show the API path that can be used to fetch any underlying | |
// data. For example, the KV plugin would set this to `data/mysecret`. The event system will automatically prepend | |
// the plugin mount to this path, if present, so it would be come `secret/data/mysecret`, for example. | |
// If this is an auth plugin event, this will additionally be prepended with `auth/`. | |
EventMetadataApiPath = "api_path" | |
// EventMetadataAPIPath is used in event metadata to show the API path that can be used to fetch any underlying | |
// data. For example, the KV plugin would set this to `data/mysecret`. The event system will automatically prepend | |
// the plugin mount to this path, if present, so it would be come `secret/data/mysecret`, for example. | |
// If this is an auth plugin event, this will additionally be prepended with `auth/`. | |
EventMetadataAPIPath = "api_path" |
sdk/logical/events.go
Outdated
if err != nil { | ||
return err | ||
} | ||
metadata := map[string]string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this change is in progress anyway as I saw the allocations comment was resolved: I think the same algorithm here will work with one less map allocated if we delete this map and use ev.Metadata.Fields
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, oops, let me copy that over.
vault/eventbus/bus.go
Outdated
data.Metadata.Fields[logical.EventMetadataSecretPath] == nil { | ||
return data | ||
} | ||
data.Metadata.Fields[logical.EventMetadataSecretPath] = structpb.NewStringValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not against it I am slightly inclined towards plugins setting relative_api_path
and then Vault using that to populate api_path
as it feels less confusing for plugin devs - that way the data is always accurate all the way through the pipeline. Whereas if we only use api_path
the plugin is kind of filling in a half-truth until it gets corrected inside Vault.
sdk/logical/events.go
Outdated
// common event metadata keys | ||
const ( | ||
EventMetadataSecretPath = "secret_path" | ||
EventMetadataOperation = "operation" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My motivating use-case for this comment was the RFC discussion where our consumers need to know whether the secret data has changed and they therefore need to re-fetch it. I don't think this field satisfies that requirement unless we have a schema that they can rely on for interpreting events. I'm open to other solutions too though; off the top of my head:
- Do we envisage emitting events for non-write events? If not, perhaps consumers should always assume underlying data change? Although I would worry a little bit about boxing ourselves in by committing to that implicit assumption.
- A different, more tightly scoped field name like a boolean "data_changed", or "updated", or "refetch" etc.?
sdk/logical/events.go
Outdated
// data. For example, the KV plugin would set this to `data/mysecret`. The event system will automatically prepend | ||
// the plugin mount to this path, if present, so it would be come `secret/data/mysecret`, for example. | ||
// If this is an auth plugin event, this will additionally be prepended with `auth/`. | ||
EventMetadataApiPath = "api_path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the extra comment, I've just done the same again and spent some time on the KV PR after reviewing this.
For an example like deleting a KV secret (POST delete/foo
), the ergonomics feel a little weird with this name. path
and api_path
are pretty synonymous from a plugin dev point of view, and whichever we use, it feels like delete/foo
is the least surprising thing to put there. How about we keep path
as-is for KV, and add data_path
for the path at which consumers can fetch the associated sensitive data?
EventMetadataApiPath = "api_path" | |
EventMetadataApiPath = "data_path" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me.
@tomhjp there are definitely non-write events we might generate in the future, like lease expirations. I think the operation might be good to keep, but we could add another field like |
a006c16
to
ebaf13a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Biggest change: we rename `Send` to `SendEvent` in `logical.EventSender`.. Initially we picked `Send` to match the underlying go-eventlogger broker's `Send` method, and to avoid the stuttering of `events.SendEvent`. However, I think it is more useful for the `logical.EventSender` interface to use the method `SendEvent` so that, for example, `framework.Backend` can implement it. This is a relatively change now that should not affect anything except the KV plugin, which is being fixed in another PR. Another change: if the `secret_path` metadata is present, then the plugin-aware `EventBus` will prepend it with the plugin mount. This allows the `secret_path` to be the full path to any referenced secret. This change is also backwards compatible, since this field was not present in the KV plugin. (It did use the slightly different `path` field, which we can keep for now.) Tested with a KV plugin modified to output the `secret_path` metadata: ``` 2023-08-21T15:39:47.384-0700 [INFO] events: Sending event: event="event :{id:\"5989b10e-3356-bcf9-5f41-88d2ef699de2\" metadata:{fields:{key:\"cu rrent_version\" value:{string_value:\"1\"}} fields:{key:\"oldest_version \" value:{string_value:\"0\"}} fields:{key:\"operation\" value:{string_v alue:\"data-write\"}} fields:{key:\"path\" value:{string_value:\"data/fo o2\"}} fields:{key:\"secret_path\" value:{string_value:\"secret/foo2\"}} }} event_type:\"kv-v2/data-write\" plugin_info:{mount_class:\"secret\" m ount_accessor:\"kv_f52ab3c4\" mount_path:\"secret/\" plugin:\"kv\"}" ```
ebaf13a
to
90560d3
Compare
Thanks! |
This preserves the existing `path` entry in the metadata for backwards compatibility. The `data_path` is the path to the secret in Vault. The `operation` is what operation is being performed, which may be plugin-dependent. This depends on hashicorp/vault#22487 being merged and bumped in `go.mod` before this will compile (since it references new constants in the SDK, though this could be removed).
Biggest change: we rename
Send
toSendEvent
inlogical.EventSender
.. Initially we pickedSend
to match the underlying go-eventlogger broker'sSend
method, and to avoid the stuttering ofevents.SendEvent
.However, I think it is more useful for the
logical.EventSender
interface to use the methodSendEvent
so that, for example,framework.Backend
can implement it.This is a relatively change now that should not affect anything except the KV plugin, which is being fixed in another PR.
Another change: if the
api_path
metadata is present, then the plugin-awareEventBus
will prepend it with the plugin mount. This allows theapi_path
to be the full path to any referenced secret.This change is also backwards compatible, since this field was not present in the KV plugin. (It did use the slightly different
path
field, which we can keep for now.)Tested with a KV plugin (hashicorp/vault-plugin-secrets-kv#124) modified to output the
secret_path
metadata: