Skip to content

Auto rules should offer matching on more than mainClass/targetAlias #540

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

Closed
9 tasks
andrewazores opened this issue Jun 24, 2021 · 7 comments · Fixed by #544
Closed
9 tasks

Auto rules should offer matching on more than mainClass/targetAlias #540

andrewazores opened this issue Jun 24, 2021 · 7 comments · Fixed by #544
Assignees
Labels
feat New feature or request

Comments

@andrewazores
Copy link
Member

andrewazores commented Jun 24, 2021

Rules should be able to match targets by more criteria than simply "same alias".

Examples of rule matches that should be possible:

  • "all targets"
  • "target has connectUrl 'service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi'"
  • "target has alias Foo"
  • "target has main class io.cryostat.Cryostat"
  • "target is listening on port 9999"
  • "target has label app=myproject"
  • "target has annotation cryostat.io/target=true"
  • "target has main class io.cryostat.Cryostat AND target has annotation cryostat.io/profiling=true"
  • "target has main class io.cryostat.Cryostat OR target has main class com.example.App"

Related: #280 #495 #413 #523

@andrewazores
Copy link
Member Author

andrewazores commented Jun 24, 2021

@ebaron I'm thinking that using JS to define the matching function for rule definitions might make sense, if we can sandbox the JS engine sensibly. It could look something like this:

{
  "name": "myrule",
  "description": "A rule with JS-defined matcher function",
  "eventSpecifier": "template=Continuous",
  "expression": "target.annotations.cryostat['JAVA_MAIN']=='io.cryostat.Cryostat' || target.annotations.cryostat['JAVA_MAIN']=='com.example.App'"
}

Generally, the expression field would just be an expression which evaluates to a boolean value, and has access to a ServiceRef (target) in its context.

If we don't sandbox the engine correctly however, this leaves things open for abuse.

{
  ...
  "expression": "java.lang.System.exit(1)"
}

or

{
  ...
  "expression": "while (true) continue"
}

etc.

Here are some preliminary resources I've come across while researching if this is a feasible avenue to explore:

https://stackoverflow.com/questions/20793089/secure-nashorn-js-execution/22268837#22268837
https://github.com/javadelight/delight-nashorn-sandbox / https://github.com/javadelight/delight-rhino-sandbox

Otherwise, we might want to implement our own parser (probably generated using a tool like JavaCC) to implement a similar system where rules allow users to define match expressions/functions using a simple language, by returning a boolean value when given a ServiceRef instance as context. This allows us to avoid the need for sandboxing entirely if we just don't allow for things like loops in the grammar and do not provide anything other than the ServiceRef instance for context, but it's much more work for us than just using Nashorn (or GraalJS, if/when we port over to Quarkus).

@andrewazores
Copy link
Member Author

andrewazores commented Jun 24, 2021

Another possibility:

{
  "name": "myrule",
  "description": "Rule with JSON match specification",
  "eventSpecifier": "template=Continuous",
  "match": {
    "kind": "AND",
    "operands": [
      {
        "kind": "EXPR",
        "path": "target.annotations.cryostat[\"JAVA_MAIN\"]",
        "value": "io.cryostat.Cryostat"
      },
      {
        "kind": "OR",
        "operands": [
          {
            "kind": "EXPR",
            "path": "target.labels[\"app\"]",
            "value": "myproject"
          },
          {
            "kind": "EXPR",
            "path": "target.annotations.platform[\"cryostat.io/target\"]",
            "value": "true"
          }
        ]
      }
    ]
  }
}

This just uses the JSON grammar, so all of the main lexing work is already done for us by Gson, but it leaves it up to us to implement parsing/verification of the syntax tree (maybe doable by Gson adapters/deserializers) to ensure it encodes a match expression, lightly parsing the path properties and matching those to fields of the ServiceRef, and then checking if those equal the expected value properties, and then evaluating the total expression result. This is maybe a bit less work than doing a whole parser-generator like JavaCC, but the rule definitions themselves totally explode when the match criteria is more than just checking the value of one field/property.

All this encodes the same expression as the JS:

target.annotations.cryostat['JAVA_MAIN'] == 'io.cryostat.Cryostat' && (target.labels['app'] == 'myproject' || target.annotations.platform['cryostat.io/target'] == 'true')'

@andrewazores
Copy link
Member Author

Otherwise, we might want to implement our own parser (probably generated using a tool like JavaCC) to implement a similar system where rules allow users to define match expressions/functions using a simple language, by returning a boolean value when given a ServiceRef instance as context. This allows us to avoid the need for sandboxing entirely if we just don't allow for things like loops in the grammar and do not provide anything other than the ServiceRef instance for context, but it's much more work for us than just using Nashorn (or GraalJS, if/when we port over to Quarkus).

Another idea: create a grammar for rule matching expressions, which happens to be a very small subset of JavaScript grammar. Generate a parser for this limited grammar and use it to validate matching expressions when Rule objects are created (by API POST or deserialized from disk). If the parsing succeeds, then pass on the original expression to the JavaScript engine and evaluate it, since it's now known to be safe.

The grammar would look something like:

RULE := EXPR(EXPROP EXPR)*

EXPROP := "&&" | "||"

EXPR := "target"(FIELD)+ OP LITERAL

FIELD := (IDENT | IDX)

IDENT := "."[a-zA-Z0-9\-_/.]+

IDX := "['"[IDENT]"']"

OP := "==" | "!-" | "===" | "!=="

LITERAL := [0-9] | "'"IDENT"'" | "true" | "false"

This allows expressions like "target.annotations.cryostat['JAVA_MAIN']=='io.cryostat.Cryostat' || target.annotations.cryostat['JAVA_MAIN']=='com.example.App'", or even simply true (match all targets), but does not allow for defining/invoking functions, loops, etc.

@andrewazores
Copy link
Member Author

https://docs.oracle.com/javase/9/docs/api/jdk/nashorn/api/tree/Parser.html

Nashorn also exposes the Parser API, which can give us the AST. We can then traverse the tree to validate it, failing the validation if any unexpected node type is found, so that validation only succeeds if the parsed AST only contains our desired restricted subset of node types.

@andrewazores
Copy link
Member Author

#544 uses Nashorn for JavaScript rule match expressions like cryostatio/cryostat#540 (comment) , and also uses the Nashorn Parser to validate Rule definitions and disallow things like function invocations, loops, try/catch, etc.

@ebaron
Copy link
Member

ebaron commented Jun 29, 2021

We may want to reconsider using Nashorn since it is deprecated in JDK 11 and removed in 15: https://openjdk.java.net/jeps/372

@andrewazores
Copy link
Member Author

Yea, though there is a standalone https://github.com/openjdk/nashorn dependency we can switch to if we upgrade to JDK15+ before some other alternative makes more sense (ex. polyglot Graal that comes with the Quarkus port)

@andrewazores andrewazores added feat New feature or request and removed enhancement labels Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants