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

bug: Handle missing policy documents in event streams #15495

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

wjnicholson
Copy link
Contributor

@wjnicholson wjnicholson commented Dec 8, 2022

To fix #15493

Will need a test too.

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 8, 2022

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Dec 8, 2022

@wjnicholson is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

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.

Hi @wjnicholson! This approach seems reasonable.

But it seems from the discussion in #14058 that we might not necessarily want to keep the existing behavior of this on other endpoints, in which case this PR would be adding work we want to rip out later. I'm going to ping @jrasell on this, as he's recently worked on the Roles feature and may have more context than me.

@jrasell
Copy link
Member

jrasell commented Feb 14, 2023

Hi @wjnicholson! This approach seems reasonable.

But it seems from the discussion in #14058 that we might not necessarily want to keep the existing behavior of this on other endpoints, in which case this PR would be adding work we want to rip out later. I'm going to ping @jrasell on this, as he's recently worked on the Roles feature and may have more context than me.

Apologies for the delay. I think there is still some discussion to have on the issue and any change would come in an upcoming major release, and therefore not in the immediate term. I'm inclined to therefore say we should accept this change, which brings the stream inline with how policies work in the RPC and client caching. Any work on changing the existing behaviour would involve multiple subsystems, so this change I don't think adds too much more and the advantage of consistency seems like a good thing.

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. Added a changelog entry and I'll get this merged once CI is green. We'll release this in 1.5.0 GA (with backports)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.3.x backport to 1.3.x release line backport/1.4.x backport to 1.4.x release line theme/auth
Projects
Development

Successfully merging this pull request may close these issues.

Event Streams: A missing ACL policy on token will cause GET /v1/event/stream to fail
4 participants