This is an automated email from the ASF dual-hosted git repository. liubao pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/servicecomb-java-chassis.git
commit bf6e0a4d9e04a61e0d95cddc428b0dc45e26b9a3 Author: liubao <[email protected]> AuthorDate: Tue Jan 5 16:18:46 2021 +0800 [SCB-2116]add configuration entity validation --- .../governance/entity/Configurable.java | 25 +++++++++++ .../servicecomb/governance/marker/Matcher.java | 8 ++++ .../governance/marker/TrafficMarker.java | 16 ++++++- .../governance/policy/AbstractPolicy.java | 20 +++++++-- .../governance/policy/BulkheadPolicy.java | 11 +++++ .../governance/policy/CircuitBreakerPolicy.java | 24 ++++++++++ .../governance/policy/GovernanceRule.java | 7 +++ .../governance/policy/RateLimitingPolicy.java | 14 ++++++ .../servicecomb/governance/policy/RetryPolicy.java | 51 +++++----------------- .../properties/GovernanceProperties.java | 13 ++++-- governance/src/test/resources/application.yaml | 17 +++++++- 11 files changed, 156 insertions(+), 50 deletions(-) diff --git a/governance/src/main/java/org/apache/servicecomb/governance/entity/Configurable.java b/governance/src/main/java/org/apache/servicecomb/governance/entity/Configurable.java new file mode 100644 index 0000000..987ddf0 --- /dev/null +++ b/governance/src/main/java/org/apache/servicecomb/governance/entity/Configurable.java @@ -0,0 +1,25 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.servicecomb.governance.entity; + +/** + * Indicates a object can be configure in configuration file or config center. + */ +public interface Configurable { + boolean isValid(); +} diff --git a/governance/src/main/java/org/apache/servicecomb/governance/marker/Matcher.java b/governance/src/main/java/org/apache/servicecomb/governance/marker/Matcher.java index 371460c..6b1b37a 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/marker/Matcher.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/marker/Matcher.java @@ -19,6 +19,7 @@ package org.apache.servicecomb.governance.marker; import java.util.List; import java.util.Map; +import org.apache.commons.lang3.StringUtils; import org.apache.servicecomb.governance.marker.operator.RawOperator; public class Matcher { @@ -30,6 +31,13 @@ public class Matcher { private String name; + public boolean isValid() { + if (StringUtils.isEmpty(name)) { + return false; + } + return true; + } + public Map<String, RawOperator> getHeaders() { return headers; } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/marker/TrafficMarker.java b/governance/src/main/java/org/apache/servicecomb/governance/marker/TrafficMarker.java index b64c168..90cb6e2 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/marker/TrafficMarker.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/marker/TrafficMarker.java @@ -20,8 +20,9 @@ import java.util.Arrays; import java.util.List; import org.apache.commons.lang3.StringUtils; +import org.apache.servicecomb.governance.entity.Configurable; -public class TrafficMarker { +public class TrafficMarker implements Configurable { private String services; @@ -31,6 +32,19 @@ public class TrafficMarker { return services; } + @Override + public boolean isValid() { + if (matches == null || matches.isEmpty()) { + return false; + } + for (Matcher matcher : matches) { + if (!matcher.isValid()) { + return false; + } + } + return true; + } + public void setServices(String services) { this.services = services; } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/AbstractPolicy.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/AbstractPolicy.java index 3e496c4..2f2aad5 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/AbstractPolicy.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/AbstractPolicy.java @@ -18,11 +18,14 @@ package org.apache.servicecomb.governance.policy; import java.util.List; -public abstract class AbstractPolicy implements Policy { +import org.apache.commons.lang3.StringUtils; +import org.apache.servicecomb.governance.entity.Configurable; - private String name; +public abstract class AbstractPolicy implements Policy, Configurable { - private GovernanceRule rules; + protected String name; + + protected GovernanceRule rules; public GovernanceRule getRules() { return rules; @@ -37,6 +40,17 @@ public abstract class AbstractPolicy implements Policy { } @Override + public boolean isValid() { + if (StringUtils.isEmpty(name)) { + return false; + } + if (rules == null) { + return false; + } + return rules.isValid(); + } + + @Override public boolean match(List<String> items) { if (rules == null) { return false; diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/BulkheadPolicy.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/BulkheadPolicy.java index e414e1d..57703b2 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/BulkheadPolicy.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/BulkheadPolicy.java @@ -46,6 +46,17 @@ public class BulkheadPolicy extends AbstractPolicy { } @Override + public boolean isValid() { + if (maxConcurrentCalls <= 0) { + return false; + } + if (maxWaitDuration < 0) { + return false; + } + return super.isValid(); + } + + @Override public String handler() { return BulkheadHandler.class.getSimpleName(); } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/CircuitBreakerPolicy.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/CircuitBreakerPolicy.java index 4353a25..6470299 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/CircuitBreakerPolicy.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/CircuitBreakerPolicy.java @@ -58,6 +58,30 @@ public class CircuitBreakerPolicy extends AbstractPolicy { public CircuitBreakerPolicy() { } + @Override + public boolean isValid() { + if (failureRateThreshold > 100 || failureRateThreshold <= 0) { + return false; + } + if (slowCallRateThreshold > 100 || slowCallRateThreshold <= 0) { + return false; + } + if (waitDurationInOpenState <= 0) { + return false; + } + if (slowCallDurationThreshold <= 0) { + return false; + } + if (permittedNumberOfCallsInHalfOpenState <= 0) { + return false; + } + if (minimumNumberOfCalls <= 0) { + return false; + } + + return super.isValid(); + } + public int getFailureRateThreshold() { return failureRateThreshold; } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/GovernanceRule.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/GovernanceRule.java index d75b250..480cf55 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/GovernanceRule.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/GovernanceRule.java @@ -47,6 +47,13 @@ public class GovernanceRule { this.precedence = precedence; } + public boolean isValid() { + if (StringUtils.isEmpty(match)) { + return false; + } + return true; + } + public boolean match(String name) { if (StringUtils.isEmpty(this.match)) { return false; diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/RateLimitingPolicy.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/RateLimitingPolicy.java index 2d82d76..8bb312a 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/RateLimitingPolicy.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/RateLimitingPolicy.java @@ -70,6 +70,20 @@ public class RateLimitingPolicy extends AbstractPolicy { } @Override + public boolean isValid() { + if (timeoutDuration < 0) { + return false; + } + if (limitRefreshPeriod <= 0) { + return false; + } + if (rate <= 0) { + return false; + } + return super.isValid(); + } + + @Override public String handler() { return RateLimitingHandler.class.getSimpleName(); } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/policy/RetryPolicy.java b/governance/src/main/java/org/apache/servicecomb/governance/policy/RetryPolicy.java index b6ec4b9..944b349 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/policy/RetryPolicy.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/policy/RetryPolicy.java @@ -16,16 +16,9 @@ */ package org.apache.servicecomb.governance.policy; -import org.springframework.util.StringUtils; - import org.apache.servicecomb.governance.handler.RetryHandler; +import org.springframework.util.StringUtils; -/** - * intervalFunction 失败时可以更改等待时间的函数 - * retryOnResultPredicate 根据返回结果决定是否进行重试 - * retryOnExceptionPredicate 根据失败异常决定是否进行重试 - * - */ public class RetryPolicy extends AbstractPolicy { public static final int DEFAULT_MAX_ATTEMPTS = 3; @@ -43,14 +36,6 @@ public class RetryPolicy extends AbstractPolicy { //需要重试的http status, 逗号分隔 private String retryOnResponseStatus; - //TODO: 需要进行重试的异常列表，反射取异常 - private String retryExceptions; - - //TODO: 需要进行忽略的异常列表 - private String ignoreExceptions; - - private boolean onSame; - public String getRetryOnResponseStatus() { if (StringUtils.isEmpty(retryOnResponseStatus)) { retryOnResponseStatus = DEFAULT_RETRY_ON_RESPONSE_STATUS; @@ -78,28 +63,15 @@ public class RetryPolicy extends AbstractPolicy { this.waitDuration = waitDuration; } - public String getRetryExceptions() { - return retryExceptions; - } - - public void setRetryExceptions(String retryExceptions) { - this.retryExceptions = retryExceptions; - } - - public String getIgnoreExceptions() { - return ignoreExceptions; - } - - public void setIgnoreExceptions(String ignoreExceptions) { - this.ignoreExceptions = ignoreExceptions; - } - - public boolean isOnSame() { - return onSame; - } - - public void setOnSame(boolean onSame) { - this.onSame = onSame; + @Override + public boolean isValid() { + if (maxAttempts < 1) { + return false; + } + if (waitDuration < 0) { + return false; + } + return super.isValid(); } @Override @@ -113,9 +85,6 @@ public class RetryPolicy extends AbstractPolicy { "maxAttempts=" + maxAttempts + ", waitDuration=" + waitDuration + ", retryOnResponseStatus='" + retryOnResponseStatus + '\'' + - ", retryExceptions='" + retryExceptions + '\'' + - ", ignoreExceptions='" + ignoreExceptions + '\'' + - ", onSame=" + onSame + '}'; } } diff --git a/governance/src/main/java/org/apache/servicecomb/governance/properties/GovernanceProperties.java b/governance/src/main/java/org/apache/servicecomb/governance/properties/GovernanceProperties.java index ab852eb..695adb6 100644 --- a/governance/src/main/java/org/apache/servicecomb/governance/properties/GovernanceProperties.java +++ b/governance/src/main/java/org/apache/servicecomb/governance/properties/GovernanceProperties.java @@ -23,6 +23,9 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import org.apache.servicecomb.governance.entity.Configurable; +import org.apache.servicecomb.governance.event.ConfigurationChangedEvent; +import org.apache.servicecomb.governance.event.EventManager; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.InitializingBean; @@ -42,10 +45,7 @@ import org.yaml.snakeyaml.representer.Representer; import com.google.common.eventbus.Subscribe; -import org.apache.servicecomb.governance.event.ConfigurationChangedEvent; -import org.apache.servicecomb.governance.event.EventManager; - -public abstract class GovernanceProperties<T> implements InitializingBean { +public abstract class GovernanceProperties<T extends Configurable> implements InitializingBean { private static final Logger LOGGER = LoggerFactory.getLogger(GovernanceProperties.class); private final Representer representer = new Representer(); @@ -164,6 +164,11 @@ public abstract class GovernanceProperties<T> implements InitializingBean { Yaml entityParser = new Yaml(new Constructor(new TypeDescription(entityClass, entityClass)), representer); T result = entityParser.loadAs(value, entityClass); setName(result, key); + + if (!result.isValid()) { + LOGGER.warn("Entity configuration is not valid and ignored. Key [{}], value [{}]", key, value); + return null; + } return result; } catch (YAMLException e) { LOGGER.error("governance config yaml is illegal : {}", e.getMessage()); diff --git a/governance/src/test/resources/application.yaml b/governance/src/test/resources/application.yaml index 69925df..db4ff33 100644 --- a/governance/src/test/resources/application.yaml +++ b/governance/src/test/resources/application.yaml @@ -22,6 +22,8 @@ servicecomb: - apiPath: exact: "/hello" name: match0 + wrong-name-inogred: | + wrong: some demo-retry: | matches: - apiPath: @@ -42,21 +44,34 @@ servicecomb: rules: match: demo-rateLimiting.match0 rate: 1 + wrongIngored: | + rate: 0 retry: retry0: | rules: match: demo-retry.xx maxAttempts: 3 + wrongIngored: | + rules: + wrong: 0 circuitBreaker: circuitBreaker0: | rules: match: demo-circuitBreaker.xx minimumNumberOfCalls: 2 slidingWindowSize: 2 + wrongIngored: | + rules: + match: demo-circuitBreaker.xx + minimumNumberOfCalls: -1 bulkhead: bulkhead0: | rules: match: demo-bulkhead.xx precedence: 100 maxConcurrentCalls: 1 - maxWaitDuration: 3000 \ No newline at end of file + maxWaitDuration: 3000 + wrongIngored: | + rules: + match: demo-bulkhead.xx + maxWaitDuration: -1 \ No newline at end of file
