METRON-1336 Patching Can Result in Bad Configuration by nickwallen · Pull Request #851 · apache/metron

nickwallen · 2017-11-28T17:42:17Z

The following problems are addressed in this PR.

A patch can be constructed that when applied creates an invalid configuration. The invalid configuration is not discovered until attempting to "pull" it back out of Zookeeper. In all cases, the result of applying a patch needs to be validated before pushing it to Zookeeper.
The Profiler configuration can only be pushed when pushing all configurations at once. Attempting to push just the Profiler configuration alone, fails silently.
I do not believe that validation was occurring in all cases; in particular when pushing a specific configuration type (like pushing "Squid", PARSING alone.) Validation was occurring in other cases, like when all configuration elements are pushed at once.
Added unit tests to validate that a 'bad' patch cannot be applied.
Added a good number of unit tests to ensure invalid configurations cannot be pushed under all circumstances (parser, enrichment, indexing, profiler, "push all").
Added a test to ensure that the Profiler configuration can be pushed independently.

Manual Testing

Launch Full Dev.

Environment definitions that will be used later.

export METRON_HOME=/usr/metron/0.4.2
export ZOOKEEPER=node1:2181

Dump all existing configurations.

$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP

Dump just the Profiler configuration. There should be none.

$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER

Create a Profiler configuration on disk.

$ cat $METRON_HOME/config/profiler.json
{
  "profiles": [
    {
      "profile": "hello-world",
      "onlyif":  "exists(ip_src_addr)",
      "foreach": "ip_src_addr",
      "init":    { "count": "0" },
      "update":  { "count": "count + 1" },
      "result":  "count"
    }
  ]
}

Push the Profiler configuration.

$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PUSH -c PROFILER -i $METRON_HOME/config

Dump just the Profiler configuration. It should be defined now.

$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER

Create a patch for the Profiler configuration.

$ cat profile.patch
[
	{
		"op": "add",
		"path": "/profiles/0/profile",
		"value": "changed-profile-name"
	}
]

Apply the patch.

$METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c PROFILER -pf profile.patch

Dump the Profiler configuration and ensure that the name of the Profile was changed.

[root@node1 ~]# $METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m DUMP -c PROFILER
  PROFILER Config: profiler
  {
    "profiles" : [ {
      "profile" : "changed-profile-name",
      "onlyif" : "exists(ip_src_addr)",
      "foreach" : "ip_src_addr",
      "init" : {
        "count" : "0"
      },
      "update" : {
        "count" : "count + 1"
      },
      "result" : "count"
    } ]
  }

Create a patch that would make the Profiler configuration invalid.

[root@node1 ~]# cat bad.patch
  [
  	{
  		"op": "add",
  		"path": "/profiles/0/invalid",
  		"value": "22"
  	}
  ]

Apply the bad patch. The script should terminate with an error.

[root@node1 ~]# $METRON_HOME/bin/zk_load_configs.sh -z $ZOOKEEPER -m PATCH -c PROFILER -pf bad.patch
2017-11-28 17:37:17 ERROR ConfigurationManager:314 - Unable to apply patch to Zookeeper config
java.lang.RuntimeException: Unable to load {
  "profiles" : [ {
    "profile" : "changed-profile-name",
    "onlyif" : "exists(ip_src_addr)",
    "foreach" : "ip_src_addr",
    "init" : {
      "count" : "0"
    },
    "update" : {
      "count" : "count + 1"
    },
    "result" : "count",
    "invalid" : "22"
  } ]
}
	at org.apache.metron.common.configuration.ConfigurationType.lambda$static$4(ConfigurationType.java:68)
	at org.apache.metron.common.configuration.ConfigurationType.deserialize(ConfigurationType.java:93)
	at org.apache.metron.common.configuration.ConfigurationsUtils.applyConfigPatchToZookeeper(ConfigurationsUtils.java:621)
	at org.apache.metron.common.cli.ConfigurationManager.patch(ConfigurationManager.java:307)
	at org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:285)
	at org.apache.metron.common.cli.ConfigurationManager.run(ConfigurationManager.java:244)
	at org.apache.metron.common.cli.ConfigurationManager.main(ConfigurationManager.java:360)
Caused by: org.apache.metron.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "invalid" (class org.apache.metron.common.configuration.profiler.ProfileConfig), not marked as ignorable (8 known properties: "init", "expires", "groupBy", "profile", "foreach", "result", "onlyif", "update"])
 at [Source: {
  "profiles" : [ {
    "profile" : "changed-profile-name",
    "onlyif" : "exists(ip_src_addr)",
    "foreach" : "ip_src_addr",
    "init" : {
      "count" : "0"
    },
    "update" : {
      "count" : "count + 1"
    },
    "result" : "count",
    "invalid" : "22"
  } ]
  ...

Pull Request Checklist

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?

cestella

Good work here @nickwallen

cestella · 2017-11-30T21:03:04Z

.../metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java


  private static String getConfigZKPath(ConfigurationType configType, Optional<String> configName) {
-    String pathSuffix = configName.isPresent() && configType != GLOBAL ? "/" + configName : "";
+    String pathSuffix = configName.isPresent() && configType != GLOBAL ? "/" + configName.get() : "";


how did this ever work before? I'm surprised we didn't get the wrong result because configName is optional.

Yeah, this confused me too. There are a lot of paths through ConfigurationUtils. Some paths worked, others didn't.

We need to refactor ConfigurationUtils so badly that it hurts.

I think that's because we handle global config in a special way. This whole class needs refactored, and I'd have done more with the original patch PR had it not blown up scope.

Yeah, this is not one of those things that can be done as a side-effect of another PR. I considered refactoring it as part of the zookeeper refactoring that I did earlier. It needs more direct attention and refactoring.

cestella · 2017-11-30T21:13:15Z

.../metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java

-          writeConfigToZookeeper(type, configName, sensorIndexingConfigs.get(sensorType), client);
+
+      case PARSER: {
+        Map<String, byte[]> configs = readSensorConfigsFromFile(rootFilePath, PARSER, configName);


These cases look like they are cut and pasted which seems like code smell to me and might be a maintenance issue. Can we extract the common code for Parser, Enrichment, and Indexing into a separate function that is called here? Perhaps something like:

void writeSensorConfigs(ConfigurationType type, Optional<String> configName, BiFunction<String, byte[], Void> callback) { Map<String, byte[]> configs = readSensorConfigsFromFile(rootFilePath, type, configName); for(String sensorType : configs.keySet()) { byte[] configData = configs.get(sensorType); callback.apply(sensorType, configData); } }

which could be called from this case via writeSensorConfigs(PARSER, configName, (sensorType, configData) -> writeSensorParserConfigToZookeeper(sensorType, configData, client));

Take the above as a very rough suggestion and you can feel to abstract it however you wish.

An other option here is to add a new method called writeSensorConfigToZookeeper(ConfigurationType type, String sensorType, byte[] configData, CuratorFramework client) that calls the appropriate writeSensorXConfigToZookeeper call.

Oh, an even better option (IMO) would be to add a writeSensorConfigToZookeeper(String sensorType, byte[] configData, CuratorFramework client) method to ConfigurationType.
That would make this:

case PARSER: case ENRICHMENT: case INDEXING: Map<String, byte[]> sensorIndexingConfigs = readSensorConfigsFromFile(rootFilePath, type,configName); for (String sensorType : configs.keySet()) { byte[] configData = configs.get(sensorType); type.writeSensorConfigToZookeeper(sensorType, configData, client); }

Anyway, I'll stop ;)

Yes, I had the same thought and tried for a bit to refactor it. I landed on this because the various other ways to do this either (1) seemed more complex and less obvious as to what we are actually doing here or (2) lead down a path of heavy refactoring of ConfigurationUtils. Both of which i wanted to avoid

Your first suggestion with the callback just seems less obvious and clear to me (IMHO).

I'll try and think through your 2nd and 3rd suggestions, but (at least right now) I'm not seeing something that doesn't add more complexity to ConfigurationUtils.

But clearly I could be totally wrong here. Let me think on it.

@cestella I submitted a PR against your PR on this PR - cestella#8

@mmiklavc I'm ok with that modification if we decide to go this route.

I like the refactoring, guys. It's looking food. Can't believe we have a PR of a PR of this PR. One more level and I think we hit Inception's limbo.

I'd be open to including those changes here or even just creating a separate PR for the refactoring work as a follow-on to this. That refactoring adds a good bit of code in itself. Maybe we could even tackle some further ConfigurationUtils clean-up in that same PR.

But If we want @cestella and @mmiklavc 's refactoring to go in with this PR, then we need @cestella to do the first "kick" and merge @mmiklavc's PR.

Haha, I know. I couldn't resist seeing if GitHub actually allowed this sort of thing, and well, it was actually pretty straightforward. I don't care if we pull it all together or submit them separately. It sounds like we're all in agreement that the refactoring is a good idea, so if it was split them Casey's interface refactoring would go in immediately following with the necessary votes. I'll leave it to you both to decide.

The meta-PR is merged

…ctionality.

Refactor ConfigutionOperation implementations to their own classes

Proposed abstraction for writeSensorToZookeeper

nickwallen · 2017-12-04T19:24:46Z

@mmiklavc @cestella The kick is complete. All PRs merged. Ready for review.

ottobackwards

A couple of notes now that all the hard stuff is done.

ottobackwards · 2017-12-04T20:09:07Z

.../metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java

-  public static void uploadConfigsToZookeeper(String rootFilePath, CuratorFramework client,
-      ConfigurationType type, Optional<String> configName) throws Exception {
+  public static void uploadConfigsToZookeeper(
+          String rootFilePath,


When reading from file, the API ~~should explicitly require~~ would be more clear if it used file Paths as opposed to just strings.
It makes the api methods easier to understand when there are a bunch of similar methods.

Sure, but this method signature already existed. Do I need to fix that on this PR?

No. I guess I miss read the ambition of this pr, given the meta-pr and all ;)

Yeah I get it. ;). It wasn't my preference to include all of that here, but I bend to make people happy.

I am familiar with that ;)

I'll open a jira

METRON-1342

ottobackwards · 2017-12-04T20:16:11Z

.../metron-common/src/main/java/org/apache/metron/common/configuration/ConfigurationsUtils.java

-        JSONUtils.INSTANCE.toJSONPretty(patchedConfig), client);
+    byte[] prettyPatchedConfig = JSONUtils.INSTANCE.toJSONPretty(patchedConfig);
+
+    // ensure the patch produces a valid result; otherwise exception thrown during deserialization


Can we reword this? It needs to be clear that some things cannot be deserialized if they have an invalid configuration.

I'm not sure exactly what you're looking for. Can you suggest some specific text?

Never mind Nick, I didn't read this correctly, and I have the "serialization mixed with logical validation" stuff upfront in my head because of my work over in #856.

Sorry

Heh, well our meta-PR was to address a code change that resulted in duplication, not just a style preference. ;)

nickwallen · 2017-12-06T19:25:00Z

Bump. Anything else we need on this one?

cestella · 2017-12-06T20:09:44Z

I'm comfortable with this. +1 by inspection

mmiklavc · 2017-12-06T20:12:23Z

+1 by inspection. Nice work Nick.

nickwallen · 2017-12-06T23:05:14Z

Thanks guys. Nice work on the meta-PRs. I like how that turned out.

apache#851

nickwallen added 3 commits November 28, 2017 08:59

METRON-1336 Patching with Can Result in Bad Configuration

372fa3b

Fix bug that prevented Profiler config from being pushed independently

669e858

Fixed test case; was using invalid global and parser JSON

0a9d8c6

cestella suggested changes Nov 30, 2017

View reviewed changes

cestella and others added 6 commits November 30, 2017 17:02

Refactoring ConfigurationType to be more clear and add some write fun…

9a64ec3

…ctionality.

forgot license.

6127bd7

Cleaned up abstraction a bit.

ec16171

Refactor ConfigutionOperation implementations to their own classes

a4af45d

Merge pull request #8 from mmiklavc/METRON-1336-configtypemod-cleanup

fe3a6c4

Refactor ConfigutionOperation implementations to their own classes

Merge pull request #12 from cestella/METRON-1336-configtypemod

9534324

Proposed abstraction for writeSensorToZookeeper

ottobackwards reviewed Dec 4, 2017

View reviewed changes

asfgit closed this in b43b3cc Dec 6, 2017

iraghumitra pushed a commit to iraghumitra/incubator-metron that referenced this pull request Feb 17, 2018

METRON-1336 Patching Can Result in Bad Configuration (nickwallen) closes

f557d82

apache#851

nickwallen deleted the METRON-1336 branch September 17, 2018 19:21

Conversation

nickwallen commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual Testing

Pull Request Checklist

Uh oh!

cestella left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cestella Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cestella Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen commented Dec 4, 2017

Uh oh!

ottobackwards left a comment

Choose a reason for hiding this comment

Uh oh!

ottobackwards Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickwallen commented Dec 6, 2017

Uh oh!

cestella commented Dec 6, 2017

Uh oh!

mmiklavc commented Dec 6, 2017

Uh oh!

nickwallen commented Dec 6, 2017

Uh oh!

nickwallen commented Nov 28, 2017 •

edited

Loading

cestella Nov 30, 2017 •

edited

Loading

cestella Nov 30, 2017 •

edited

Loading

ottobackwards Dec 4, 2017 •

edited

Loading