Validate Property Values by ddanielr · Pull Request #3177 · apache/accumulo

ddanielr · 2023-01-31T03:37:29Z

Closes #3145
These changes add the ability to validate system, namespace, and table-specific properties for accumulo.

Values are now being validated on the backend and client validation exceptions show as a ThriftPropertyException exception type.

See #3173 for WIP feedback if desired.

Refactor the `isTablePropertyValid` method to be more generic. This change moves the `isValidTablePropertyKey` method calls out from the `isTablePropertyValid` method and to the calling locations. This allows for the `isTablePropertyValid` method to be renamed to `isValidProperty`. Moving that method allowed for error messages to accurately discribe an invalid table property error and an invalid value error. Added 'throws' to method signatures in partial attempt to improve error visibilites.

Adds new `ThriftPropertyException` to allow for the `IllegalArgumentException` runtime expection to be caught and displayed to the client. Raise exceptions for setting invalid properties at the system, namespace, and table level.

Validate System properties as well as check if property is mutable in zookeeper.

Add tests for setting invalid system, namespace, and table properties. Change the error messages to be more uniform for test validation.

dlmarion · 2023-01-31T12:10:38Z

+          break;
+        case CLASSNAME:
+          Shell.log.debug("CLASSNAME properties currently fail this test");
+          Shell.log.debug("Regex used for CLASSNAME property types may need to be modified");


Should we fix the regex as part of this PR?

Currently, all of these changes don't alter property structure, they just enforce the previously defined types.

Modifying a property regex has the potential to break legitimate uses.
So, I'd rather open an issue for the regex fix and track that as a separate action.

dlmarion · 2023-01-31T12:16:12Z

Kicked off full IT build

dlmarion · 2023-01-31T18:25:42Z

Full IT build succeeded.

Change the `tableName` variable to use `getUniqueNames()` for uniformity.

EdColeman

Looks good - just some minor suggestions.

Remove unnecessary `toString()` calls on `propertyType`.

Add a property validation test in the `PropertyTest` class. This new test replaces the boolean-only validation test.

EdColeman

LGTM - maybe as a follow-on the tests in PropertyTest could be expanded to test variations of valid values. The boolean test that was modified was testing combinations of true, True, false, False, my initial though that it was unnecessary to loop through all of the properties, not the condition.

The testPropertyValidation test is a nice addition to check that each type will fail validation.

While the is a specific host:port test in PropertyTest, it may be an improvement if tests were included for things like 100M, 100m, 100G, 100g for memory, (or whatever is valid) for each of the expected types (assuming that they are not tested elsewhere)

EdColeman

Noticed that the check failed with a junit package issue:

Expected 0, but found 1 classes using the wrong JUnit APIs:
core/src/test/java/org/apache/accumulo/core/clientImpl/TabletLocatorImplTest.java
Error: Process completed with exit code 1.

Not sure where that is coming from, but it should be corrected before merging

EdColeman · 2023-02-01T14:52:42Z

The offending import in TabletLocatorImplTest is

import org.junit.Assert;

Also, there may be other checkstyle issues that need to be addressed:

[WARNING] src/main/java/org/apache/accumulo/core/clientImpl/TabletLocatorImpl.java:[356,9] (blocks) NeedBraces: 'if' construct must use '{}'s.
[WARNING] src/test/java/org/apache/accumulo/core/clientImpl/TabletLocatorImplTest.java:[61] (regexp) RegexpSinglelineJava: Use static imports for Assert.* methods for consistency

ddanielr · 2023-02-01T14:57:54Z

@EdColeman those changes weren't introduced in this PR.

I think the latest merge to 2.1 caused the problem
cb22cc4

EdColeman · 2023-02-01T15:12:43Z

I have a PR incoming to fix 2.1

EdColeman · 2023-02-01T15:48:05Z

Should be fixed when #3179 is merged

ddanielr · 2023-02-01T19:23:37Z

@EdColeman I merged in the latest changes from 2.1 and all tests are now passing.

Switch the logger from "debug" to "error" in SystemPropUtil Use test namespace instead of creating a new one in NamespacesIT Add a logger and remove `System.out.print` statements in PropertyTest.

EdColeman · 2023-02-02T14:43:03Z

      IllegalArgumentException iae =
          new IllegalArgumentException("Zookeeper property is not mutable: " + property);
-      log.debug("Attempted to set zookeeper property.  It is not mutable", iae);
+      log.error("Encountered error setting zookeeper property", iae);


Did you happen to look at the exceptions as printed in the logs? It seems likely that the method that catches the exception will also printout the stack trace. If there was additional info to help development, maybe a trace statement? Otherwise the logs may end up with two identical stack traces without adding additional information.

Also, error seems rather aggressive, services are going to keep running,

If this code get reworked, it may be worth considering.

(I realize this has been merged, it was changed, committed before I got back to it. Just wanted to mention it if it helps going forward)

@EdColeman confirmed that the calling method does in fact log the exceptions at the error level.
I've opened #3184 to revert this change.

ddanielr added 4 commits January 31, 2023 03:05

Add new thrift exception

844fce2

Adds new `ThriftPropertyException` to allow for the `IllegalArgumentException` runtime expection to be caught and displayed to the client. Raise exceptions for setting invalid properties at the system, namespace, and table level.

Validate system properties

98ec28c

Validate System properties as well as check if property is mutable in zookeeper.

Add tests for new thrift exception

c20e726

Add tests for setting invalid system, namespace, and table properties. Change the error messages to be more uniform for test validation.

ddanielr mentioned this pull request Jan 31, 2023

Check Prop Value Format and Key types. #3146

Closed

dlmarion reviewed Jan 31, 2023

View reviewed changes

Change tableName value assignment method

4e0347e

Change the `tableName` variable to use `getUniqueNames()` for uniformity.

dlmarion approved these changes Jan 31, 2023

View reviewed changes

EdColeman reviewed Jan 31, 2023

View reviewed changes

Comment thread test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java Outdated

EdColeman reviewed Jan 31, 2023

View reviewed changes

Comment thread test/src/main/java/org/apache/accumulo/test/shell/ShellIT.java Outdated

EdColeman reviewed Jan 31, 2023

View reviewed changes

Comment thread core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java Outdated

EdColeman reviewed Jan 31, 2023

View reviewed changes

ddanielr added 3 commits February 1, 2023 00:26

Remove unnecessary calls

45554c9

Remove unnecessary `toString()` calls on `propertyType`.

Add property validation test

368334b

Add a property validation test in the `PropertyTest` class. This new test replaces the boolean-only validation test.

Merge branch '2.1' into bugfix/property-validation

dbb0049

EdColeman approved these changes Feb 1, 2023

View reviewed changes

EdColeman requested changes Feb 1, 2023

View reviewed changes

Merge branch '2.1' into bugfix/property-validation

6756c3c

EdColeman approved these changes Feb 1, 2023

View reviewed changes

DomGarguilo reviewed Feb 1, 2023

View reviewed changes

Comment thread core/src/test/java/org/apache/accumulo/core/conf/PropertyTest.java Outdated

Comment thread server/base/src/main/java/org/apache/accumulo/server/util/SystemPropUtil.java Outdated

Comment thread test/src/main/java/org/apache/accumulo/test/NamespacesIT.java Outdated

Update tests and logger details

c6f33a3

Switch the logger from "debug" to "error" in SystemPropUtil Use test namespace instead of creating a new one in NamespacesIT Add a logger and remove `System.out.print` statements in PropertyTest.

dlmarion merged commit 39fbd1b into apache:2.1 Feb 2, 2023

dlmarion mentioned this pull request Feb 2, 2023

Shell commands allow for invalid Property types to be set #3145

Closed

dlmarion linked an issue Feb 2, 2023 that may be closed by this pull request

Shell commands allow for invalid Property types to be set #3145

Closed

EdColeman reviewed Feb 2, 2023

View reviewed changes

ddanielr deleted the bugfix/property-validation branch February 2, 2023 20:26

DomGarguilo mentioned this pull request Feb 2, 2023

Fix usage of Wait.waitFor() in ITs #3185

Merged

ctubbsii added this to the 2.1.1 milestone Jul 12, 2024

ctubbsii mentioned this pull request Jan 15, 2026

Audit Thrift RPC handlers to identify mismatched TException types #6061

Open

ctubbsii mentioned this pull request Jun 11, 2026

Audit and fix thrift RPC problems #6423

Open

Uh oh!

Conversation

ddanielr commented Jan 31, 2023

Uh oh!

dlmarion Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

ddanielr Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlmarion commented Jan 31, 2023

Uh oh!

dlmarion commented Jan 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EdColeman left a comment

Choose a reason for hiding this comment

Uh oh!

EdColeman left a comment

Choose a reason for hiding this comment

Uh oh!

EdColeman left a comment

Choose a reason for hiding this comment

Uh oh!

EdColeman commented Feb 1, 2023

Uh oh!

ddanielr commented Feb 1, 2023

Uh oh!

EdColeman commented Feb 1, 2023

Uh oh!

EdColeman commented Feb 1, 2023

Uh oh!

ddanielr commented Feb 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EdColeman Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

ddanielr Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants