Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
authz: Rbac engine audit logging #6225
authz: Rbac engine audit logging #6225
Changes from 81 commits
8b9f59a
56ab1cd
0f3b6e7
6bff58b
9b3ab47
bbafb89
5dad66e
73b1390
27cfe85
c8751d3
441fb5c
2326e9f
abcb93f
3f57e69
b7863ee
48a875e
edf40f1
551ced9
1718c45
dad7293
7b9e96e
1f4c0c0
2eb4512
fd97b2c
ca8f66c
bf571d6
49f1ff3
066adbb
472d752
66fa61a
95a5253
67a71ff
1a4e978
1a5b03d
5490046
1c4b097
0747d62
fa5e894
b95b6f1
9ef033b
4896f7e
6fb2f46
760f946
d426edb
5aef84f
1b60ec1
6287772
90a1306
bc236d9
60338b3
c4e6067
4fec2a8
60088e3
50f2673
26e48ce
67613d3
468126a
4a9e3f8
16ea27e
89bc833
938b6b9
752d9e6
844216c
a46cd2e
f86c660
cab68ba
21a3788
5e5f9e9
724e066
89aca52
1685b62
2f51981
8e20f7f
c5798a9
d35a865
9e16b15
697ad75
2bead31
5ce64e9
1402f64
ac17f03
488c09d
9fa7542
5392f60
421acff
0960c8b
2cb5de1
361dcba
4dd9061
a81070c
50d1699
1637d73
4c2b1bb
218ba4b
1ec90ed
85696a7
d8777da
cfaf71d
dd6ff8d
4362f75
d582f2c
6908c86
ea8f50e
dd2f2a6
52f77b8
2f0a376
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
IIRC, empty audit logger config is a valid thing but it means that Audit is disabled
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.
We might've changed, but I believe last we discussed that custom config on the individual Audit Logger needed to be an empty JSON object?
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 nil means
config
field is missing from the given json, then it will be fine. We want the config to be a valid json object only if it's present.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.
Changed, PTAL
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.
Does this get plumbed back to the user somehow? I traced it back a couple levels and it's never augmented or changed. I don't think this would be a useful error message for a user. Similar with all the other errors in this function / PR - please make sure that the failure modes are understood and that the errors will make sense where they appear.
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 ends up bubbling up through the
NewEngine
funcLooks like that is called in
authz.NewStatic(authzPolicy string)
by the user or via xds config plumbing.I think these errors make sense in the realm of the user getting them back from calling `authz.NewStatic(.), what do you think?
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.
The authz policy is a string and you're giving them errors about nil pointers. That doesn't seem right.
"missing required field: TypedConfig"
or something?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, I'll reword the errors to match that. Will comment back once I push that commit
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.
Reworded some, I think the others left still make sense? PTAL
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.
When the user calls
NewStatic
, are they ending up here? I thought they wouldn't be using the xds protos in that case, andTypedConfig
wouldn't even be a thing for them?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.
Yes - the way the golang was already structured it all pushes into these protos in the end - the
rbac_engine.go
file is ininternal/xds
and was already written to use RBAC protos directlyThere 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.
Does that mean this error won't ever go back out to users of
NewStatic
then, because we ensure the structure is right when we come through that 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.
In the
NewStatic
case it will have gone throughtranslatePolicy
inrbac_translator.go
.This converts from a string policy to the rbac proto.
Then that is passed along to
NewChainEngine
.So checks that happen in
rbac_translator.go
will be guaranteed to be good by the time this path gets toNewChainEngine
.