Skip to content
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

WFCORE-1932 validate resource subset-match in interface definition #1901

Merged
merged 1 commit into from Mar 15, 2017

Conversation

soul2zimate
Copy link
Contributor

@soul2zimate soul2zimate commented Nov 4, 2016

@bstansberry
Copy link
Contributor

Thanks for these. :)

I don't think the exceptions should use the @Cause parameter though. Management ops should only be responding with server side stack traces when the operation handler doesn't know what happened and needs to just dump data. If it does know what happened, it should either just ignore the cause, or if the cause includes useful info the server doesn't understand, it should extract that text and incorporate it in it's own message. But the stack trace doesn't add anything.

In these validators, the server knows what happened.

In the case of UnknownHostException, the JDK creates the exception and the message can include useful data. So incorporating e.toString() in the OFE message helps. Just using e.getLocalizedMessage() isn't great because in the common case of new UnknownHostException(unknownhostname), getLocalizedMessage() just returns "unknownhostname". Calling e.toString() to include the exception class name in the message adds some context. In the case of the subnet IAEs, the server is the one generating the IAE, and the IAE itself is just an internal way of passing the failure message up the stack. There's no reason to call iae.toString() as the class name adds nothing. iae.getLocalizedMessage() provides all the relevant information.

* @author wangc based on work of @author rwinston@apache.org
*
*/
public class SubsetValidator extends StringLengthValidator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be "SubnetValidator"?

@bstansberry
Copy link
Contributor

Oops; I forgot an important point. The existence of InetAddressValidator confused me, but actually currently that class is not used.

The reason it is not used is because validation of interfaces needs to be done very carefully in a managed domain. The process handling the operation may not be the process that uses the value, and there very well could be different results between processes. For example, the DC can't be rejecting a hostname value that is unknown to it, because it's possible it's understood on the server that uses that interface definition.

Furthermore, even on a standalone server, the value could be invalid when added to the model but will be valid later when the server is started normally. For example, the server is being provisioned using the offline CLI, with the server in admin-only mode.

This is why we don't do this kind of Stage.MODEL validation. It is deferred to Stage.RUNTIME as part of NetworkInterfaceService.start().

However, the subnet validation seems safe, as there is nothing about that calculation that would vary between processes.

@soul2zimate
Copy link
Contributor Author

Thanks for the explanation, I did not figure out why there is an unused InetAddressValidator in the first place.
I think I will only keep the subnet validator(without @Cause param for log) as the address are checked in the Utility class ParsedInterfaceCriteria on runtime as you explained, and quote comment above to jira.

Should I just delete the obsolete InetAddressValidator ?

@soul2zimate soul2zimate changed the title WFCORE-1932 validate resource inet-address, loopback-address and subs… WFCORE-1932 validate resource subset-match in interface definition Mar 13, 2017
@soul2zimate
Copy link
Contributor Author

I have rebased this to only keep the subnet-match validation as explained above.
The unused InetAddressValidator remains untouched

@bstansberry bstansberry added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Mar 14, 2017
@bstansberry
Copy link
Contributor

@soul2zimate Sorry for letting this one sit for so long!

@bstansberry bstansberry merged commit eb60eb7 into wildfly:master Mar 15, 2017
@soul2zimate soul2zimate deleted the WFCORE-1932 branch March 15, 2017 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
2 participants