From 4de201a42c387cce6ce94f16f3baa562b429fb63 Mon Sep 17 00:00:00 2001 From: Dennis Frommknecht Date: Mon, 15 May 2023 12:16:37 +0200 Subject: [PATCH 1/2] Use consistent list of micrometer tags in web observation handler The tag `spring.security.reached.filter.name` is only set if a filter-name is available, otherwise the tag is omitted entirely. This leads to issues with metric-exporters that don't support dynamic tags, but rather expect tag-names of a metric to be always the same. The most prominent example is the Prometheus-exporter. Instead of omitting the tag if no filer-name is set, a none-value is applied instead, making the tag-list consistent in all cases Closes gh-13179 --- .../web/ObservationFilterChainDecorator.java | 11 ++-- .../ObservationWebFilterChainDecorator.java | 10 ++-- .../ObservationFilterChainDecoratorTests.java | 52 +++++++++++++++++++ ...servationWebFilterChainDecoratorTests.java | 52 +++++++++++++++++++ 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java index 939ece90b90..e4dc43dddfe 100644 --- a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java @@ -22,6 +22,7 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import io.micrometer.common.KeyValue; import io.micrometer.common.KeyValues; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationConvention; @@ -515,13 +516,11 @@ public String getContextualName(FilterChainObservationContext context) { @Override public KeyValues getLowCardinalityKeyValues(FilterChainObservationContext context) { - KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) + return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) - .and(FILTER_SECTION_NAME, context.getFilterSection()); - if (context.getFilterName() != null) { - kv = kv.and(FILTER_NAME, context.getFilterName()); - } - return kv; + .and(FILTER_SECTION_NAME, context.getFilterSection()) + .and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty()) + ? context.getFilterName() : KeyValue.NONE_VALUE); } @Override diff --git a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java index 5d6f5f528b7..fca2092e77c 100644 --- a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java @@ -686,13 +686,11 @@ public String getContextualName(WebFilterChainObservationContext context) { @Override public KeyValues getLowCardinalityKeyValues(WebFilterChainObservationContext context) { - KeyValues kv = KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) + return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) - .and(FILTER_SECTION_NAME, context.getFilterSection()); - if (context.getFilterName() != null) { - kv = kv.and(FILTER_NAME, context.getFilterName()); - } - return kv; + .and(FILTER_SECTION_NAME, context.getFilterSection()) + .and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty()) + ? context.getFilterName() : KeyValue.NONE_VALUE); } @Override diff --git a/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java b/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java index d10e83bc31b..60ae1232165 100644 --- a/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java +++ b/web/src/test/java/org/springframework/security/web/ObservationFilterChainDecoratorTests.java @@ -16,14 +16,22 @@ package org.springframework.security.web; +import java.io.IOException; import java.util.List; +import java.util.stream.Stream; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; import io.micrometer.observation.ObservationRegistry; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.mockito.ArgumentCaptor; import org.springframework.mock.web.MockHttpServletRequest; @@ -106,4 +114,48 @@ void decorateFiltersWhenErrorsThenClosesObservationOnlyOnce() throws Exception { verify(handler).onScopeClosed(any()); } + @ParameterizedTest + @MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag") + void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(Filter filter, + String expectedFilterNameTag) throws Exception { + ObservationHandler handler = mock(ObservationHandler.class); + given(handler.supportsContext(any())).willReturn(true); + ObservationRegistry registry = ObservationRegistry.create(); + registry.observationConfig().observationHandler(handler); + ObservationFilterChainDecorator decorator = new ObservationFilterChainDecorator(registry); + FilterChain chain = mock(FilterChain.class); + FilterChain decorated = decorator.decorate(chain, List.of(filter)); + decorated.doFilter(new MockHttpServletRequest("GET", "/"), new MockHttpServletResponse()); + ArgumentCaptor context = ArgumentCaptor.forClass(Observation.Context.class); + verify(handler, times(3)).onScopeClosed(context.capture()); + assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue()) + .isEqualTo(expectedFilterNameTag); + } + + static Stream decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag() { + Filter filterWithName = new BasicAuthenticationFilter(); + + // Anonymous class leads to an empty filter-name + Filter filterWithoutName = new Filter() { + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + chain.doFilter(request, response); + } + }; + + return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"), + Arguments.of(filterWithoutName, "none")); + } + + private static class BasicAuthenticationFilter implements Filter { + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + chain.doFilter(request, response); + } + + } + } diff --git a/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java b/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java index d4a6e702ad5..fc5379118bb 100644 --- a/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java +++ b/web/src/test/java/org/springframework/security/web/server/ObservationWebFilterChainDecoratorTests.java @@ -18,16 +18,22 @@ import java.util.ArrayList; import java.util.List; +import java.util.stream.Stream; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; import io.micrometer.observation.ObservationRegistry; import io.micrometer.observation.contextpropagation.ObservationThreadLocalAccessor; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.ArgumentCaptor; import reactor.core.publisher.Mono; import org.springframework.mock.http.server.reactive.MockServerHttpRequest; import org.springframework.mock.web.server.MockServerWebExchange; +import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; import org.springframework.web.server.WebFilterChain; @@ -35,6 +41,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -113,6 +120,51 @@ void decorateWhenCustomAfterFilterThenObserves() { handler.assertSpanStop(9, "http"); } + @ParameterizedTest + @MethodSource("decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments") + void decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTag(WebFilter filter, + String expectedFilterNameTag) { + ObservationHandler handler = mock(ObservationHandler.class); + given(handler.supportsContext(any())).willReturn(true); + ObservationRegistry registry = ObservationRegistry.create(); + registry.observationConfig().observationHandler(handler); + ObservationWebFilterChainDecorator decorator = new ObservationWebFilterChainDecorator(registry); + WebFilterChain chain = mock(WebFilterChain.class); + given(chain.filter(any())).willReturn(Mono.empty()); + WebFilterChain decorated = decorator.decorate(chain, List.of(filter)); + decorated.filter(MockServerWebExchange.from(MockServerHttpRequest.get("/").build())).block(); + + ArgumentCaptor context = ArgumentCaptor.forClass(Observation.Context.class); + verify(handler, times(3)).onStop(context.capture()); + + assertThat(context.getValue().getLowCardinalityKeyValue("spring.security.reached.filter.name").getValue()) + .isEqualTo(expectedFilterNameTag); + } + + static Stream decorateFiltersWhenCompletesThenHasSpringSecurityReachedFilterNameTagArguments() { + WebFilter filterWithName = new BasicAuthenticationFilter(); + + // Anonymous class leads to an empty filter-name + WebFilter filterWithoutName = new WebFilter() { + @Override + public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { + return chain.filter(exchange); + } + }; + + return Stream.of(Arguments.of(filterWithName, "BasicAuthenticationFilter"), + Arguments.of(filterWithoutName, "none")); + } + + static class BasicAuthenticationFilter implements WebFilter { + + @Override + public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { + return chain.filter(exchange); + } + + } + static class AccumulatingObservationHandler implements ObservationHandler { List contexts = new ArrayList<>(); From 9583b0634dd993d5c73f40c6dae43c92982789ca Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 17 May 2023 13:40:21 -0600 Subject: [PATCH 2/2] Polish Use StringUtils#hasText PR gh-13179 --- .../security/web/ObservationFilterChainDecorator.java | 5 +++-- .../web/server/ObservationWebFilterChainDecorator.java | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java index e4dc43dddfe..6586bb7521e 100644 --- a/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/ObservationFilterChainDecorator.java @@ -37,6 +37,7 @@ import org.apache.commons.logging.LogFactory; import org.springframework.core.log.LogMessage; +import org.springframework.util.StringUtils; /** * A {@link org.springframework.security.web.FilterChainProxy.FilterChainDecorator} that @@ -519,8 +520,8 @@ public KeyValues getLowCardinalityKeyValues(FilterChainObservationContext contex return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) .and(FILTER_SECTION_NAME, context.getFilterSection()) - .and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty()) - ? context.getFilterName() : KeyValue.NONE_VALUE); + .and(FILTER_NAME, (StringUtils.hasText(context.getFilterName())) ? context.getFilterName() + : KeyValue.NONE_VALUE); } @Override diff --git a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java index fca2092e77c..7d947722405 100644 --- a/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java +++ b/web/src/main/java/org/springframework/security/web/server/ObservationWebFilterChainDecorator.java @@ -34,6 +34,7 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.StringUtils; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.server.WebFilter; import org.springframework.web.server.WebFilterChain; @@ -689,8 +690,8 @@ public KeyValues getLowCardinalityKeyValues(WebFilterChainObservationContext con return KeyValues.of(CHAIN_SIZE_NAME, String.valueOf(context.getChainSize())) .and(CHAIN_POSITION_NAME, String.valueOf(context.getChainPosition())) .and(FILTER_SECTION_NAME, context.getFilterSection()) - .and(FILTER_NAME, (context.getFilterName() != null && !context.getFilterName().isEmpty()) - ? context.getFilterName() : KeyValue.NONE_VALUE); + .and(FILTER_NAME, (StringUtils.hasText(context.getFilterName())) ? context.getFilterName() + : KeyValue.NONE_VALUE); } @Override