Was there a reason you didn't use RbacFilter directly?

I moved the rbac parsing logic to a common location in io.grpc.xds.internal.rbac.engine package due to following reasons

1. to avoid dependency on xds. Also RbacFilter is not public in io.grpc.xds; cannot be accessed from outside package
2. for code readability. the code seemed more readable to me if we split the rbac parsing code into a different class, instead of depending on RbacFilter implementation

Was there a reason you didn't use RbacFilter directly?

I moved the rbac parsing logic to a common location in io.grpc.xds.internal.rbac.engine package due to following reasons

1. to avoid dependency on xds. Also RbacFilter is not public in io.grpc.xds; cannot be accessed from outside package
2. for code readability. the code seemed more readable to me if we split the rbac parsing code into a different class, instead of depending on RbacFilter implementation

I think Eric is saying you only need to call rbacfilter.buildServerIntercetor()
but yea we have to make RbacFilter accessible from auth

I think Eric is saying you only need to call rbacfilter.buildServerIntercetor() but yea we have to make RbacFilter accessible from auth

In non-xDS authorization, we have upto two RBAC engines per Server Interceptor. And the grpc user can use the factory method to create the interceptor and attach it to server enabling grpc authorization.
gRPC Java authz API design

If we use rbac filter's method to generate server interceptor, then we would have upto two interceptors. This could complicate the API that we expose to the users or we could have two interceptors within an interceptor and keep the API unchanged. Also, when we introduce dynamic file reloading support, that is we reload the authorization policy periodically from file system, we would keep on creating new server interceptors as the policy changes, and this could complicate the design like we would have to consider the conditions where both interceptors aren't updated atomically, and we have authorization decisions being made with stale and new interceptor say. Plus code wise, I don't see any code duplication, just the creation and interception of interceptor, so I feel like we should keep xds and non-xds separate.

**EDIT: We can just swap the old with new internal static interceptor in file watcher to deal with synchronization issues. I still prefer keeping them separate as it simplifies code.

This would complicate the API that we expose to the users

No. Two vs 1 interceptor internally should have no bearing on the API we expose to users.

I suggest we do something like

This would complicate the API that we expose to the users

No. Two vs 1 interceptor internally should have no bearing on the API we expose to users.

Right! I meant the same thing above (quoting previous message "or we could have two interceptors within an interceptor and keep the API unchanged")
I updated the implementation based on the comment. Please take a look. Sorry it took so long. I was on a month long vacation and was busy catching up on some work.

@ejona86 Please take a look at the PR when you find time. Thank you:)

@ejona86 Friendly ping!:)

@ejona86 PTAL

@YifeiZhuang, do you know how we were missing contains and stringMatcher?

@YifeiZhuang, do you know how we were missing contains and stringMatcher?

Oh, apparently they aren't used by this code, so that resolves some of my questions. I think they were just added later. @ashithasantosh simply noticed they were missing when comparing to C++, which makes sense.

@ejona86 Thank you for the review. PTAL I resolved all the comments.
@YifeiZhuang already approved the PR, should I wait on a final review?