This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ambari.git
The following commit(s) were added to refs/heads/trunk by this push:
new 2411ccf AMBARI-25037. Allow skipping parts of Add Service request
validation (#2717)
2411ccf is described below
commit 2411ccf6d633aa97cf48d181b6254f3ac36d5f89
Author: Doroszlai, Attila <[email protected]>
AuthorDate: Thu Dec 13 16:06:59 2018 +0100
AMBARI-25037. Allow skipping parts of Add Service request validation (#2717)
---
.../server/controller/AddServiceRequest.java | 45 ++++++++-
.../topology/addservice/RequestValidator.java | 29 ++++--
.../server/controller/AddServiceRequestTest.java | 4 +
.../topology/addservice/RequestValidatorTest.java | 112 ++++++++++++++-------
.../test/resources/add_service_api/request1.json | 1 +
5 files changed, 142 insertions(+), 49 deletions(-)
diff --git
a/ambari-server/src/main/java/org/apache/ambari/server/controller/AddServiceRequest.java
b/ambari-server/src/main/java/org/apache/ambari/server/controller/AddServiceRequest.java
index 04d63a1..02577ca 100644
---
a/ambari-server/src/main/java/org/apache/ambari/server/controller/AddServiceRequest.java
+++
b/ambari-server/src/main/java/org/apache/ambari/server/controller/AddServiceRequest.java
@@ -71,15 +71,17 @@ public class AddServiceRequest {
private static final String STACK_VERSION = "stack_version";
private static final String SERVICES = "services";
private static final String COMPONENTS = "components";
+ private static final String VALIDATION = "validation";
public static final Set<String> TOP_LEVEL_PROPERTIES = ImmutableSet.of(
- OPERATION_TYPE, CONFIG_RECOMMENDATION_STRATEGY, PROVISION_ACTION_PROPERTY,
+ OPERATION_TYPE, CONFIG_RECOMMENDATION_STRATEGY, PROVISION_ACTION_PROPERTY,
VALIDATION,
STACK_NAME, STACK_VERSION, SERVICES, COMPONENTS, CONFIGURATIONS
);
private final OperationType operationType;
private final ConfigRecommendationStrategy recommendationStrategy;
private final ProvisionAction provisionAction;
+ private final ValidationType validationType;
private final String stackName;
private final String stackVersion;
private final Set<Service> services;
@@ -93,6 +95,7 @@ public class AddServiceRequest {
@JsonProperty(OPERATION_TYPE) OperationType operationType,
@JsonProperty(CONFIG_RECOMMENDATION_STRATEGY) ConfigRecommendationStrategy
recommendationStrategy,
@JsonProperty(PROVISION_ACTION_PROPERTY) ProvisionAction provisionAction,
+ @JsonProperty(VALIDATION) ValidationType validationType,
@JsonProperty(STACK_NAME) String stackName,
@JsonProperty(STACK_VERSION) String stackVersion,
@JsonProperty(SERVICES) Set<Service> services,
@@ -101,7 +104,7 @@ public class AddServiceRequest {
@JsonProperty(CREDENTIALS) Set<Credential> credentials,
@JsonProperty(CONFIGURATIONS) Collection<? extends Map<String, ?>> configs
) {
- this(operationType, recommendationStrategy, provisionAction, stackName,
stackVersion, services, components,
+ this(operationType, recommendationStrategy, provisionAction,
validationType, stackName, stackVersion, services, components,
security, credentials,
ConfigurableHelper.parseConfigs(configs)
);
@@ -111,6 +114,7 @@ public class AddServiceRequest {
OperationType operationType,
ConfigRecommendationStrategy recommendationStrategy,
ProvisionAction provisionAction,
+ ValidationType validationType,
String stackName,
String stackVersion,
Set<Service> services,
@@ -122,6 +126,7 @@ public class AddServiceRequest {
this.operationType = null != operationType ? operationType :
OperationType.ADD_SERVICE;
this.recommendationStrategy = null != recommendationStrategy ?
recommendationStrategy : ConfigRecommendationStrategy.NEVER_APPLY;
this.provisionAction = null != provisionAction ? provisionAction :
ProvisionAction.INSTALL_AND_START;
+ this.validationType = validationType != null ? validationType :
ValidationType.DEFAULT;
this.stackName = stackName;
this.stackVersion = stackVersion;
this.services = null != services ? services : emptySet();
@@ -160,6 +165,12 @@ public class AddServiceRequest {
return provisionAction;
}
+ @JsonProperty(VALIDATION)
+ @ApiModelProperty(name = VALIDATION)
+ public ValidationType getValidationType() {
+ return validationType;
+ }
+
@JsonProperty(STACK_NAME)
@ApiModelProperty(name = STACK_NAME)
public String getStackName() {
@@ -233,6 +244,7 @@ public class AddServiceRequest {
AddServiceRequest other = (AddServiceRequest) obj;
return Objects.equals(operationType, other.operationType) &&
+ Objects.equals(validationType, other.validationType) &&
Objects.equals(recommendationStrategy, other.recommendationStrategy) &&
Objects.equals(provisionAction, other.provisionAction) &&
Objects.equals(stackName, other.stackName) &&
@@ -246,7 +258,7 @@ public class AddServiceRequest {
@Override
public int hashCode() {
- return Objects.hash(operationType, recommendationStrategy,
provisionAction, stackName, stackVersion,
+ return Objects.hash(operationType, validationType, recommendationStrategy,
provisionAction, stackName, stackVersion,
services, components, configuration, security);
// credentials is ignored for hashcode, since it's not serialized
}
@@ -255,6 +267,7 @@ public class AddServiceRequest {
public String toString() {
return MoreObjects.toStringHelper(this)
.add(OPERATION_TYPE, operationType)
+ .add(VALIDATION, validationType)
.add(CONFIG_RECOMMENDATION_STRATEGY, recommendationStrategy)
.add(PROVISION_ACTION_PROPERTY, provisionAction)
.add(STACK_NAME, stackName)
@@ -273,6 +286,32 @@ public class AddServiceRequest {
ADD_SERVICE, DELETE_SERVICE, MOVE_SERVICE
}
+ public enum ValidationType {
+ /**
+ * Perform all validation checks.
+ */
+ STRICT {
+ @Override
+ public boolean strictValidation() {
+ return true;
+ }
+ },
+ /**
+ * Skip the parts of validation that are not strictly necessary.
+ */
+ PERMISSIVE {
+ @Override
+ public boolean strictValidation() {
+ return false;
+ }
+ },
+ ;
+
+ public static final ValidationType DEFAULT = STRICT;
+
+ public abstract boolean strictValidation();
+ }
+
public static final class Component {
static final String COMPONENT_NAME = "name";
diff --git
a/ambari-server/src/main/java/org/apache/ambari/server/topology/addservice/RequestValidator.java
b/ambari-server/src/main/java/org/apache/ambari/server/topology/addservice/RequestValidator.java
index f976f82..4a8d14c 100644
---
a/ambari-server/src/main/java/org/apache/ambari/server/topology/addservice/RequestValidator.java
+++
b/ambari-server/src/main/java/org/apache/ambari/server/topology/addservice/RequestValidator.java
@@ -43,6 +43,7 @@ import org.apache.ambari.server.state.SecurityType;
import org.apache.ambari.server.state.StackId;
import org.apache.ambari.server.state.kerberos.KerberosDescriptor;
import org.apache.ambari.server.state.kerberos.KerberosDescriptorFactory;
+import org.apache.ambari.server.state.kerberos.KerberosServiceDescriptor;
import org.apache.ambari.server.topology.Configuration;
import org.apache.ambari.server.topology.SecurityConfigurationFactory;
import org.apache.ambari.server.topology.StackFactory;
@@ -134,7 +135,7 @@ public class RequestValidator {
@VisibleForTesting
void validateSecurity() {
request.getSecurity().ifPresent(requestSecurity -> {
- CHECK.checkArgument(requestSecurity.getType() ==
cluster.getSecurityType(),
+ CHECK.checkArgument(!strictValidation() || requestSecurity.getType() ==
cluster.getSecurityType(),
"Security type in the request (%s), if specified, should match
cluster's security type (%s)",
requestSecurity.getType(), cluster.getSecurityType()
);
@@ -162,13 +163,17 @@ public class RequestValidator {
KerberosDescriptor descriptor =
kerberosDescriptorFactory.createInstance(descriptorMap);
- Set<String> servicesWithNewDescriptor =
descriptor.getServices().keySet();
- Set<String> newServices = state.getNewServices().keySet();
- Set<String> nonNewServices =
ImmutableSet.copyOf(Sets.difference(servicesWithNewDescriptor, newServices));
+ if (strictValidation()) {
+ Map<String, KerberosServiceDescriptor> descriptorServices =
descriptor.getServices();
+ Set<String> servicesWithNewDescriptor = descriptorServices != null ?
descriptorServices.keySet() : ImmutableSet.of();
+ Set<String> newServices = state.getNewServices().keySet();
+ Set<String> nonNewServices =
ImmutableSet.copyOf(Sets.difference(servicesWithNewDescriptor, newServices));
- CHECK.checkArgument(nonNewServices.isEmpty(),
- "Kerberos descriptor should be provided only for new services, but
found other services: %s",
- nonNewServices);
+ CHECK.checkArgument(nonNewServices.isEmpty(),
+ "Kerberos descriptor should be provided only for new services, but
found other services: %s",
+ nonNewServices
+ );
+ }
try {
descriptor.toMap();
@@ -238,8 +243,10 @@ public class RequestValidator {
void validateConfiguration() {
Configuration config = request.getConfiguration();
- for (String type : NOT_ALLOWED_CONFIG_TYPES) {
- CHECK.checkArgument(!config.getProperties().containsKey(type), "Cannot
change '%s' configuration in Add Service request", type);
+ if (strictValidation()) {
+ for (String type : NOT_ALLOWED_CONFIG_TYPES) {
+ CHECK.checkArgument(!config.getProperties().containsKey(type), "Cannot
change '%s' configuration in Add Service request", type);
+ }
}
Configuration clusterConfig = getClusterDesiredConfigs();
@@ -262,6 +269,10 @@ public class RequestValidator {
"Requested host not associated with cluster %s: %s",
cluster.getClusterName(), unknownHosts);
}
+ private boolean strictValidation() {
+ return request.getValidationType().strictValidation();
+ }
+
private Configuration getClusterDesiredConfigs() {
try {
return Configuration.of(configHelper.calculateExistingConfigs(cluster));
diff --git
a/ambari-server/src/test/java/org/apache/ambari/server/controller/AddServiceRequestTest.java
b/ambari-server/src/test/java/org/apache/ambari/server/controller/AddServiceRequestTest.java
index 35491d6..2d97bd0 100644
---
a/ambari-server/src/test/java/org/apache/ambari/server/controller/AddServiceRequestTest.java
+++
b/ambari-server/src/test/java/org/apache/ambari/server/controller/AddServiceRequestTest.java
@@ -21,6 +21,8 @@ package org.apache.ambari.server.controller;
import static org.apache.ambari.server.controller.AddServiceRequest.Component;
import static
org.apache.ambari.server.controller.AddServiceRequest.OperationType.ADD_SERVICE;
import static org.apache.ambari.server.controller.AddServiceRequest.Service;
+import static
org.apache.ambari.server.controller.AddServiceRequest.ValidationType.PERMISSIVE;
+import static
org.apache.ambari.server.controller.AddServiceRequest.ValidationType.STRICT;
import static
org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_AND_START;
import static
org.apache.ambari.server.controller.internal.ProvisionAction.INSTALL_ONLY;
import static
org.apache.ambari.server.topology.ConfigRecommendationStrategy.ALWAYS_APPLY;
@@ -86,6 +88,7 @@ public class AddServiceRequestTest {
assertEquals(ADD_SERVICE, request.getOperationType());
assertEquals(ALWAYS_APPLY, request.getRecommendationStrategy());
assertEquals(INSTALL_ONLY, request.getProvisionAction());
+ assertEquals(PERMISSIVE, request.getValidationType());
assertEquals("HDP", request.getStackName());
assertEquals("3.0", request.getStackVersion());
@@ -134,6 +137,7 @@ public class AddServiceRequestTest {
assertEquals(ADD_SERVICE, request.getOperationType());
assertEquals(NEVER_APPLY, request.getRecommendationStrategy());
assertEquals(INSTALL_AND_START, request.getProvisionAction());
+ assertEquals(STRICT, request.getValidationType());
assertNull(request.getStackName());
assertNull(request.getStackVersion());
assertEquals(Optional.empty(), request.getSecurity());
diff --git
a/ambari-server/src/test/java/org/apache/ambari/server/topology/addservice/RequestValidatorTest.java
b/ambari-server/src/test/java/org/apache/ambari/server/topology/addservice/RequestValidatorTest.java
index d0ab25c..e9c3ff7 100644
---
a/ambari-server/src/test/java/org/apache/ambari/server/topology/addservice/RequestValidatorTest.java
+++
b/ambari-server/src/test/java/org/apache/ambari/server/topology/addservice/RequestValidatorTest.java
@@ -77,13 +77,19 @@ public class RequestValidatorTest extends EasyMockSupport {
@Before
public void setUp() {
- validator.setState(RequestValidator.State.INITIAL);
- expect(cluster.getClusterName()).andReturn("TEST").anyTimes();
- expect(cluster.getServices()).andStubReturn(ImmutableMap.of());
- expect(cluster.getSecurityType()).andStubReturn(SecurityType.NONE);
- expect(request.getServices()).andStubReturn(ImmutableSet.of());
- expect(request.getComponents()).andStubReturn(ImmutableSet.of());
- expect(request.getSecurity()).andStubReturn(Optional.empty());
+ try {
+ validator.setState(RequestValidator.State.INITIAL);
+ expect(cluster.getClusterName()).andReturn("TEST").anyTimes();
+ expect(cluster.getServices()).andStubReturn(ImmutableMap.of());
+ expect(cluster.getSecurityType()).andStubReturn(SecurityType.NONE);
+
expect(configHelper.calculateExistingConfigs(cluster)).andStubReturn(Configuration.newEmpty().asPair());
+ expect(request.getServices()).andStubReturn(ImmutableSet.of());
+ expect(request.getComponents()).andStubReturn(ImmutableSet.of());
+ expect(request.getSecurity()).andStubReturn(Optional.empty());
+
expect(request.getValidationType()).andStubReturn(AddServiceRequest.ValidationType.DEFAULT);
+ } catch (Exception e) {
+ throw new IllegalStateException(e);
+ }
}
@After
@@ -347,22 +353,15 @@ public class RequestValidatorTest extends EasyMockSupport
{
@Test
public void rejectsNoneSecurityWhenClusterHasKerberos() {
- secureCluster();
-
expect(request.getSecurity()).andReturn(Optional.of(SecurityConfiguration.NONE)).anyTimes();
- replayAll();
-
- IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
validator::validateSecurity);
- assertTrue(e.getMessage().contains("KERBEROS"));
+ testBothValidationTypes(validator::validateSecurity, "KERBEROS", () -> {
+ secureCluster();
+
expect(request.getSecurity()).andReturn(Optional.of(SecurityConfiguration.NONE)).anyTimes();
+ });
}
@Test
public void rejectsKerberosSecurityWhenClusterHasNone() {
- requestSpecifiesSecurity();
- replayAll();
-
- assertThrows(IllegalArgumentException.class, validator::validateSecurity);
- IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
validator::validateSecurity);
- assertTrue(e.getMessage().contains("KERBEROS"));
+ testBothValidationTypes(validator::validateSecurity, "KERBEROS",
this::requestSpecifiesSecurity);
}
@Test
@@ -371,7 +370,6 @@ public class RequestValidatorTest extends EasyMockSupport {
expect(request.getSecurity()).andReturn(Optional.of(requestSecurity)).anyTimes();
replayAll();
- assertThrows(IllegalArgumentException.class, validator::validateSecurity);
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
validator::validateSecurity);
assertTrue(e.getMessage().contains("Kerberos descriptor"));
}
@@ -382,7 +380,6 @@ public class RequestValidatorTest extends EasyMockSupport {
expect(request.getSecurity()).andReturn(Optional.of(requestSecurity)).anyTimes();
replayAll();
- assertThrows(IllegalArgumentException.class, validator::validateSecurity);
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
validator::validateSecurity);
assertTrue(e.getMessage().contains("Kerberos descriptor reference"));
}
@@ -394,7 +391,6 @@ public class RequestValidatorTest extends EasyMockSupport {
expect(request.getSecurity()).andReturn(Optional.of(invalidConfig)).anyTimes();
replayAll();
- assertThrows(IllegalArgumentException.class, validator::validateSecurity);
IllegalArgumentException e = assertThrows(IllegalArgumentException.class,
validator::validateSecurity);
assertTrue(e.getMessage().contains("Kerberos descriptor and reference"));
}
@@ -442,7 +438,7 @@ public class RequestValidatorTest extends EasyMockSupport {
}
@Test
- public void rejectsDescriptorWithAdditionalServices() {
+ public void acceptsDescriptorWithAdditionalServices() {
String newService = "KAFKA", otherService = "ZOOKEEPER";
secureCluster();
requestServices(true, newService);
@@ -454,6 +450,18 @@ public class RequestValidatorTest extends EasyMockSupport {
}
@Test
+ public void acceptsDescriptorWithoutServices() {
+ secureCluster();
+ requestServices(true, "KAFKA");
+ KerberosDescriptor kerberosDescriptor =
requestHasKerberosDescriptorFor(false);
+ replayAll();
+
+ validator.validateSecurity();
+
+ assertEquals(kerberosDescriptor,
validator.getState().getKerberosDescriptor());
+ }
+
+ @Test
public void combinesRequestConfigWithClusterAndStack() throws
AmbariException {
Configuration requestConfig = Configuration.newEmpty();
requestConfig.setProperty("kafka-broker", "zookeeper.connect",
"zookeeper.connect:request");
@@ -482,22 +490,47 @@ public class RequestValidatorTest extends EasyMockSupport
{
@Test
public void rejectsKerberosEnvChange() {
- Configuration requestConfig = Configuration.newEmpty();
- requestConfig.setProperty("kerberos-env", "some-property", "some-value");
-
expect(request.getConfiguration()).andReturn(requestConfig.copy()).anyTimes();
- replayAll();
-
- assertThrows(IllegalArgumentException.class,
validator::validateConfiguration);
+ testBothValidationTypes(validator::validateConfiguration, () -> {
+ Configuration requestConfig = Configuration.newEmpty();
+ requestConfig.setProperty("kerberos-env", "some-property", "some-value");
+
expect(request.getConfiguration()).andReturn(requestConfig.copy()).anyTimes();
+
validator.setState(RequestValidator.State.INITIAL.with(simpleMockStack()));
+ });
}
@Test
public void rejectsKrb5ConfChange() {
- Configuration requestConfig = Configuration.newEmpty();
- requestConfig.setProperty("krb5-conf", "some-property", "some-value");
-
expect(request.getConfiguration()).andReturn(requestConfig.copy()).anyTimes();
+ testBothValidationTypes(validator::validateConfiguration, () -> {
+ Configuration requestConfig = Configuration.newEmpty();
+ requestConfig.setProperty("krb5-conf", "some-property", "some-value");
+
expect(request.getConfiguration()).andReturn(requestConfig.copy()).anyTimes();
+
validator.setState(RequestValidator.State.INITIAL.with(simpleMockStack()));
+ });
+ }
+
+ /**
+ * Tests that the state created by {@code testCaseSetup} is rejected by
strict validation, but accepted by permissive one.
+ * @param validation the method to call on validator
+ * @param testCaseSetup code to setup the state ("when"), call to
replayAll() should be omitted
+ */
+ private void testBothValidationTypes(Runnable validation, Runnable
testCaseSetup) {
+ testBothValidationTypes(validation, null, testCaseSetup);
+ }
+
+ private void testBothValidationTypes(Runnable validation, String
expectedMessage, Runnable testCaseSetup) {
+ testCaseSetup.run();
replayAll();
+ Exception e = assertThrows(IllegalArgumentException.class, validation);
+ if (expectedMessage != null) {
+ assertTrue(e.getMessage().contains(expectedMessage));
+ };
- assertThrows(IllegalArgumentException.class,
validator::validateConfiguration);
+ resetAll();
+ setUp();
+ permissiveValidation();
+ testCaseSetup.run();
+ replayAll();
+ validation.run();
}
private static void verifyConfigOverrides(Configuration requestConfig,
Configuration clusterConfig, Configuration stackConfig, Configuration
actualConfig) {
@@ -529,10 +562,11 @@ public class RequestValidatorTest extends EasyMockSupport
{
private Stack simpleMockStack() {
Stack stack = createNiceMock(Stack.class);
Set<String> stackServices = ImmutableSet.of("KAFKA", "ZOOKEEPER");
- expect(stack.getServices()).andReturn(stackServices).anyTimes();
-
expect(stack.getServiceForComponent("KAFKA_BROKER")).andReturn("KAFKA").anyTimes();
-
expect(stack.getServiceForComponent("ZOOKEEPER_SERVER")).andReturn("ZOOKEEPER").anyTimes();
-
expect(stack.getServiceForComponent("ZOOKEEPER_CLIENT")).andReturn("ZOOKEEPER").anyTimes();
+ expect(stack.getServices()).andStubReturn(stackServices);
+
expect(stack.getServiceForComponent("KAFKA_BROKER")).andStubReturn("KAFKA");
+
expect(stack.getServiceForComponent("ZOOKEEPER_SERVER")).andStubReturn("ZOOKEEPER");
+
expect(stack.getServiceForComponent("ZOOKEEPER_CLIENT")).andStubReturn("ZOOKEEPER");
+
expect(stack.getValidDefaultConfig()).andStubReturn(Configuration.newEmpty());
return stack;
}
@@ -542,6 +576,10 @@ public class RequestValidatorTest extends EasyMockSupport {
);
}
+ private void permissiveValidation() {
+
expect(request.getValidationType()).andReturn(AddServiceRequest.ValidationType.PERMISSIVE).anyTimes();
+ }
+
private void requestServices(boolean validated, String... services) {
expect(request.getServices()).andReturn(
Arrays.stream(services)
diff --git a/ambari-server/src/test/resources/add_service_api/request1.json
b/ambari-server/src/test/resources/add_service_api/request1.json
index 73320bd..75fb12f 100644
--- a/ambari-server/src/test/resources/add_service_api/request1.json
+++ b/ambari-server/src/test/resources/add_service_api/request1.json
@@ -1,6 +1,7 @@
{
"operation_type" : "ADD_SERVICE",
"config_recommendation_strategy" : "ALWAYS_APPLY",
+ "validation": "PERMISSIVE",
"provision_action" : "INSTALL_ONLY",
"stack_name" : "HDP",
"stack_version" : "3.0",
