diff --git a/deploy-board/deploy_board/templates/configs/promote_config.tmpl b/deploy-board/deploy_board/templates/configs/promote_config.tmpl index d20c39afe4..da7a04e687 100644 --- a/deploy-board/deploy_board/templates/configs/promote_config.tmpl +++ b/deploy-board/deploy_board/templates/configs/promote_config.tmpl @@ -235,7 +235,7 @@ btn.button('reset'); }, error: function (data) { - $('#errorBannerId').append(data.responseText); + $('#errorBannerId').append(`${data.status}: ${data.responseText}`); $('#errorBannerId').show(); } }); diff --git a/deploy-board/deploy_board/templates/error.tmpl b/deploy-board/deploy_board/templates/error.tmpl index cb62f669ec..c3097ba1c5 100644 --- a/deploy-board/deploy_board/templates/error.tmpl +++ b/deploy-board/deploy_board/templates/error.tmpl @@ -2,5 +2,9 @@ - + diff --git a/deploy-board/deploy_board/webapp/helpers/base_client.py b/deploy-board/deploy_board/webapp/helpers/base_client.py index 2b58269b57..1b670832af 100644 --- a/deploy-board/deploy_board/webapp/helpers/base_client.py +++ b/deploy-board/deploy_board/webapp/helpers/base_client.py @@ -63,7 +63,7 @@ def api(path, token=None, params=None, data=None): "Oops! You do not have the required permissions for this action. Contact an environment ADMIN for " "assistance. " + response.text) - if response.status_code == 400: + if response.status_code == 400 or response.status_code == 422: raise IllegalArgumentException(response.text) if response.status_code == 404: diff --git a/deploy-board/deploy_board/webapp/helpers/exceptions.py b/deploy-board/deploy_board/webapp/helpers/exceptions.py index 22ebb95304..4c0f518570 100644 --- a/deploy-board/deploy_board/webapp/helpers/exceptions.py +++ b/deploy-board/deploy_board/webapp/helpers/exceptions.py @@ -17,10 +17,6 @@ """ -class IllegalArgumentException(Exception): - pass - - class FailedAuthenticationException(Exception): pass @@ -34,4 +30,11 @@ class NotFoundException(Exception): class TeletraanException(Exception): - pass + def __init__(self, message: str, status=500) -> None: + super().__init__(message) + self.status = status + + +class IllegalArgumentException(TeletraanException): + def __init__(self, message: str) -> None: + super().__init__(message, status=400) diff --git a/deploy-board/deploy_board/webapp/promote_views.py b/deploy-board/deploy_board/webapp/promote_views.py index 2146f3d8bf..f3df822a24 100644 --- a/deploy-board/deploy_board/webapp/promote_views.py +++ b/deploy-board/deploy_board/webapp/promote_views.py @@ -23,6 +23,7 @@ from django.views.generic import View from . import common from .helpers import environs_helper +from .helpers.exceptions import TeletraanException, IllegalArgumentException class EnvPromoteConfigView(View): @@ -66,5 +67,8 @@ def post(self, request, name, stage): data["delay"] = int(query_dict["promoteDelay"]) if "promoteQueueSize" in query_dict: data["queueSize"] = int(query_dict["promoteQueueSize"]) - environs_helper.update_env_promotes_config(request, name, stage, data=data) + try: + environs_helper.update_env_promotes_config(request, name, stage, data=data) + except TeletraanException as e: + return HttpResponse(e, status=e.status, content_type="application/json") return self.get(request, name, stage) diff --git a/deploy-service/common/pom.xml b/deploy-service/common/pom.xml index 72342a175f..056c8261b2 100644 --- a/deploy-service/common/pom.xml +++ b/deploy-service/common/pom.xml @@ -285,5 +285,14 @@ 0.10.5 runtime + + org.quartz-scheduler + quartz + + + io.dropwizard + dropwizard-validation + ${dropwizard.version} + \ No newline at end of file diff --git a/deploy-service/common/src/main/java/com/pinterest/deployservice/bean/PromoteBean.java b/deploy-service/common/src/main/java/com/pinterest/deployservice/bean/PromoteBean.java index 3a77589944..869b21fd9a 100644 --- a/deploy-service/common/src/main/java/com/pinterest/deployservice/bean/PromoteBean.java +++ b/deploy-service/common/src/main/java/com/pinterest/deployservice/bean/PromoteBean.java @@ -4,9 +4,9 @@ * Licensed 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. @@ -15,11 +15,21 @@ */ package com.pinterest.deployservice.bean; -import com.fasterxml.jackson.annotation.JsonProperty; +import java.io.Serializable; + +import javax.validation.constraints.Max; +import javax.validation.constraints.Min; +import javax.validation.constraints.NotNull; + import org.apache.commons.lang.builder.ReflectionToStringBuilder; +import org.apache.commons.lang3.StringUtils; +import org.quartz.CronExpression; -import java.io.Serializable; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.pinterest.deployservice.common.Constants; +import io.dropwizard.validation.ValidationMethod; /** * Keep the bean and table in sync *

@@ -47,16 +57,19 @@ public class PromoteBean implements Updatable, Serializable { @JsonProperty("lastUpdate") private Long last_update; + @NotNull private PromoteType type; @JsonProperty("predStage") private String pred_stage; + @Min(1) @Max(Constants.DEFAULT_MAX_PROMOTE_QUEUE_SIZE) @JsonProperty("queueSize") private Integer queue_size; private String schedule; + @Min(0) private Integer delay; @JsonProperty("disablePolicy") @@ -165,4 +178,22 @@ public SetClause genSetClause() { public String toString() { return ReflectionToStringBuilder.toString(this); } + + @ValidationMethod(message = "schedule must be a valid cron expression") + @JsonIgnore + public boolean isScheduleValid() { + if (type != PromoteType.AUTO) { + return true; + } + if (StringUtils.isBlank(schedule)) { + return true; + } + + try { + new CronExpression(schedule); + return true; + } catch (Exception e) { + return false; + } + } } diff --git a/deploy-service/pom.xml b/deploy-service/pom.xml index e182a8b7a8..12284f2a01 100644 --- a/deploy-service/pom.xml +++ b/deploy-service/pom.xml @@ -14,6 +14,8 @@ UTF-8 UTF-8 1.11.3 + 1.3.29 + 5.10.1 @@ -36,10 +38,28 @@ pom import + + org.quartz-scheduler + quartz + 2.3.2 + + + + org.junit.jupiter + junit-jupiter-engine + ${junit-jupiter.version} + test + + + + org.junit.vintage + junit-vintage-engine + ${junit-jupiter.version} + junit junit @@ -71,8 +91,19 @@ maven-source-plugin 2.1.2 + + org.apache.maven.plugins + maven-surefire-plugin + 3.2.5 + + + + org.apache.maven.plugins + maven-surefire-plugin + + @@ -116,4 +147,4 @@ - + \ No newline at end of file diff --git a/deploy-service/teletraanservice/pom.xml b/deploy-service/teletraanservice/pom.xml index ebea17ba5a..f0e36115bb 100644 --- a/deploy-service/teletraanservice/pom.xml +++ b/deploy-service/teletraanservice/pom.xml @@ -19,7 +19,6 @@ UTF-8 UTF-8 true - 1.3.29 1.7.3 @@ -97,7 +96,6 @@ org.quartz-scheduler quartz - 2.3.2 @@ -120,6 +118,18 @@ io.micrometer micrometer-core + + io.dropwizard + dropwizard-testing + ${dropwizard.version} + test + + + org.junit.jupiter + junit-jupiter-params + ${junit-jupiter.version} + test + diff --git a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java index 532780ed58..892d6e65b9 100644 --- a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java +++ b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/TeletraanService.java @@ -25,6 +25,7 @@ import io.dropwizard.health.conf.HealthConfiguration; import io.dropwizard.health.core.HealthCheckBundle; import io.dropwizard.jersey.jackson.JsonProcessingExceptionMapper; +import io.dropwizard.jersey.validation.JerseyViolationExceptionMapper; import io.dropwizard.setup.Bootstrap; import io.dropwizard.setup.Environment; import io.swagger.jaxrs.config.BeanConfig; @@ -172,7 +173,9 @@ public void run(TeletraanServiceConfiguration configuration, Environment environ environment.healthChecks().register("generic", new GenericHealthCheck(context)); - // Exception handler + // Exception handlers + // Constrains validation exceptions, returns 4xx + environment.jersey().register(new JerseyViolationExceptionMapper()); // Jackson Json parsing exceptions environment.jersey().register(new JsonProcessingExceptionMapper(true)); environment.jersey().register(new GenericExceptionMapper(configuration.getSystemFactory().getClientError())); diff --git a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/exception/GenericExceptionMapper.java b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/exception/GenericExceptionMapper.java index 04512fd4dd..7369fd6392 100644 --- a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/exception/GenericExceptionMapper.java +++ b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/exception/GenericExceptionMapper.java @@ -10,8 +10,6 @@ import java.io.StringWriter; import javax.servlet.http.HttpServletRequest; -import javax.validation.ConstraintViolationException; -import javax.validation.ConstraintViolation; import javax.ws.rs.WebApplicationException; import javax.ws.rs.core.Context; import javax.ws.rs.core.Response; @@ -56,17 +54,6 @@ public Response toResponse(Throwable t) { return Response.serverError().entity(sb.toString()).build(); } } - } else if (t instanceof ConstraintViolationException) { - StringBuilder sb = new StringBuilder(); - ConstraintViolationException cve = (ConstraintViolationException)t; - for (ConstraintViolation cv : cve.getConstraintViolations()) { - if (cv.getInvalidValue() != null) { - sb.append(cv.getPropertyPath().toString() + ":" + cv.getInvalidValue().toString()); - sb.append(" " + cv.getMessage()); - } - } - sb.append("\nParameters in request violate configured constraints."); - return Response.status(Response.Status.BAD_REQUEST).entity(sb.toString()).build(); } else { String errorMessage = buildErrorMessage(request); StringBuilder sb = new StringBuilder(); diff --git a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/resource/EnvPromotes.java b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/resource/EnvPromotes.java index e6a7210aa1..1dfdf7c889 100644 --- a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/resource/EnvPromotes.java +++ b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/resource/EnvPromotes.java @@ -4,9 +4,9 @@ * Licensed 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. @@ -74,10 +74,10 @@ public PromoteBean get(@ApiParam(value = "Environment name", required = true)@Pa value = "Update promote info", notes = "Updates promote info given environment and stage names by given promote info object") public void update(@Context SecurityContext sc, - @ApiParam(value = "Environment name", required = true)@PathParam("envName") String envName, - @ApiParam(value = "Stage name", required = true)@PathParam("stageName") String stageName, - @ApiParam(value = "Promote object to update with", required = true) - @Valid PromoteBean promoteBean) throws Exception { + @ApiParam(value = "Environment name", required = true) @PathParam("envName") String envName, + @ApiParam(value = "Stage name", required = true) @PathParam("stageName") String stageName, + @ApiParam(value = "Promote object to update with", required = true) @Valid PromoteBean promoteBean) + throws Exception { EnvironBean environBean = Utils.getEnvStage(environDAO, envName, stageName); authorizer.authorize(sc, new Resource(environBean.getEnv_name(), Resource.Type.ENV), Role.OPERATOR); String operator = sc.getUserPrincipal().getName(); diff --git a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/worker/AutoPromoter.java b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/worker/AutoPromoter.java index 14e82afa20..45ccf80d29 100644 --- a/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/worker/AutoPromoter.java +++ b/deploy-service/teletraanservice/src/main/java/com/pinterest/teletraan/worker/AutoPromoter.java @@ -103,7 +103,7 @@ void processBatch() throws Exception { errorBudgetSuccess.increment(); } catch (Throwable t) { // Catch all throwable so that subsequent job not suppressed - LOG.error("AutoPromoter failed to process {}, Exception: {}", envId, t); + LOG.error("AutoPromoter failed to process {}", envId, t); errorBudgetFailure.increment(); } diff --git a/deploy-service/teletraanservice/src/test/java/com/pinterest/teletraan/resource/EnvPromotesTest.java b/deploy-service/teletraanservice/src/test/java/com/pinterest/teletraan/resource/EnvPromotesTest.java new file mode 100644 index 0000000000..5622a9efcc --- /dev/null +++ b/deploy-service/teletraanservice/src/test/java/com/pinterest/teletraan/resource/EnvPromotesTest.java @@ -0,0 +1,105 @@ +package com.pinterest.teletraan.resource; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; + +import java.util.stream.Stream; + +import javax.ws.rs.client.Entity; +import javax.ws.rs.core.Response; + +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import com.pinterest.deployservice.bean.PromoteBean; +import com.pinterest.deployservice.bean.PromoteDisablePolicy; +import com.pinterest.deployservice.bean.PromoteType; +import com.pinterest.deployservice.common.Constants; +import com.pinterest.teletraan.TeletraanServiceContext; + +import io.dropwizard.testing.junit5.DropwizardExtensionsSupport; +import io.dropwizard.testing.junit5.ResourceExtension; + +@ExtendWith(DropwizardExtensionsSupport.class) +public class EnvPromotesTest { + + private static final ResourceExtension EXT; + + static { + TeletraanServiceContext context = new TeletraanServiceContext(); + + EXT = ResourceExtension.builder() + .addResource(new EnvPromotes(context)) + .build(); + } + + @ParameterizedTest + @MethodSource("invalidPromoteBeansSource") + public void invalidPromoteBeans_422(PromoteBean promoteBean) { + final Response put = EXT.target(Target.V1_ENV_PROMOTES).request().put(Entity.json(promoteBean)); + + assertEquals(422, put.getStatus()); + } + + @ParameterizedTest + @MethodSource("validPromoteBeansSource") + public void invalidPromoteBeans_not422(PromoteBean promoteBean) { + final Response put = EXT.target(Target.V1_ENV_PROMOTES).request().put(Entity.json(promoteBean)); + + assertNotEquals(422, put.getStatus()); + } + + static Stream invalidPromoteBeansSource() { + PromoteBean negativeQueueSize = validPromoteBean(); + negativeQueueSize.setQueue_size(-1); + PromoteBean largeQueueSize = validPromoteBean(); + largeQueueSize.setQueue_size(Constants.DEFAULT_MAX_PROMOTE_QUEUE_SIZE + 1); + PromoteBean negativeDelay = validPromoteBean(); + negativeDelay.setDelay(-1); + PromoteBean invalidCron = validPromoteBean(); + invalidCron.setSchedule("invalidCron"); + PromoteBean invalidCron2 = validPromoteBean(); + invalidCron2.setSchedule("0 0 0 * * *"); + + return Stream.of( + Arguments.of(new PromoteBean()), + Arguments.of(negativeQueueSize), + Arguments.of(largeQueueSize), + Arguments.of(negativeDelay), + Arguments.of(invalidCron), + Arguments.of(invalidCron2) + ); + } + + static Stream validPromoteBeansSource() { + PromoteBean manual = validPromoteBean(); + manual.setType(PromoteType.MANUAL); + manual.setSchedule("invalidCron"); + PromoteBean emptySchedule = validPromoteBean(); + emptySchedule.setSchedule(" "); + PromoteBean validSchedule = validPromoteBean(); + validSchedule.setSchedule("0 0 0 * * ?"); + + return Stream.of( + Arguments.of(validPromoteBean()), + Arguments.of(manual), + Arguments.of(validSchedule), + Arguments.of(emptySchedule) + ); + } + + static PromoteBean validPromoteBean() { + PromoteBean promoteBean = new PromoteBean(); + promoteBean.setQueue_size(1); + promoteBean.setDelay(0); + promoteBean.setType(PromoteType.AUTO); + promoteBean.setDisable_policy(PromoteDisablePolicy.AUTO); + return promoteBean; + } + + private static class Target { + private static final String V1_ENV_PROMOTES = "v1/envs/testEnv/testStage/promotes"; + } +}