add wildcard support for ServiceMemberSkipDomains #2202
Conversation
Signed-off-by: Shimaoka Shuya <[email protected]>
Signed-off-by: Shimaoka Shuya <[email protected]>
| } | ||
| // then, we conduct a perfect match search | ||
| if (skipDomain.equals(domainName)) { |
There was a problem hiding this comment.
this should be "else if" otherwise we're wasting operations comparing the same value which already contains * against a domain name that will never include *
There was a problem hiding this comment.
thank you. I've fixed it.
| if (!validateServiceMemberSkipDomains.contains(domainName)) { | ||
| if (dbService.getServiceIdentity(domainName, serviceName, true) == null) { | ||
| throw ZMSUtils.requestError("Principal " + memberName + " is not a valid service", caller); | ||
| for (String skipDomain : validateServiceMemberSkipDomains) { |
There was a problem hiding this comment.
validateServiceMemberSkipDomains was created as a Set to have simple contains check. if we're treating as a regular list instead, you should change the definition to be an ArrayList instead of a Set.
There was a problem hiding this comment.
thank you. I've fixed it.
Signed-off-by: Shimaoka Shuya <[email protected]>
Signed-off-by: Shimaoka Shuya <[email protected]>
Signed-off-by: Shimaoka Shuya <[email protected]>
|
I have added test cases. |
| @@ -830,7 +830,7 @@ void loadConfigurationSettings() { | |||
|
|
|||
| final String skipDomains = System.getProperty( | |||
| ZMSConsts.ZMS_PROP_VALIDATE_SERVICE_MEMBERS_SKIP_DOMAINS, ""); | |||
| validateServiceMemberSkipDomains = new HashSet<>(Arrays.asList(skipDomains.split(","))); | |||
| validateServiceMemberSkipDomains = new ArrayList<>(Arrays.asList(skipDomains.split(","))); | |||
There was a problem hiding this comment.
No need to create a new ArrayList<> here. Arrays.asList already does the job and returns an ArrayList. So just call it:
validateServiceMemberSkipDomains = Arrays.asList(skipDomains.split(","));
There was a problem hiding this comment.
I considered writing it that way, but Arrays.asList() and new ArrayList return different types. (ref)
As a result, there may be some confusion about this behavior in the future.
However, I am not too concerned about it, so I think Arrays.asList() is fine.
Thanks for pointing this out.
Signed-off-by: Shimaoka Shuya <[email protected]>
There was a problem hiding this comment.
@hiragi-gkuth one last request.
https://github.com/AthenZ/athenz/blob/master/servers/zms/conf/zms.properties#L434
can you update the documentation to indicate that the domain name can now include * wildcard at the end of the domain name to providing subdomain matching.
|
Thank you for your review. I've updated the documentation |
|
I forgot to commit with signed-off. I will rebase later. |
hiragi-gkuth
force-pushed
the
add-domain-prefix-check-for-service-validation
branch
from
8446c69 to
477703a
Compare
Signed-off-by: Shimaoka Shuya <[email protected]>
hiragi-gkuth
force-pushed
the
add-domain-prefix-check-for-service-validation
branch
from
477703a to
7a8fa34
Compare
resolve #2201
I'm going to add tests.