spring-cloud / spring-cloud-deployer-kubernetes

The Spring Cloud Deployer implementation for Kubernetes
Apache License 2.0
157 stars 98 forks source link

PropertyParserUtils is not able to parse comma separated list of values for the same annotaiton. #479

Closed ckings45 closed 2 years ago

ckings45 commented 2 years ago

The PropertyParserUtils#getStringPairsToMap is written in such a way that it is unable to parse a comma separated list of values for the same annotation.

I have a Spring Cloud Data Flow Server up and running. I tried launching a task using the Spring Cloud Java Task DSL using the following deployment properties (I am trying to launch "myTask" pod with the annotation "validated.keys" with a value of "key1,key2"):

Map<String, String> taskLaunchProperties = new DeploymentPropertiesBuilder()                         
   .put("deployer.mytask.kubernetes.jobAnnotations", "validated.keys:key1,key2").build();

This results in the following exception on the client side:

org.springframework.web.client.HttpServerErrorException$InternalServerError: 500 : [[{"logref":"IllegalArgumentException","message":"Invalid annotation value: key2","links":[]}]]

The reason for the above exception is the following line in PropertyParserUtils#getStringPairsToMap:

Assert.isTrue(splitString.length == 2, String.format("Invalid annotation value: %s", pair));

My goal is to pass comma separated list of values to the same annotation. This information is required by one of our internal frameworks within our organization. Failing to provide this comma separated list of values for a pod will block the pod from being launched. How can I pass a comma separated list of values for a single annotation when using Spring Cloud Data Flow (Which internally uses spring cloud deployer)

More Details: Spring Cloud Data Flow Server version : 2.5.3.RELEASE Question also posted on StackOverflow : Spring Cloud Data Flow : Unable to pass comma separated list for job annotation

cppwfs commented 2 years ago

Can you retry using the latest release 2.9x of Data Flow? Version 2.5.3 is no longer supported.

ckings45 commented 2 years ago

@cppwfs Thanks for your response. We tried using the latest version of SCDF server (2.9.2). The issue is reproducible with version 2.9.2 as well.

Here is the stack trace from the Spring Cloud Data Flow Server Pod : Once again - The issue seems to be in the PropertyParserUtils#getStringPairsToMap method. It cannot parse the value validated.keys:key1,key2 because it expects only one value per annotation. It does not like the fact that a comma is present in the annotation value.

java.lang.IllegalArgumentException: Invalid annotation value: key2"' at org.springframework.util.Assert.isTrue(Assert.java:121) ~[spring-core-5.3.14.jar!/:5.3.14] at org.springframework.cloud.deployer.spi.kubernetes.support.PropertyParserUtils.getStringPairsToMap(PropertyParserUtils.java:46) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.DeploymentPropertiesResolver.getJobAnnotations(DeploymentPropertiesResolver.java:701) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.KubernetesTaskLauncher.launch(KubernetesTaskLauncher.java:283) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.KubernetesTaskLauncher.launch(KubernetesTaskLauncher.java:120) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.dataflow.server.service.impl.DefaultTaskExecutionService.executeTask(DefaultTaskExecutionService.java:402) ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2] at org.springframework.cloud.dataflow.server.service.impl.DefaultTaskExecutionService$$FastClassBySpringCGLIB$$422cda43.invoke() ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2]

cppwfs commented 2 years ago

Thank you for upgrading to 2.9.2. I'll update the label for the issue.

cppwfs commented 2 years ago

@ckings45 I just tested this scenario using the sample project here and was unable to reproduce. Can you take a peak at my example and see what I missed. Thanks.

ckings45 commented 2 years ago

@cppwfs I believe that the issue is not reproducible because your client is pointing to a local SCDF server instance which internally results in the usage of LocalTaskLauncher. Can you try pointing to an SCDF server instance deployed on k8s so that the KubernetesTaskLauncher gets used instead? My understanding is that the issue occurs when we try to launch a task on k8s.

cppwfs commented 2 years ago

It was pointing to a SCDF instance that was running in a minikube that was using the kubernetes deployer.

ckings45 commented 2 years ago

@cppwfs I used your sample client code and pointed to our SCDF server pod hosted on our enterprise OpenShift platform. I am still able to reproduce the error.

What do we know so far?

  1. It is quite evident from the testAnnotationParseInvalidValue unit test in PropertyParserUtilsTest that it does not support comma separated values for the same annotation.
  2. It is quite evident from the SCDF server logs that PropertyParserUtils#getStringPairsToMap is being called by the SCDF server and it is unable to parse comma separated values for the same annotation.

Looking at 1 and 2 above, we can conclude that PropertyParserUtils#getStringPairsToMap needs to be fixed to support comma separated values for the same annotation?

We could perhaps work backwards from PropertyParserUtils.java to understand why PropertyParserUtils and the KubernetesTaskLauncher are not called by your SCDF server; however, I would say that the JUnits and the server logs I have provided are quite conclusive of the root cause. There is a clear need to refactor PropertyParserUtils.java to support comma separated values for the same annotation.

Error message : on client

HTTP exception while trying to launch the task . Root cause : {"_embedded":{"errors":[{"message":"Invalid annotation value: key2","logref":"IllegalArgumentException","_links":{"self":{"href":"/"}}}]}}

Error message on the server

java.lang.IllegalArgumentException: Invalid annotation value: key2 at org.springframework.util.Assert.isTrue(Assert.java:121) ~[spring-core-5.3.15.jar!/:5.3.15] at org.springframework.cloud.deployer.spi.kubernetes.support.PropertyParserUtils.getStringPairsToMap(PropertyParserUtils.java:46) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.DeploymentPropertiesResolver.getJobAnnotations(DeploymentPropertiesResolver.java:701) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.KubernetesTaskLauncher.launch(KubernetesTaskLauncher.java:283) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.deployer.spi.kubernetes.KubernetesTaskLauncher.launch(KubernetesTaskLauncher.java:120) ~[spring-cloud-deployer-kubernetes-2.7.2.jar!/:2.7.2] at org.springframework.cloud.dataflow.server.service.impl.DefaultTaskExecutionService.executeTask(DefaultTaskExecutionService.java:402) ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2] at org.springframework.cloud.dataflow.server.service.impl.DefaultTaskExecutionService$$FastClassBySpringCGLIB$$422cda43.invoke() ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2] at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:218) ~[spring-core-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.invokeJoinpoint(CglibAopProxy.java:783) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:753) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123) ~[spring-tx-5.3.15.jar!/:5.3.15] at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:388) ~[spring-tx-5.3.15.jar!/:5.3.15] at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119) ~[spring-tx-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.CglibAopProxy$CglibMethodInvocation.proceed(CglibAopProxy.java:753) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:698) ~[spring-aop-5.3.15.jar!/:5.3.15] at org.springframework.cloud.dataflow.server.service.impl.DefaultTaskExecutionService$$EnhancerBySpringCGLIB$$72a9e518.executeTask() ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2] at org.springframework.cloud.dataflow.server.controller.TaskExecutionController.launch(TaskExecutionController.java:193) ~[spring-cloud-dataflow-server-core-2.9.2.jar!/:2.9.2]

ckings45 commented 2 years ago

@cppwfs Thanks for acknowledging the issue and getting it fixed. I see that the issue is closed. Is there somewhere we can track the availability of this fix and the release in which it will be available?

ckings45 commented 2 years ago

@cppwfs Apologies for my ignorance but is there a way I can find out what release will this fix be a part of? I saw that you mentioned the link to the release calendar in another issue that I raised (https://calendar.spring.io/); however, I am not completely sure how I can track the above issue on this Calendar and find out when the fix in PropertyParseUtils would be available and for which version..

cppwfs commented 2 years ago

It will be in SCDF 2.10