From 8dbd47ceb5e9d1140186bb37a767a3a3eb817af3 Mon Sep 17 00:00:00 2001 From: Larry Safran Date: Mon, 10 Jul 2023 18:07:54 +0000 Subject: [PATCH] Sort the policies in a rule by policy name when parsing from proto. (#10334) * Sort the policies in a rule by policy name when parsing from proto. This fixes the server sending a GOAWAY when an LDS update with no changes other than ordering is received. * Remove use of deprecated method setSourceIp * Fix style issues * Update RbacFilterTest.java --- xds/src/main/java/io/grpc/xds/RbacFilter.java | 9 ++- .../test/java/io/grpc/xds/RbacFilterTest.java | 75 ++++++++++++++++++- 2 files changed, 78 insertions(+), 6 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/RbacFilter.java b/xds/src/main/java/io/grpc/xds/RbacFilter.java index 0f5ac1025b..6a55f7f193 100644 --- a/xds/src/main/java/io/grpc/xds/RbacFilter.java +++ b/xds/src/main/java/io/grpc/xds/RbacFilter.java @@ -59,8 +59,10 @@ import java.net.UnknownHostException; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Collectors; import javax.annotation.Nullable; /** RBAC Http filter implementation. */ @@ -117,9 +119,12 @@ final class RbacFilter implements Filter, ServerInterceptorBuilder { default: return ConfigOrError.fromError("Unknown rbacConfig action type: " + rbacConfig.getAction()); } - Map policyMap = rbacConfig.getPoliciesMap(); List policyMatchers = new ArrayList<>(); - for (Map.Entry entry: policyMap.entrySet()) { + List> sortedPolicyEntries = rbacConfig.getPoliciesMap().entrySet() + .stream() + .sorted((a,b) -> a.getKey().compareTo(b.getKey())) + .collect(Collectors.toList()); + for (Map.Entry entry: sortedPolicyEntries) { try { Policy policy = entry.getValue(); if (policy.hasCondition() || policy.hasCheckedCondition()) { diff --git a/xds/src/test/java/io/grpc/xds/RbacFilterTest.java b/xds/src/test/java/io/grpc/xds/RbacFilterTest.java index f86b36df5e..29af01b222 100644 --- a/xds/src/test/java/io/grpc/xds/RbacFilterTest.java +++ b/xds/src/test/java/io/grpc/xds/RbacFilterTest.java @@ -26,6 +26,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import com.google.api.expr.v1alpha1.Expr; +import com.google.common.collect.ImmutableList; import com.google.protobuf.Any; import com.google.protobuf.Message; import com.google.protobuf.UInt32Value; @@ -339,6 +340,22 @@ public class RbacFilterTest { assertThat(result.config).isEqualTo(RbacConfig.create(null)); } + @Test + public void testOrderIndependenceOfPolicies() { + Message rawProto = buildComplexRbac(ImmutableList.of(1, 2, 3, 4, 5, 6), true); + ConfigOrError ascFirst = new RbacFilter().parseFilterConfig(Any.pack(rawProto)); + + rawProto = buildComplexRbac(ImmutableList.of(1, 2, 3, 4, 5, 6), false); + ConfigOrError ascLast = new RbacFilter().parseFilterConfig(Any.pack(rawProto)); + + assertThat(ascFirst.config).isEqualTo(ascLast.config); + + rawProto = buildComplexRbac(ImmutableList.of(6, 5, 4, 3, 2, 1), true); + ConfigOrError decFirst = new RbacFilter().parseFilterConfig(Any.pack(rawProto)); + + assertThat(ascFirst.config).isEqualTo(decFirst.config); + } + private static Metadata metadata(String key, String value) { Metadata metadata = new Metadata(); metadata.put(Metadata.Key.of(key, Metadata.ASCII_STRING_MARSHALLER), value); @@ -369,11 +386,61 @@ public class RbacFilterTest { private io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC buildRbac( List permissionList, List principalList) { return io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC.newBuilder() - .setRules(RBAC.newBuilder().setAction(Action.DENY) - .putPolicies("policy-name", Policy.newBuilder() - .addAllPermissions(permissionList) - .addAllPrincipals(principalList).build()).build()).build(); + .setRules(buildRbacRule("policy-name", Action.DENY, + permissionList, principalList)).build(); + } + private static RBAC buildRbacRule(String policyName, Action action, + List permissionList, List principalList) { + return RBAC.newBuilder().setAction(action) + .putPolicies(policyName, Policy.newBuilder() + .addAllPermissions(permissionList) + .addAllPrincipals(principalList).build()) + .build(); + } + + private io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC buildComplexRbac( + List ids, boolean listsFirst) { + Policy policy1 = createSimplePolicyUsingLists(0); + + RBAC.Builder ruleBuilder = RBAC.newBuilder().setAction(Action.DENY); + + if (listsFirst) { + ruleBuilder.putPolicies("list-policy", policy1); + } + + String base = "filterConfig\\u003dRbacConfig{authConfig\\u003dAuthConfig{policies\\u003d[Poli" + + "cyMatcher{name\\u003dpsm-interop-authz-policy-20230514-0917-er2uh_td_rbac_rule_"; + + for (Integer id : ids) { + ruleBuilder.putPolicies(base + id, createSimplePolicyUsingLists(id)); + } + + if (!listsFirst) { + ruleBuilder.putPolicies("list-policy", policy1); + } + + return io.envoyproxy.envoy.extensions.filters.http.rbac.v3.RBAC.newBuilder() + .setRules(ruleBuilder.build()).build(); + } + + private static Policy createSimplePolicyUsingLists(int id) { + CidrRange cidrRange = CidrRange.newBuilder().setAddressPrefix("10.10." + id + ".0") + .setPrefixLen(UInt32Value.of(24)).build(); + List permissionList = Arrays.asList( + Permission.newBuilder().setAndRules(Permission.Set.newBuilder() + .addRules(Permission.newBuilder().setDestinationIp(cidrRange).build()) + .addRules(Permission.newBuilder().setDestinationPort(9090).build()).build() + ).build()); + List principalList = Arrays.asList( + Principal.newBuilder().setAndIds(Principal.Set.newBuilder() + .addIds(Principal.newBuilder().setDirectRemoteIp(cidrRange).build()) + .addIds(Principal.newBuilder().setRemoteIp(cidrRange).build()) + .build()).build()); + + return Policy.newBuilder() + .addAllPermissions(permissionList) + .addAllPrincipals(principalList).build(); } private ConfigOrError parseOverride(List permissionList,