Skip to content

rbac: add dynamic metadata to network level filter#4910

Merged
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
venilnoronha:rbac-metadata
Nov 7, 2018
Merged

rbac: add dynamic metadata to network level filter#4910
mattklein123 merged 7 commits intoenvoyproxy:masterfrom
venilnoronha:rbac-metadata

Conversation

@venilnoronha
Copy link
Copy Markdown
Member

@venilnoronha venilnoronha commented Oct 30, 2018

Description: This PR enhances the network level RBAC filter by adding dynamic metadata to the connection's StreamInfo object.
Risk Level: Low
Testing: Updated tests
Docs Changes: N/A
Release Notes: Added a note about this change

@venilnoronha venilnoronha force-pushed the rbac-metadata branch 3 times, most recently from a48c2e0 to 43d53d9 Compare October 30, 2018 17:50
@yangminzhu
Copy link
Copy Markdown
Contributor

yangminzhu commented Oct 31, 2018

/cc @quanjielin Could you take a look at this? Thanks.

yangminzhu
yangminzhu reviewed Oct 31, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@yangminzhu yangminzhu left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Nov 2, 2018

@yangminzhu and @quanjielin / @rodaine any more comments on this?

@venilnoronha
rbac: add dynamic metadata to network level filter
4c72c46 
This commit enhances the network level RBAC filter by adding dynamic
metadata to the connection's StreamInfo object.

Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha
Move const field to common RBAC utility
8d067fa 
Signed-off-by: Venil Noronha <veniln@vmware.com>
quanjielin
quanjielin reviewed Nov 2, 2018
View reviewed changes
yangminzhu
yangminzhu reviewed Nov 2, 2018
View reviewed changes
source/extensions/filters/network/rbac/rbac_filter.cc Outdated
Copy link
Copy Markdown
Contributor

@yangminzhu yangminzhu Nov 2, 2018

Choose a reason for hiding this comment

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

@liminw @quanjielin
Can we merge this with the HTTP filter to use the same set of constants? Instead of two sets of constants representing essentially the same thing.

Copy link
Copy Markdown
Contributor

@quanjielin quanjielin Nov 2, 2018

Choose a reason for hiding this comment

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

we should make tcp/http aligned, the work could happen in this PR or separated PR.

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 2, 2018

Choose a reason for hiding this comment

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

Unsure if renaming the HTTP metadata fields/values will cause any regression externally.

Copy link
Copy Markdown
Contributor

@quanjielin quanjielin Nov 2, 2018

Choose a reason for hiding this comment

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

From istio side, it's marked as Alpha feature, which is 'No guarantees on backward compatibility'.

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 7, 2018

Choose a reason for hiding this comment

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

Tracked via #4981.

@quanjielin
Copy link
Copy Markdown
Contributor

quanjielin commented Nov 2, 2018

/lgtm

@venilnoronha
Add shadow_engine_result field to RBAC metadata
9b7faa9 
Signed-off-by: Venil Noronha <veniln@vmware.com>
@venilnoronha
Fix setDynamicMetadata interface
1ff64c8 
Signed-off-by: Venil Noronha <veniln@vmware.com>
lizan
lizan reviewed Nov 2, 2018
View reviewed changes
@venilnoronha
Replace RBAC static const with ConstSingleton
7ffdcd8 
Signed-off-by: Venil Noronha <veniln@vmware.com>
@mattklein123 mattklein123 self-assigned this Nov 4, 2018
lizan
lizan reviewed Nov 6, 2018
View reviewed changes
@venilnoronha
Use ConstSingleton for RBAC filter metadata keys
fcd1eab 
Signed-off-by: Venil Noronha <veniln@vmware.com>
lizan
lizan previously approved these changes Nov 6, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Great, thanks!

mattklein123
mattklein123 reviewed Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, a doc question. Thank you!

source/extensions/filters/common/rbac/utility.h

class DynamicMetadataKeys {
public:
const std::string ShadowPolicyIdField{"shadow_effective_policyID"};
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Nov 7, 2018

Choose a reason for hiding this comment

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

Does this key and the other keys in this file need to be documented somewhere? Is the idea here that this is meant to be sent in logging data? If we have well known metadata keys I think they should be documented somewhere?

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 7, 2018

Choose a reason for hiding this comment

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

It may be useful to document these constants as other filters may be able to augment Envoy's behavior based on these. I think that should be done along with the rename suggested in this comment in another PR.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Nov 7, 2018

Choose a reason for hiding this comment

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

OK if you plan on doing a follow up, SGTM.

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 7, 2018

Choose a reason for hiding this comment

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

Will do. Let me know if you have suggestions on where to document these.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Nov 7, 2018

Choose a reason for hiding this comment

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

I'm not sure, maybe a new section on "well known dynamic metadata" under here: https://www.envoyproxy.io/docs/envoy/latest/configuration/configuration? I would take a look and see what feels right.

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 7, 2018

Choose a reason for hiding this comment

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

Cool, thanks!

Copy link
Copy Markdown
Member Author

@venilnoronha venilnoronha Nov 7, 2018

Choose a reason for hiding this comment

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

Tracked via #4981.

@venilnoronha venilnoronha mentioned this pull request Nov 7, 2018
Closed
@mattklein123 mattklein123 merged commit ce5621f into envoyproxy:master Nov 7, 2018
@rshriram rshriram mentioned this pull request Nov 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+4 more reviewers

@yangminzhu yangminzhu yangminzhu left review comments

@rshriram rshriram rshriram left review comments

@quanjielin quanjielin quanjielin left review comments

@lizan lizan lizan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@venilnoronha @yangminzhu @rshriram @quanjielin @lizan @mattklein123