Skip to content

Add REST accessor for cluster message/state constraints - #217

Open
LZD-PratyushBhatt wants to merge 1 commit into
devfrom
lzd/message-constraint-rest
Open

Add REST accessor for cluster message/state constraints#217
LZD-PratyushBhatt wants to merge 1 commit into
devfrom
lzd/message-constraint-rest

Conversation

@LZD-PratyushBhatt

@LZD-PratyushBhatt LZD-PratyushBhatt commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Operators previously had to hand-edit the CONSTRAINT ZNodes (/{cluster}/CONFIGS/CONSTRAINT/{type}) to add a message constraint, which is error prone during an incident. This adds a ConstraintAccessor exposing CRUD over individual constraint items:

  GET    /clusters/{clusterId}/constraints/{constraintType}
  GET    /clusters/{clusterId}/constraints/{constraintType}/{constraintId}
  PUT    /clusters/{clusterId}/constraints/{constraintType}/{constraintId}
  DELETE /clusters/{clusterId}/constraints/{constraintType}/{constraintId}

PUT accepts a flat JSON map of constraint attributes and delegates to HelixAdmin.setConstraint, validating the constraint type, attribute keys, and CONSTRAINT_VALUE the same way the core loader does. The accessor is auto-registered via the existing package scan. Adds TestConstraintAccessor covering create/get/delete, multiple items of the same type, and the invalid-type, missing-cluster, and invalid-body error paths.

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)

Description

  • Here are some details about my PR, including screenshots of any UI changes:

(Write a concise description including what, why, how)

Tests

  • The following tests are written for this issue:

(List the names of added unit/integration tests)

  • The following is the result of the "mvn test" command on the appropriate module:

(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)

Changes that Break Backward Compatibility (Optional)

  • My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:

(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)

Documentation (Optional)

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

Operators previously had to hand-edit the CONSTRAINT ZNodes
(/{cluster}/CONFIGS/CONSTRAINT/{type}) to add a message constraint,
which is error prone during an incident. This adds a ConstraintAccessor
exposing CRUD over individual constraint items:

  GET    /clusters/{clusterId}/constraints/{constraintType}
  GET    /clusters/{clusterId}/constraints/{constraintType}/{constraintId}
  PUT    /clusters/{clusterId}/constraints/{constraintType}/{constraintId}
  DELETE /clusters/{clusterId}/constraints/{constraintType}/{constraintId}

PUT accepts a flat JSON map of constraint attributes and delegates to
HelixAdmin.setConstraint, validating the constraint type, attribute keys,
and CONSTRAINT_VALUE the same way the core loader does. The accessor is
auto-registered via the existing package scan. Adds TestConstraintAccessor
covering create/get/delete, multiple items of the same type, and the
invalid-type, missing-cluster, and invalid-body error paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Path("{constraintType}/{constraintId}")
@ApiOperation(value = "Create or overwrite a constraint item",
notes = "Helix REST Constraints Put API")
public Response setConstraint(@PathParam("clusterId") String clusterId,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we should add validation here on the MESSAGE_TYPE, STATE_TRANSITION and CONSTRAINT_VALUE fields to prevent users from adding wrong values.... for instance constraint_value >=0... basically the idea is the body of the message to be set on ZNode should be prevalidated

public Response setConstraint(@PathParam("clusterId") String clusterId,
@PathParam("constraintType") String constraintTypeStr,
@PathParam("constraintId") String constraintId, String content) {
if (!doesClusterExist(clusterId)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One more thing we should think about is if we can also provide support for a batch API, suppose I want to add constraints for multiple instances in one go, with the current structure, I'll have to run the API again and again, can we do an atomic batch operation instead and support batch API instead?

@sjainit

sjainit commented Aug 5, 2026

Copy link
Copy Markdown

Add a sample helix-rest curl in the PR description for this operation and lets update the helix-rest documentation as well for this... i'll moving it to helix-docs soon

@sjainit sjainit left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Attribute values are regex-matched, not literal — this affects the prevalidation you asked for. Constraint matching uses messageValue.matches(constraintValue) in ConstraintItem.match, i.e. the stored attribute value is used as the regular expression argument of String.matches. So INSTANCE/RESOURCE/TRANSITION/STATE values are treated as patterns, not literals — a literal instance name like lva2-app58760.prod.linkedin.com_15088 happens to match itself, but the .s are wildcards and any regex metacharacter in a name would misbehave. Any value validation should therefore either escape/anchor these, or explicitly document that values are patterns.

}

ConstraintItemBuilder builder = new ConstraintItemBuilder();
builder.addConstraintAttributes(attributes);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This call (and builder.build() below) is outside any try/catch, so a syntactically-valid body with a null value returns HTTP 500 instead of 400.

Example: {"MESSAGE_TYPE":"STATE_TRANSITION","CONSTRAINT_VALUE":null} → Jackson yields a non-empty map {…, CONSTRAINT_VALUE=null} (so it passes the isEmpty() guard above) → ConstraintValue.valueOf(null) throws NullPointerException. That's not an IllegalArgumentException, so ConstraintItemBuilder's internal catch doesn't cover it, the NPE escapes this unguarded call, and Jersey maps it to 500. Verified end-to-end.

Suggest guarding it so bad input is a clean 400:

ConstraintItem item;
try {
  builder.addConstraintAttributes(attributes);
  item = builder.build();
} catch (RuntimeException e) {
  return badRequest("Invalid constraint attributes: " + e.getMessage());
}

This also avoids writing null-valued attributes into the ZNode (a non-CONSTRAINT_VALUE key with a null value doesn't throw, but currently gets stored). Please add a regression test (CONSTRAINT_VALUE:null → 400) to lock it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants