Entitle com.unboundid.ldap.listener as test package #130706
Conversation
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| } | ||
| return false; | ||
| idx = -idx - 2; // candidate package (insertion point - 1) | ||
| return idx >= 0 && idx < entitledTestPackages.length && packageName.startsWith(entitledTestPackages[idx]); |
There was a problem hiding this comment.
This could probably use a unit test, given the opportunity for off-by-one errors.
It inherits an existing bug where org.example.pack will be viewed as a prefix of org.example.package even though the two packages are unrelated. We somewhat got away with this before because it was a fixed set of packages known not to have this problem, but now that the API can add them dynamically, it becomes a possibility.
(We've overdue for writing a proper string prefix trie. 😅)
Also, I think our experience with FileAccessTree shows that this will fail for looking up org.example.foo if the array already contains org.example and org.example.bar. In this case, the binary search will land on org.example.bar which is not a prefix. In FileAccessTree we deal with this by removing entries that have a prefix already in the list.
There was a problem hiding this comment.
Thanks @prdoyle! I've added respective tests and fixed the issues mentioned above. Forbidding adding redundant entries here, which I think is sufficient for the use case.
| if (TestEntitlementBootstrap.isEnabledForTest()) { | ||
| TestEntitlementBootstrap.setActive(false == withoutEntitlements); | ||
| TestEntitlementBootstrap.setTriviallyAllowingTestCode(false == withEntitlementsOnTestCode); | ||
| if (entitledPackages != null) { | ||
| assert withEntitlementsOnTestCode == false : "Cannot use @WithEntitlementsOnTestCode together with @EntitledTestPackages"; |
There was a problem hiding this comment.
these contradict each other
There was a problem hiding this comment.
Hmm. Why is that? I can imagine a case where we want entitlements enforced on test code, but also want to grant certain test packages.
|
Also I think we're still backporting to 8.19? |
I did, but this (aka having to entitle more test packages) has come up multiple times now and in this case the required package is highly specific, so I'd rather extend the list of packages for specific tests only. |
mosche
added
auto-backport
| } | ||
|
|
||
| private static boolean isNotPrefixMatch(String name, String prefix, boolean discardExactMatch) { | ||
| assert prefix.endsWith(".") == false : "Invalid package prefix ending with '.' [" + prefix + "]"; |
There was a problem hiding this comment.
Some ideas:
Another approach might be to make the period at the end mandatory. Then a simple prefix match would work.
A third approach would be to add the period automatically to all prefixes. Again the simple prefix match would work.
There was a problem hiding this comment.
I thought about it. In either case, it shouldn't be lenient. So I either have to check the final dot is not there OR always require it to be there. Not much of a difference imho.
That makes a lot of sense. I bet most of the packages in the existing list could be moved into this annotation instead, and the list would then just have truly universal packages. |
|
Thx @prdoyle |
This removes
inbound_networkfrom x-pack core and instead configurescom.unboundid.ldap.listeneras test package for LDAP and related test cases that start an LDAP server listening for requests.Relates to ES-12128