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.

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/acl events #9595

Merged
merged 6 commits into from
Dec 11, 2020
Merged

Events/acl events #9595

merged 6 commits into from
Dec 11, 2020

Conversation

drewbailey
Copy link
Contributor

This PR updates the transaction type for acl token and policy upsert requests to include the message type, which includes it in the event stream.

An unexported secretID value is added to ACLTokenEvent so that the event broker can access the secretID value when evaluating if a subscription should be closed or not, while ensuring that the secretID value is not included in the event stream output when marshaled.

@vercel vercel bot temporarily deployed to Preview – nomad December 9, 2020 21:03 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 9, 2020 21:03 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 9, 2020 21:11 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 9, 2020 21:11 Inactive
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Left one recommendation for making the API more fool Tim-proof, but not a blocker.
Looks like you've got some failing tests to clean up though?

nomad/state/events.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 13:53 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 13:53 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 14:01 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 14:01 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 14:21 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 14:21 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 14:51 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 14:51 Inactive
nomad/fsm_test.go Outdated Show resolved Hide resolved
test that values are omitted

test event creation

test acl events

payloads are pointers

fix failing tests, do all security steps inside constructor
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 21:11 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 21:11 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 21:37 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 21:37 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 10, 2020 23:28 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 10, 2020 23:28 Inactive
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM 👍

nomad/stream/event_broker.go Outdated Show resolved Hide resolved
setupfn func(t *testing.T, fsm *nomadFSM)
raftReq func(t *testing.T) []byte
reqTopic structs.Topic
eventfn func(t *testing.T, e []structs.Event)
Copy link
Member

Choose a reason for hiding this comment

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

I really like how this test got structured with these testing funcs as test cases. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I think this one is right in the sweet spot for it being legible. table tests with huge tables and small test case runs can get hard to read 🤷‍♂️

@vercel vercel bot temporarily deployed to Preview – nomad December 11, 2020 15:21 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 11, 2020 15:21 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad December 11, 2020 15:25 Inactive
@vercel vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 11, 2020 15:27 Inactive
@drewbailey drewbailey merged commit 3e793ea into master Dec 11, 2020
@drewbailey drewbailey deleted the events/acl-events branch December 11, 2020 15:40
schmichael pushed a commit that referenced this pull request Dec 16, 2020
* fix acl event creation

* allow way to access secretID without exposing it to stream

test that values are omitted

test event creation

test acl events

payloads are pointers

fix failing tests, do all security steps inside constructor

* increase time

* ignore empty tokens

* uncomment line

* changelog
@github-actions
Copy link

github-actions bot commented Dec 6, 2022

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants