Skip to content

Commit 4bfd5e6

Browse files
sauravzgsauravzg
committed
Fixup: 12494 address comments and bring back up to updated ext authz spec
1 parent 0223141 commit 4bfd5e6

File tree

6 files changed

+307
-399
lines changed

6 files changed

+307
-399
lines changed

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java‎

Lines changed: 65 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -17,41 +17,25 @@
1717
package io.grpc.xds.internal.headermutations;
1818

1919
import com.google.common.collect.ImmutableList;
20-
import com.google.common.collect.ImmutableSet;
21-
import io.envoyproxy.envoy.config.core.v3.HeaderValueOption;
20+
import io.grpc.xds.internal.grpcservice.HeaderValueValidationUtils;
2221
import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations;
2322
import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations;
2423
import java.util.Collection;
25-
import java.util.Locale;
2624
import java.util.Optional;
2725
import java.util.function.Predicate;
2826

2927
/**
3028
* The HeaderMutationFilter class is responsible for filtering header mutations based on a given set
3129
* of rules.
3230
*/
33-
public interface HeaderMutationFilter {
31+
public class HeaderMutationFilter {
32+
private final Optional<HeaderMutationRulesConfig> mutationRules;
3433

35-
/**
36-
* A factory for creating {@link HeaderMutationFilter} instances.
37-
*/
38-
@FunctionalInterface
39-
interface Factory {
40-
/**
41-
* Creates a new instance of {@code HeaderMutationFilter}.
42-
*
43-
* @param mutationRules The rules for header mutations. If an empty {@code Optional} is
44-
* provided, all header mutations are allowed by default, except for certain system
45-
* headers. If a {@link HeaderMutationRulesConfig} is provided, mutations will be
46-
* filtered based on the specified rules.
47-
*/
48-
HeaderMutationFilter create(Optional<HeaderMutationRulesConfig> mutationRules);
49-
}
5034

51-
/**
52-
* The default factory for creating {@link HeaderMutationFilter} instances.
53-
*/
54-
Factory INSTANCE = HeaderMutationFilterImpl::new;
35+
36+
public HeaderMutationFilter(Optional<HeaderMutationRulesConfig> mutationRules) { // NOPMD
37+
this.mutationRules = mutationRules;
38+
}
5539

5640
/**
5741
* Filters the given header mutations based on the configured rules and returns the allowed
@@ -62,111 +46,73 @@ interface Factory {
6246
* @throws HeaderMutationDisallowedException if a disallowed mutation is encountered and the rules
6347
* specify that this should be an error.
6448
*/
65-
HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException;
66-
67-
/** Default implementation of {@link HeaderMutationFilter}. */
68-
final class HeaderMutationFilterImpl implements HeaderMutationFilter {
69-
private final Optional<HeaderMutationRulesConfig> mutationRules;
70-
71-
/**
72-
* Set of HTTP/2 pseudo-headers and the host header that are critical for routing and protocol
73-
* correctness. These headers cannot be mutated by user configuration.
74-
*/
75-
private static final ImmutableSet<String> IMMUTABLE_HEADERS =
76-
ImmutableSet.of("host", ":authority", ":scheme", ":method");
77-
78-
private HeaderMutationFilterImpl(Optional<HeaderMutationRulesConfig> mutationRules) { // NOPMD
79-
this.mutationRules = mutationRules;
80-
}
81-
82-
@Override
83-
public HeaderMutations filter(HeaderMutations mutations)
84-
throws HeaderMutationDisallowedException {
85-
ImmutableList<HeaderValueOption> allowedRequestHeaders =
86-
filterCollection(mutations.requestMutations().headers(),
87-
header -> isHeaderMutationAllowed(header.getHeader().getKey())
88-
&& !appendsSystemHeader(header));
89-
ImmutableList<String> allowedRequestHeadersToRemove =
90-
filterCollection(mutations.requestMutations().headersToRemove(),
91-
header -> isHeaderMutationAllowed(header) && isHeaderRemovalAllowed(header));
92-
ImmutableList<HeaderValueOption> allowedResponseHeaders =
93-
filterCollection(mutations.responseMutations().headers(),
94-
header -> isHeaderMutationAllowed(header.getHeader().getKey())
95-
&& !appendsSystemHeader(header));
96-
return HeaderMutations.create(
97-
RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove),
98-
ResponseHeaderMutations.create(allowedResponseHeaders));
99-
}
49+
public HeaderMutations filter(HeaderMutations mutations)
50+
throws HeaderMutationDisallowedException {
51+
ImmutableList<HeaderValueOption> allowedRequestHeaders =
52+
filterCollection(mutations.requestMutations().headers(),
53+
this::shouldIgnore, this::isHeaderMutationAllowed);
54+
ImmutableList<String> allowedRequestHeadersToRemove =
55+
filterCollection(mutations.requestMutations().headersToRemove(),
56+
this::shouldIgnore, this::isHeaderMutationAllowed);
57+
ImmutableList<HeaderValueOption> allowedResponseHeaders =
58+
filterCollection(mutations.responseMutations().headers(),
59+
this::shouldIgnore, this::isHeaderMutationAllowed);
60+
return HeaderMutations.create(
61+
RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove),
62+
ResponseHeaderMutations.create(allowedResponseHeaders));
63+
}
10064

101-
/**
102-
* A generic helper to filter a collection based on a predicate.
103-
*
104-
* @param items The collection of items to filter.
105-
* @param isAllowedPredicate The predicate to apply to each item.
106-
* @param <T> The type of items in the collection.
107-
* @return An immutable list of allowed items.
108-
* @throws HeaderMutationDisallowedException if an item is disallowed and disallowIsError is
109-
* true.
110-
*/
111-
private <T> ImmutableList<T> filterCollection(Collection<T> items,
112-
Predicate<T> isAllowedPredicate) throws HeaderMutationDisallowedException {
113-
ImmutableList.Builder<T> allowed = ImmutableList.builder();
114-
for (T item : items) {
115-
if (isAllowedPredicate.test(item)) {
116-
allowed.add(item);
117-
} else if (disallowIsError()) {
118-
throw new HeaderMutationDisallowedException(
119-
"Header mutation disallowed for header: " + item);
120-
}
65+
/**
66+
* A generic helper to filter a collection based on a predicate.
67+
*/
68+
private <T> ImmutableList<T> filterCollection(Collection<T> items,
69+
Predicate<T> isIgnoredPredicate, Predicate<T> isAllowedPredicate)
70+
throws HeaderMutationDisallowedException {
71+
ImmutableList.Builder<T> allowed = ImmutableList.builder();
72+
for (T item : items) {
73+
if (isIgnoredPredicate.test(item)) {
74+
continue;
75+
}
76+
if (isAllowedPredicate.test(item)) {
77+
allowed.add(item);
78+
} else if (disallowIsError()) {
79+
throw new HeaderMutationDisallowedException(
80+
"Header mutation disallowed for header: " + item);
12181
}
122-
return allowed.build();
12382
}
83+
return allowed.build();
84+
}
12485

125-
private boolean isHeaderRemovalAllowed(String headerKey) {
126-
return !isSystemHeaderKey(headerKey);
127-
}
86+
private boolean shouldIgnore(String key) {
87+
return HeaderValueValidationUtils.shouldIgnore(key);
88+
}
12889

129-
private boolean appendsSystemHeader(HeaderValueOption headerValueOption) {
130-
String key = headerValueOption.getHeader().getKey();
131-
boolean isAppend = headerValueOption
132-
.getAppendAction() == HeaderValueOption.HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD;
133-
return isAppend && isSystemHeaderKey(key);
134-
}
90+
private boolean shouldIgnore(HeaderValueOption option) {
91+
return HeaderValueValidationUtils.shouldIgnore(option.header());
92+
}
13593

136-
private boolean isSystemHeaderKey(String key) {
137-
return key.startsWith(":") || key.toLowerCase(Locale.ROOT).equals("host");
138-
}
94+
private boolean isHeaderMutationAllowed(HeaderValueOption option) {
95+
return isHeaderMutationAllowed(option.header().key());
96+
}
13997

140-
private boolean isHeaderMutationAllowed(String headerName) {
141-
String lowerCaseHeaderName = headerName.toLowerCase(Locale.ROOT);
142-
if (IMMUTABLE_HEADERS.contains(lowerCaseHeaderName)) {
143-
return false;
144-
}
145-
return mutationRules.map(rules -> isHeaderMutationAllowed(lowerCaseHeaderName, rules))
146-
.orElse(true);
147-
}
98+
private boolean isHeaderMutationAllowed(String headerName) {
99+
return mutationRules.map(rules -> isHeaderMutationAllowed(headerName, rules))
100+
.orElse(true);
101+
}
148102

149-
private boolean isHeaderMutationAllowed(String lowerCaseHeaderName,
150-
HeaderMutationRulesConfig rules) {
151-
// TODO(sauravzg): The priority is slightly unclear in the spec.
152-
// Both `disallowAll` and `disallow_expression` take precedence over `all other
153-
// settings`.
154-
// `allow_expression` takes precedence over everything except `disallow_expression`.
155-
// This is a conflict between ordering for `allow_expression` and `disallowAll`.
156-
// Choosing to proceed with current envoy implementation which favors `allow_expression` over
157-
// `disallowAll`.
158-
if (rules.disallowExpression().isPresent()
159-
&& rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) {
160-
return false;
161-
}
162-
if (rules.allowExpression().isPresent()) {
163-
return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches();
164-
}
165-
return !rules.disallowAll();
103+
private boolean isHeaderMutationAllowed(String lowerCaseHeaderName,
104+
HeaderMutationRulesConfig rules) {
105+
if (rules.disallowExpression().isPresent()
106+
&& rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) {
107+
return false;
166108
}
167-
168-
private boolean disallowIsError() {
169-
return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false);
109+
if (rules.allowExpression().isPresent()) {
110+
return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches();
170111
}
112+
return !rules.disallowAll();
113+
}
114+
115+
private boolean disallowIsError() {
116+
return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false);
171117
}
172118
}

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import com.google.auto.value.AutoValue;
2020
import com.google.common.collect.ImmutableList;
21-
import io.envoyproxy.envoy.config.core.v3.HeaderValueOption;
2221

2322
/** A collection of header mutations for both request and response headers. */
2423
@AutoValue

0 commit comments

Comments
 (0)