-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add V2 TCP traffic permissions #18706
Conversation
8918584
to
7486723
Compare
885f871
to
48d70ff
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.
Thanks for doing this! I only reviewed the protos and had some feedback which I think will impact the rest of the code so I didn't review that yet.
repeated L4Source exclude_sources = 2; | ||
|
||
// We don't need destination rules here because they either apply to L7 features or multi-ports. | ||
// In the case of multi-ports, the proxystateconverter is responsible for filtering per-port intentions. |
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.
proxystateconverter
only coverts v1 stuff to v2 stuff? Is that the right place for this logic? It feels like something that the code that generates IR (i.e. sidecar proxy controller) should figure out.
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.
Yeah, this was wrong. I changed this to xDS controller.
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.
Would it be xds controller though? The xDS controller should only be responsible for filling in service endpoints, leaf certs and CA certs. I think it should be sidecar-proxy controller
proto-public/pbmesh/v1alpha1/pbproxystate/traffic_permissions.proto
Outdated
Show resolved
Hide resolved
proto-public/pbmesh/v1alpha1/pbproxystate/traffic_permissions.proto
Outdated
Show resolved
Hide resolved
proto-public/pbmesh/v1alpha1/pbproxystate/traffic_permissions.proto
Outdated
Show resolved
Hide resolved
proto-public/pbmesh/v1alpha1/pbproxystate/traffic_permissions.proto
Outdated
Show resolved
Hide resolved
720eb00
to
5a8b621
Compare
5a8b621
to
157a0c2
Compare
@@ -552,72 +556,260 @@ func TestMakeRBACNetworkAndHTTPFilters(t *testing.T) { | |||
} | |||
) | |||
|
|||
makeL4Spiffe := func(name string, entMeta *acl.EnterpriseMeta) 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.
These tests are a bit different from the other v2 xds tests because we are using completely different code in v1 and v2. The tests are colocated and sharing the same golden files so we can visually inspect the v1 and v2 intentions / traffic permissions to show that they are equivalent.
8c0f78e
to
b71ac4f
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.
Leaving some more comments mostly on the structure. I think we can probably un-nest some things and make the xds generation do less logic and have envoy do that logic for us. Sorry if I haven't communicated it well on the RFC! Happy to chat about any of this!
// intentions is a list of intentions for this destination. | ||
repeated L7Intention intentions = 4; | ||
// traffic_permissions is a list of intentions for this destination. | ||
repeated L7TrafficPermission traffic_permissions = 4; | ||
// add_empty_intention specifies whether to add an empty intention for this destination, when there are no other intentions specified. | ||
bool add_empty_intention = 5; |
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.
should we rename this? When consul is default deny, we'd still need to know that we need to add an empty rbac filter.
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 removed this field because we can figure this out from other data.
repeated L4Source exclude_sources = 2; | ||
|
||
// We don't need destination rules here because they either apply to L7 features or multi-ports. | ||
// In the case of multi-ports, the proxystateconverter is responsible for filtering per-port intentions. |
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.
Would it be xds controller though? The xDS controller should only be responsible for filling in service endpoints, leaf certs and CA certs. I think it should be sidecar-proxy controller
message L7Principal { | ||
repeated Spiffe spiffes = 1; | ||
repeated Spiffe exclude_spiffes = 2; | ||
} |
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.
Why is the structure different from L4?
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 updated this based on our discussion earlier.
repeated string exclude_spiffe_regexes = 2; | ||
} | ||
|
||
message L7Principal { |
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.
Since we're only doing L4 I wonder if we should omit it altogether. Otherwise, what do you think if we do the following:
message L7Principal {
L7PrincipalID principal_id = 1;
repeated L7PrincipalID exclude_principal_ids = 2;
}
message L7PrincipalID {
oneof id {
MTLSAuthPrincipal mtls = 1;
XFCCPrincipal xfcc = 2;
JWTPrincipal jwt = 3;
}
}
message MTLSAuthPrincipal {
string spiffe_regex = 1;
}
message XFCCPrincipal {
string spiffe_regex = 1;
}
message JWTPrincipal {
// todo:
}
message L7TrafficPermissions {
repeated L7Policy allow_policies = 1;
repeated L7Policy deny_policies = 2;
}
message L7Policy {
repeated L7Principal principals = 1;
repeated L7Rule rules = 2;
}
message L7Rule {
L7PolicyRule rule = 1;
repeated L7PolicyRule exclude_rules = 2;
}
message L7PolicyRule {
string path_exact = 1;
string path_prefix = 2;
string path_regex = 3;
// methods is the list of HTTP methods. If no methods are specified,
// this rule will apply to all methods.
repeated string methods = 4;
RuleHeader header = 5;
}
The idea I have here with principals based on current usage is that it seems that today we use either Authenticated
, or Header
(only for xfcc) or Segment
(only for JWT). Structuring it this way would allow us for more flexibility when we implement JWT without changing the API too much.
The naming with L7 is hard so I ended up using the word Policy
but instead of repeating Permission
again.
if len(trafficPermissions.Permissions) == 1 { | ||
return baseL4PermissionKey | ||
} |
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.
Are you thinking to do this for backward compat?
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.
Yeah, exactly. So we can compare the output to existing golden files. It probably won't make sense to do this for long, but it feels nice in the short term.
message L4TrafficPermissions { | ||
TrafficPermissionAction default_action = 1; | ||
// permission is constructed by pre-flattening all relevant permissions. | ||
repeated L4Permission permissions = 2; |
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 seems like with this you could end up with a list of lists of allow and deny principals which will result in many RBAC filters (i.e. more than 2) unless we flatten it into two? I was imagining that we would combine all allow policies into one rbac filter and all deny policies into one rbac which will then mean that we will always have at most two rbac filters for any filter chain. This would then result in in-lining this object here:
message L4TrafficPermissions {
repeated L4Principal allow_principals = 1;
repeated L4Principal deny_principals = 2;
}
This would then be similar to how computed traffic permissions are structured also.
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.
Yeah, it was a bit awkward, but ended up getting flattened into two or fewer RBAC filters. The solution we came up with earlier fixes the awkwardness.
} | ||
|
||
if allowPolicy != nil { | ||
allowRBAC.Policies[policyLabel(i)] = allowPolicy |
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 was imagining that we would instead put each AllowPrincipal
into a policy with the index of that AllowPrincipal
object. This would allow us to avoid constructing the orPrincipal
for all allow/deny principals.
b71ac4f
to
04cf904
Compare
Description
Testing & Reproduction steps
PR Checklist