upgrade kubernetes extension k8s client api jar to v16 #13175
Conversation
| import java.text.SimpleDateFormat; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.TimeZone; |
There was a problem hiding this comment.
Please don't change the import order. This is unnecessary.
And this is not the Druid' code style.
If you're using intellij, see: https://github.com/apache/druid/blob/master/dev/intellij-setup.md
There was a problem hiding this comment.
| return Config.defaultClient(); | ||
| final SimpleDateFormat dateFormat = new SimpleDateFormat( | ||
| "yyyyMMdd'T'HHmmss.SSS'Z'"); | ||
| dateFormat.setTimeZone(TimeZone.getTimeZone("UTC")); |
There was a problem hiding this comment.
Why is the timezone specified as UTC? What if users deploy Druid by setting -Duser.timezone to some other zones?
There was a problem hiding this comment.
I parsed out the exact timestamps that are read from the pod metadata. Where in the codebase is the formatter that sets that? I can simply use that format to match whatever the user has configured. I'll look and see if I can find it, but if you happen to know where to look I'd appreciate it.
There was a problem hiding this comment.
So the formatter is used to parse the date string that are returned from K8S server side? And the string is always in UTC format? If so, the change makes senses.
There was a problem hiding this comment.
It's the kind of thing that should be always UTC, since it's a server thing, not a user-facing or data-related timestamp.
There was a problem hiding this comment.
I need to confirm that this always originates in K8s, and is always consistent. It is likely better to allow this format string to be configured by the user, but to default the most common format.
|
Thanks for the contribution! @coryverta, for posterity could you please note what issue you were running into that this change fixed? One note. This will not actually fully address #12904, since there's some additional housekeeping needed to actually move something out of experimental status: doc updates and the like. IMO, it's best to do that separately anyway, perhaps one release from now so we have some more chance for production experience to roll in. |
coryverta
changed the title
… pod is metadata is non-null
There was a problem hiding this comment.
@coryverta I just noticed that this PR is raised against apache:24.0.1. Could you please re-raise it against master? We do all patches to master first, then port them to release branches as needed.
There is also some kind of compilation problem: https://lgtm.com/projects/g/apache/druid/logs/rev/pr-a2a67ae736a7253b7f263af823cc7f3b945d068f/lang:java/stage:Build%20merge_5c1b4c6978cbb2f6649e6f1310d8cdeb58469eab
[2022-10-04 20:17:36] [build-stdout] [2022-10-04 20:17:36] [autobuild] [ERROR] /opt/src/extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/DefaultK8sApiClient.java:[68,16] method patchNamespacedPod in class io.kubernetes.client.openapi.apis.CoreV1Api cannot be applied to given types;
[2022-10-04 20:17:36] [build-stdout] [2022-10-04 20:17:36] [autobuild] required: java.lang.String,java.lang.String,io.kubernetes.client.custom.V1Patch,java.lang.String,java.lang.String,java.lang.String,java.lang.String,java.lang.Boolean
[2022-10-04 20:17:36] [build-stdout] [2022-10-04 20:17:36] [autobuild] found: java.lang.String,java.lang.String,io.kubernetes.client.custom.V1Patch,java.lang.String,<nulltype>,<nulltype>,<nulltype>
[2022-10-04 20:17:36] [build-stdout] [2022-10-04 20:17:36] [autobuild] reason: actual and formal argument lists differ in length
[2022-10-04 20:17:36] [build-stdout] [2022-10-04 20:17:36] [autobuild] [INFO] 1 error
Something wrong with the new client version, perhaps?
|
NOTE: there is an underlying bug in the v16 kubernetes client API jar that is conflated with a bug involving pod deletions. I will close this specific PR and replace it with two, one that addresses the underlying bugs in the existing extendsion against client api v11.0.3, and then a follow-up to adopt v16. |
Applies to #12904.
Description
Kubernetes client API jar v.11.0.0 contains several critical security vulnerabilities. Adopting v16 requires two small changes to the existing code:
The date parsing in particular is necessary to handle the possible date format differences.
This PR has: