-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix SimpleKdcLdapServerTests by overriding java.locale.providers #60447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix SimpleKdcLdapServerTests by overriding java.locale.providers #60447
Conversation
Ensure date digits are always represented as ASCII chars.
|
Pinging @elastic/es-security (:Security/Authentication) |
pgomulka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, but I wanted to make sure we don't break anything for jdk8.
left a comment to clarify
x-pack/qa/evil-tests/build.gradle
Outdated
| } | ||
|
|
||
| unitTest { | ||
| systemProperty 'java.locale.providers','SPI,COMPAT' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen with jdk8 ? COMPAT is not known there. as per http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/util/locale/provider/LocaleProviderAdapter.java#l135 it should throw some exceptions (although when I tried to reproduce it it did not fail)
can we make this conditional?
Also, do we need SPI there? SPI options is not used in 6.8 by ES as we do not provide IsoCalendarProvider (because SPIs have to be shipped with jre)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I forgot that we talked about this. I'll make it conditional for java 9 and later.
As for SPI, you are right that it is not needed here, I am adding it here with the attempt to be consistent with:
elasticsearch/gradle/ide.gradle
Lines 74 to 76 in 0299031
| defaults(JUnit) { | |
| vmParameters = '-ea -Djava.locale.providers=SPI,COMPAT' | |
| } |
|
Thanks @pgomulka I have updated the PR as suggested. |
pgomulka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thank you for looking into this @ywangd
The JDK system property makes sure date digits are always represented as ASCII chars.
This in turn helps the Kerberos response to be correctly encoded and recognised by the client.
Resolves: #57749