From dbdc8eb6b0530d137edadf790ea7725d4f2eb7f5 Mon Sep 17 00:00:00 2001 From: Glenn Renfro Date: Thu, 12 Dec 2019 16:57:27 -0500 Subject: [PATCH 1/3] Meaning message returned if schedule name too long resolves #353 --- .../kubernetes/KubernetesScheduler.java | 30 ++++++++++++++++++- .../kubernetes/KubernetesSchedulerTests.java | 19 ++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java index 71f00a77..9e0c4650 100644 --- a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java +++ b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java @@ -32,6 +32,7 @@ import io.fabric8.kubernetes.api.model.batch.CronJobList; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientException; +import javax.validation.ConstraintViolationException; import org.springframework.cloud.deployer.spi.scheduler.CreateScheduleException; import org.springframework.cloud.deployer.spi.scheduler.ScheduleInfo; @@ -54,6 +55,8 @@ public class KubernetesScheduler implements Scheduler { private static final String SCHEDULE_EXPRESSION_FIELD_NAME = "spec.schedule"; + private static final String SCHEDULE_METADATA_FIELD_NAME = "metadata.name"; + private final KubernetesClient kubernetesClient; private final KubernetesSchedulerProperties kubernetesSchedulerProperties; @@ -82,6 +85,11 @@ public void schedule(ScheduleRequest scheduleRequest) { throw new CreateScheduleException(invalidCronExceptionMessage, e); } + invalidCronExceptionMessage = getExceptionMessageForField(e, SCHEDULE_METADATA_FIELD_NAME); + if (isScheduleNameTooLong(invalidCronExceptionMessage)) { + throw new CreateScheduleException(invalidCronExceptionMessage, e); + } + throw new CreateScheduleException("Failed to create schedule " + scheduleRequest.getScheduleName(), e); } } @@ -145,7 +153,19 @@ protected CronJob createCronJob(ScheduleRequest scheduleRequest) { String schedule = scheduleRequest.getSchedulerProperties().get(SchedulerPropertyKeys.CRON_EXPRESSION); Assert.hasText(schedule, "The property: " + SchedulerPropertyKeys.CRON_EXPRESSION + " must be defined"); - Container container = new ContainerCreator(this.kubernetesSchedulerProperties, scheduleRequest).build(); + Container container; + try { + container = new ContainerCreator(this.kubernetesSchedulerProperties, scheduleRequest).build(); + } + catch (ConstraintViolationException constraintViolationException) { + if (constraintViolationException.getMessage().contains("size must be between")) { + throw new CreateScheduleException(String.format("'%s' because the number of characters for the " + + "schedule name exceeds the Kubernetes maximum number of characters allowed for this field.", + scheduleRequest.getScheduleName()), constraintViolationException); + } + else + throw constraintViolationException; + } String taskServiceAccountName = KubernetesSchedulerPropertyResolver.getTaskServiceAccountName(scheduleRequest, this.kubernetesSchedulerProperties); @@ -190,4 +210,12 @@ private void setImagePullSecret(ScheduleRequest scheduleRequest, CronJob cronJob .add(localObjectReference); } } + + private boolean isScheduleNameTooLong(String message ) { + boolean result = false; + if(StringUtils.hasText(message) && message.contains("must be no more than") && message.endsWith("characters")) { + result = true; + } + return result; + } } diff --git a/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java b/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java index 684f04ab..6fa8efa5 100644 --- a/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java +++ b/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java @@ -228,6 +228,25 @@ public void testInvalidCronSyntax() { fail(); } + @Test + public void testNameTooLong() { + Map schedulerProperties = Collections.singletonMap(CRON_EXPRESSION, "0/10 * * * *"); + + AppDefinition appDefinition = new AppDefinition(randomName(), null); + ScheduleRequest scheduleRequest = new ScheduleRequest(appDefinition, schedulerProperties, null, null, + "tencharlng-scdf-itcouldbesaidthatthisislongtoowaytoo-oops", testApplication()); + + try { + scheduler.schedule(scheduleRequest); + } + catch (CreateScheduleException createScheduleException) { + assertThat(createScheduleException.getMessage()).contains("must be no more than"); + return; + } + + fail(); + } + @Test public void testWithExecEntryPoint() { KubernetesSchedulerProperties kubernetesSchedulerProperties = new KubernetesSchedulerProperties(); From 210191b747bf1c3708de20b0410749d167ff0152 Mon Sep 17 00:00:00 2001 From: Glenn Renfro Date: Fri, 13 Dec 2019 10:30:31 -0500 Subject: [PATCH 2/3] Updated based on code review --- .../spi/scheduler/kubernetes/KubernetesScheduler.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java index 9e0c4650..242c4a5f 100644 --- a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java +++ b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java @@ -86,7 +86,7 @@ public void schedule(ScheduleRequest scheduleRequest) { } invalidCronExceptionMessage = getExceptionMessageForField(e, SCHEDULE_METADATA_FIELD_NAME); - if (isScheduleNameTooLong(invalidCronExceptionMessage)) { + if (validationScheduleNameLength(invalidCronExceptionMessage)) { throw new CreateScheduleException(invalidCronExceptionMessage, e); } @@ -211,11 +211,7 @@ private void setImagePullSecret(ScheduleRequest scheduleRequest, CronJob cronJob } } - private boolean isScheduleNameTooLong(String message ) { - boolean result = false; - if(StringUtils.hasText(message) && message.contains("must be no more than") && message.endsWith("characters")) { - result = true; - } - return result; + private boolean validationScheduleNameLength(String message ) { + return (StringUtils.hasText(message) && message.contains("must be no more than") && message.endsWith("characters")); } } From 7abc86a305acb928e1a4f3abd1f09c8d4e602bfb Mon Sep 17 00:00:00 2001 From: Glenn Renfro Date: Mon, 16 Dec 2019 12:34:28 -0500 Subject: [PATCH 3/3] Now uses the existing infra for checking invalid schedule names. --- .../kubernetes/KubernetesScheduler.java | 29 +++---------------- .../kubernetes/KubernetesSchedulerTests.java | 12 ++++++-- 2 files changed, 13 insertions(+), 28 deletions(-) diff --git a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java index 242c4a5f..5ccf1d40 100644 --- a/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java +++ b/src/main/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesScheduler.java @@ -32,7 +32,6 @@ import io.fabric8.kubernetes.api.model.batch.CronJobList; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientException; -import javax.validation.ConstraintViolationException; import org.springframework.cloud.deployer.spi.scheduler.CreateScheduleException; import org.springframework.cloud.deployer.spi.scheduler.ScheduleInfo; @@ -55,8 +54,6 @@ public class KubernetesScheduler implements Scheduler { private static final String SCHEDULE_EXPRESSION_FIELD_NAME = "spec.schedule"; - private static final String SCHEDULE_METADATA_FIELD_NAME = "metadata.name"; - private final KubernetesClient kubernetesClient; private final KubernetesSchedulerProperties kubernetesSchedulerProperties; @@ -85,11 +82,6 @@ public void schedule(ScheduleRequest scheduleRequest) { throw new CreateScheduleException(invalidCronExceptionMessage, e); } - invalidCronExceptionMessage = getExceptionMessageForField(e, SCHEDULE_METADATA_FIELD_NAME); - if (validationScheduleNameLength(invalidCronExceptionMessage)) { - throw new CreateScheduleException(invalidCronExceptionMessage, e); - } - throw new CreateScheduleException("Failed to create schedule " + scheduleRequest.getScheduleName(), e); } } @@ -98,6 +90,9 @@ public void validateScheduleName(ScheduleRequest request) { if(request.getScheduleName() == null) { throw new CreateScheduleException("The name for the schedule request is null", null); } + if(request.getScheduleName().length() > 52) { + throw new CreateScheduleException(String.format("because Schedule Name: '%s' has too many characters. Schedule name length must be 52 characters or less", request.getScheduleName()), null); + } if(!Pattern.matches("^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", request.getScheduleName())) { throw new CreateScheduleException("Invalid Format for Schedule Name. Schedule name can only contain lowercase letters, numbers 0-9 and hyphens.", null); } @@ -153,19 +148,7 @@ protected CronJob createCronJob(ScheduleRequest scheduleRequest) { String schedule = scheduleRequest.getSchedulerProperties().get(SchedulerPropertyKeys.CRON_EXPRESSION); Assert.hasText(schedule, "The property: " + SchedulerPropertyKeys.CRON_EXPRESSION + " must be defined"); - Container container; - try { - container = new ContainerCreator(this.kubernetesSchedulerProperties, scheduleRequest).build(); - } - catch (ConstraintViolationException constraintViolationException) { - if (constraintViolationException.getMessage().contains("size must be between")) { - throw new CreateScheduleException(String.format("'%s' because the number of characters for the " + - "schedule name exceeds the Kubernetes maximum number of characters allowed for this field.", - scheduleRequest.getScheduleName()), constraintViolationException); - } - else - throw constraintViolationException; - } + Container container = new ContainerCreator(this.kubernetesSchedulerProperties, scheduleRequest).build(); String taskServiceAccountName = KubernetesSchedulerPropertyResolver.getTaskServiceAccountName(scheduleRequest, this.kubernetesSchedulerProperties); @@ -210,8 +193,4 @@ private void setImagePullSecret(ScheduleRequest scheduleRequest, CronJob cronJob .add(localObjectReference); } } - - private boolean validationScheduleNameLength(String message ) { - return (StringUtils.hasText(message) && message.contains("must be no more than") && message.endsWith("characters")); - } } diff --git a/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java b/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java index 6fa8efa5..cf4011aa 100644 --- a/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java +++ b/src/test/java/org/springframework/cloud/deployer/spi/scheduler/kubernetes/KubernetesSchedulerTests.java @@ -44,6 +44,7 @@ import io.fabric8.kubernetes.client.DefaultKubernetesClient; import io.fabric8.kubernetes.client.KubernetesClient; import io.fabric8.kubernetes.client.KubernetesClientException; +import javax.validation.ConstraintViolationException; import org.junit.AfterClass; import org.junit.ClassRule; import org.junit.Test; @@ -230,20 +231,25 @@ public void testInvalidCronSyntax() { @Test public void testNameTooLong() { + final String baseScheduleName = "tencharlng-scdf-itcouldbesaidthatthisislongtoowaytoo"; Map schedulerProperties = Collections.singletonMap(CRON_EXPRESSION, "0/10 * * * *"); AppDefinition appDefinition = new AppDefinition(randomName(), null); ScheduleRequest scheduleRequest = new ScheduleRequest(appDefinition, schedulerProperties, null, null, - "tencharlng-scdf-itcouldbesaidthatthisislongtoowaytoo-oops", testApplication()); + baseScheduleName, testApplication()); + //verify no validation fired. + scheduler.schedule(scheduleRequest); + + scheduleRequest = new ScheduleRequest(appDefinition, schedulerProperties, null, null, + baseScheduleName + "1", testApplication()); try { scheduler.schedule(scheduleRequest); } catch (CreateScheduleException createScheduleException) { - assertThat(createScheduleException.getMessage()).contains("must be no more than"); + assertThat(createScheduleException.getMessage()).isEqualTo(String.format("Failed to create schedule because Schedule Name: '%s' has too many characters. Schedule name length must be 52 characters or less", baseScheduleName + "1")); return; } - fail(); }