from:"justinleet"

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/500
  
"confident this PR", not "confident that".  In retrospect, it sounds like 
I'm referring to your last comment, not just in general.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #500: METRON-795: Install Metron REST with Ambari MPa...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/500
  
@merrimanr I'm pretty confident that is going to collide unpleasantly with 
https://github.com/apache/incubator-metron/pull/518 (The MPack should function 
in a kerberized cluster).

I haven't thought through the details, but do you think it's potentially 
sufficient to make this component optional (cardinality 0+, rather than 1)?  
It's an extra step for the user, but it's only selecting it on the component 
list.  At that point, we say the REST API + UI is only supported on 
non-kerberized clusters (which is true anyway), and we (hopefully) don't break 
Kerberizing the core components.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #518: METRON-799: The MPack should function in a kerb...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/518
  
Full dev is able to successfully startup, be Kerberized via Ambari without 
errors (including Storm service check), and have new data flow through.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #518: METRON-799: The MPack should function in a kerb...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/518
  
Still need to do some additional testing (I'm spiin, but this is updated to 
pull the topology.auto-credentials down to the topology level.

This should let Storm actually run service check (because AutoTGT isn't 
initializing).  At the same time, Metron should be able to set these 
appropriately and run them.   This involves passing the config down to the 
various files (the .properties files that feed Flux) and updating integration 
tests slightly.

Oddly enough, the use of the tickets in the client_jaas file caused issues, 
so it's now using the ticket cache and running a kinit before acting on 
topologies.  I strongly dislike having the kinits, but there's no obvious 
reason for the difference in behavior between ticket cache and keytab.




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #518: METRON-799: The MPack should function in a kerb...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/518
  
So coming back to the AutoTGT discussion and making it easier to see.  
There's basically two approaches we can takes that (should) work.

1. The current use of AutoTGT.  This requires setting up .storm/storm.yaml 
for the Storm nodes, and users will have to do the same.
2. The AutoHDFS / AutoHBase solution mentioned by @dlyle65535.  This 
requires symlinking some jars and configs to make them available to Storm.  
Should only be necessary for Nimbuses, but it does mean that HDFS/HBase 
upgrades can make the symlinks stale.

Either solution requires Ambari acting on a different node (potentially) 
than the one running the scripts.  I don't know if Ambari has any resources for 
handling that sort of things.  It could potentially be a command over ssh to 
another node, but presumably that requires passwordless ssh setup or someone to 
manually create the symlinks (which may be acceptable for this pass).

I'm fairly strongly inclined towards the second one, primarily because it 
requires less effort on the users part.  Ambari work is fairly similar either 
way.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110413918
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
+File(metron_storm_dir + '/storm.yaml',
+ content=Template('storm.yaml.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

Yeah, you're right, you should be able to change it via HA.  I just meant I 
can't go and change it to something in full dev.

Re: the AutoTGT stuff
I'm unsure if HA Storm would be an issue, but assuming Storm handles 
spinning up secure nimbuses correctly it shouldn't be.  New nimbuses are 
required to make sure they get all code of active topologies.  I would assume 
this includes getting any TGTs (although I haven't verified this) to ensure 
that active topologies can continue to run.  We'd need to test on an actual HA 
cluster.

I disagree with AutoHDFS/AutoHBase being less complex, because if I recall 
correctly (and I don't, please correct me), that solution required setting up 
symlinks on each Storm node.  I don't even know that we easily have that 
capability in Ambari as the Metron service. Even if we do work around that, I 
don't see AutoTGT as complicated, and the Storm docs mention "On a kerberos 
secure cluster they should be set by default to point to 
org.apache.storm.security.auth.kerberos.AutoTGT."  It seems reasonable to go 
with Storm's recommendation unless we have a compelling reason not to.

The custom storm.yaml only contains 3 configs that we need to run
1. Nimbus seeds (gathered from Storm configs themselves)
2. Jaas file
3. Thrift protocol (which admittedly doesn't grab correctly from Storm, but 
is essentially a constant for the purpose of Kerberos).

Of these, only the jaas file really matters for management, and all it does 
it define named stanzas and it's not anything particularly complicated.

Having said that, the Storm service check could make AutoTGT a lot less 
attractive if it's not easily workable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110411025
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
+File(metron_storm_dir + '/storm.yaml',
+ content=Template('storm.yaml.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

It's not possible to change it via Ambari, but it will populate the 
variable appropriately.  There's some translation magic happening somewhere, 
because Ambari lists the value of nimbus.seeds as 'node1', but it actually 
passes it down as a list (which is why the new commit doesn't surround it with 
square brackets). I'm guessing this magic is also why the thrift config doesn't 
get passed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110405590
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
+File(metron_storm_dir + '/storm.yaml',
+ content=Template('storm.yaml.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

nimbus.seeds is updated to grab the actual value.  Oddly, 
storm.thrift.transport doesn't end up flowing correctly to the template.  I 
double checked all the spelling and so on, and it always ended up just being 
the variable, not the value.  I'm inclined to let that one go, since it's only 
even created in a Kerberized environment anyway and it's a constant.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #518: METRON-799: The MPack should function in a kerb...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/518
  
@dlyle65535 I'll take a look at nimbus.admins.  I'm pretty confident it's 
running as Storm user though,
Partial output from Ambari:
```
Traceback (most recent call last):
 ...
user=params.storm_user
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110388704
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

My understanding, and correct me if I'm wrong, is that nimbus distributed 
and renews the TGT from Kerberos.  I'm honestly not sure how we'd get 
everything to line up otherwise if we have to distribute keytabs and jaas files 
and everything else out.

This does raise the question of what happens when the max renewal of a 
Kerberos ticket has passed. I don't know enough about the implementation to 
know what happens, and I'd be interested in thoughts.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110385969
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/enrichment_commands.py
 ---
@@ -131,47 +153,35 @@ def init_geo(self):
 self.set_geo_configured()
 
 def init_kafka_topics(self):
-Logger.info('Creating Kafka topics')
-command_template = """{0}/kafka-topics.sh \
---zookeeper {1} \
---create \
---topic {2} \
---partitions {3} \
---replication-factor {4} \
---config retention.bytes={5}"""
-num_partitions = 1
-replication_factor = 1
-retention_gigabytes = int(self.__params.metron_topic_retention)
-retention_bytes = retention_gigabytes * 1024 * 1024 * 1024
-
-Logger.info("Creating topics for enrichment")
-topics = [self.__enrichment_topic]
-for topic in topics:
-Logger.info("Creating topic'{0}'".format(topic))
-Execute(command_template.format(self.__params.kafka_bin_dir,
-self.__params.zookeeper_quorum,
-topic,
-num_partitions,
-replication_factor,
-retention_bytes))
-
-Logger.info("Done creating Kafka topics")
+Logger.info('Creating Kafka topics for enrichment')
+# All errors go to indexing topics, so create it here if it's not 
already
+metron_service.init_kafka_topics(self.__params, 
[self.__enrichment_topic, self.__params.metron_error_topic])
 self.set_kafka_configured()
 
+def init_kafka_acls(self):
+Logger.info('Creating Kafka topics')
--- End diff --

Yes, yes I did.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110385783
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/indexing_commands.py
 ---
@@ -72,55 +93,46 @@ def remote_repo():
 raise ValueError("Unsupported repo type 
'{0}'".format(repo_type))
 
 def init_kafka_topics(self):
-Logger.info('Creating Kafka topics')
-command_template = """{0}/kafka-topics.sh \
---zookeeper {1} \
---create \
---topic {2} \
---partitions {3} \
---replication-factor {4} \
---config retention.bytes={5}"""
-num_partitions = 1
-replication_factor = 1
-retention_gigabytes = int(self.__params.metron_topic_retention)
-retention_bytes = retention_gigabytes * 1024 * 1024 * 1024
-Logger.info("Creating topics for indexing")
-
-Logger.info("Creating topic'{0}'".format(self.__indexing))
-Execute(command_template.format(self.__params.kafka_bin_dir,
-self.__params.zookeeper_quorum,
-self.__indexing,
-num_partitions,
-replication_factor,
-retention_bytes))
-Logger.info("Done creating Kafka topics")
+Logger.info('Creating Kafka topics for indexing')
+metron_service.init_kafka_topics(self.__params, [self.__indexing])
+
+def init_kafka_acls(self):
+Logger.info('Creating Kafka ACLs')
+# Indexed topic names matches the group
+metron_service.init_kafka_acls(self.__params, [self.__indexing], 
[self.__indexing])
 
 def init_hdfs_dir(self):
-Logger.info('Creating HDFS indexing directory')
+Logger.info('Setting up HDFS indexing directory')
+
+# Non Kerberized Metron runs under 'storm', requiring write under 
the 'hadoop' group.
+# Kerberized Metron runs under it's own user.
+ownership = 0755 if self.__params.security_enabled else 0775
+Logger.info('HDFS indexing directory ownership is: ' + 
str(ownership))
 
self.__params.HdfsResource(self.__params.metron_apps_indexed_hdfs_dir,
type="directory",
action="create_on_execute",
owner=self.__params.metron_user,
group=self.__params.hadoop_group,
--- End diff --

I decided not to mess with it.  If we have a preference on it not being 
owned by metron:hadoop, we can go ahead and do that, but I think we probably 
need a more thorough discussion of how we want all that owned and permissioned 
anyway.  Leaving it only readable seemed like a reasonable compromise for now. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110385480
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

All the stuff in (or referenced in the case of client_jaas) ~metron/.storm 
should just need to be on the Metron node, because everything gets kicked off 
there.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/518#discussion_r110385183
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_security.py
 ---
@@ -0,0 +1,74 @@
+"""
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+"""
+
+import os.path
+from resource_management.core.source import Template
+from resource_management.core.resources.system import Directory, File
+from resource_management.core import global_lock
+from resource_management.core.logger import Logger
+from resource_management.core.resources.system import Execute
+from resource_management.libraries.functions import format as ambari_format
+
+
+# Convenience function for ensuring home dirs are setup consistently.
+def storm_security_setup(params):
+if params.security_enabled:
+# I don't think there's an Ambari way to get a user's local home 
dir , so have Python perform tilde expansion.
+# Ambari's Directory doesn't do tilde expansion.
+metron_storm_dir_tilde = '~' + params.metron_user + '/.storm'
+metron_storm_dir = os.path.expanduser(metron_storm_dir_tilde)
+Directory(metron_storm_dir,
+  mode=0755,
+  owner=params.metron_user,
+  group=params.metron_group
+  )
+
+File(ambari_format('{client_jaas_path}'),
+ content=Template('client_jaas.conf.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
+File(metron_storm_dir + '/storm.yaml',
+ content=Template('storm.yaml.j2'),
+ owner=params.metron_user,
+ group=params.metron_group,
+ mode=0755
+ )
+
--- End diff --

Just the properties in here.  Couple thoughts as you bring this up.

We probably want to have a ticket to make sure turning off Kerberos works 
correctly in the future.  The properties in the file (except nimbus.seeds) are 
essentially set properties.  We need our own client_jaas, and the 
storm.thrift.transport has to be there for some reason and that's pretty much 
constant on a secure cluster.  That should also be made a property that flows 
down from Storm.

nimbus.seeds is wrong and I need to carry that over from the actual Storm 
property. And I even made sure it was in params_linux and forgot to use it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #518: METRON-799: The MPack should function in...

GitHub user justinleet opened a pull request:

https://github.com/apache/incubator-metron/pull/518

METRON-799: The MPack should function in a kerberized cluster

## Contributor Comments
Allows the Ambari Kerberos wizard to handle Metron setup.

Changes include:
- Creation of Keytabs
- Running everything as the Metron user, including Storm topology workers
(on a Kerberized cluster).
- Setup for Metron user to actually be able to run (client_jaas setup, home
Storm dir setup, etc.)
- Adjusting perms to 0755. The exception is the HDFS output folder on a
non-kerb cluster is left as 0775 because we don't have Storm running workers as
metron user on. When Kerberizing, the permissions will be restricted down to
0755.
- Kafka ACLs
- HBase ACLs
- Refactored Topic creation to use a common function so I didn't have to
edit the same thing 3 times.
- Automated updating of Storm configs (the AutoTGT and running workers as
user)

There's still more testing I want to do, but this is definitely far enough
along to submit a PR.

I've spun this up on full dev with the now modified Kerberos setup
instructions, with the caveat that Ambari's Storm service check fails (it's
harmless, as far as I can tell). See below for more details. As this does not
touch the sensors, data will need to be pushed manually (same as the old
instructions). I've been able to push data from Kafka to Elasticsearch/HDFS.

### The Bad News
I would love insight on a problem, if anybody has some. I haven't edited
the docs to reflect this yet, in the hopes it'll be resolved.

Storm's service check will fail during (and after) Kerberization. Metron
can immediately be started perfectly fine. Nothing is legit wrong, but this
setup means that the storm user is unable to submit to the cluster (it doesn't
have it's home directory setup with some configs). Unfortunately, Ambari runs
the service check as the storm user.

This can be worked around by creating ~storm/.storm/storm.yaml
```
nimbus.seeds : ['node1']
java.security.auth.login.config :
'/usr/hdp/current/storm-supervisor/conf/storm_jaas.conf'
storm.thrift.transport :
'org.apache.storm.security.auth.kerberos.KerberosSaslTransportPlugin
```
`java.security.auth.login.conf` can also be
`/etc/storm/conf/storm_jaas.conf`, but the value above leads me to my next
point. All of these values already exist in storm.yaml. The fact that they
need to be specified again in the user's home is really strange. And it'll give
an error that the TGT found is not renewable, not something you'd expected.

I'm unsure if there are restrictions on where Ambari chooses to run service
check, so it's possible this would have to be setup on every node Storm lives
on the cluster. I'm also unsure if we can actually have Ambari automate this if
it turns out to be necessary, since we aren't the Storm service.

## Pull Request Checklist

Thank you for submitting a contribution to Apache Metron (Incubating).
Please refer to our [Development
Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235)
for the complete guide to follow for contributions.
Please refer also to our [Build Verification
Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview)
for complete smoke testing guides.

In order to streamline the review of the contribution we ask you follow
these guidelines and ask you to double check the following:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to
be created at [Metron
Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).

- [x] Does your PR title start with METRON- where is the JIRA
number you are trying to resolve? Pay particular attention to the hyphen "-"
character.
- [x] Has your PR been rebased against the latest commit within the target
branch (typically master)?

### For code changes:
- [x] Have you included steps to reproduce the behavior or problem that is
being changed or addressed?
- [x] 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 incubating-metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh
```

- ~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

[GitHub] incubator-metron issue #506: METRON-818: Ambari elasticsearch.properties tem...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/506
  
Thanks for running it up and verifying it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #506: METRON-818: Ambari elasticsearch.properties tem...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/506
  
@JonZeolla did you have any issues spinning this up?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #505: METRON-817: Customise output file path p...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/505#discussion_r109447598
  
--- Diff: 
metron-platform/metron-writer/src/main/java/org/apache/metron/writer/hdfs/HdfsWriter.java
 ---
@@ -74,17 +91,43 @@ public BulkWriterResponse write(String sourceType
) throws Exception
   {
 BulkWriterResponse response = new BulkWriterResponse();
-SourceHandler handler = 
getSourceHandler(configurations.getIndex(sourceType));
+// Currently treating all the messages in a group for pass/failure.
 try {
-  handler.handle(messages);
-} catch(Exception e) {
+  // Messages can all result in different HDFS paths, because of 
Stellar Expressions, so we'll need to iterate through
+  for(JSONObject message : messages) {
+Map val = 
configurations.getSensorConfig(sourceType);
+String path = getHdfsPathExtension(
+sourceType,
+
(String)configurations.getSensorConfig(sourceType).getOrDefault(IndexingConfigurations.OUTPUT_PATH_FUNCTION_CONF,
 ""),
+message
+);
+SourceHandler handler = getSourceHandler(sourceType, path);
+handler.handle(message);
+  }
+} catch (Exception e) {
   response.addAllErrors(e, tuples);
 }
 
 response.addAllSuccesses(tuples);
 return response;
   }
 
+  public String getHdfsPathExtension(String sourceType, String 
stellarFunction, JSONObject message) {
+// If no function is provided, just use the sourceType directly
+if(stellarFunction == null || stellarFunction.trim().isEmpty()) {
+  return sourceType;
+}
+
+StellarCompiler.Expression expression = 
sourceTypeExpressionMap.computeIfAbsent(stellarFunction, s -> 
stellarProcessor.compile(stellarFunction));
+VariableResolver resolver = new MapVariableResolver(message);
--- End diff --

@cestella Made that change.  I did make the check `if(objResult != null && 
!(objResult instanceof String)`, to avoid having falling into the IAE when 
objResult is null.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #506: METRON-818: Ambari elasticsearch.properties tem...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/506
  
Full dev spun up and ran fine, and I see items showing up in ES and HDFS


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #505: METRON-817: Customise output file path p...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/505#discussion_r109438625
  
--- Diff: 
metron-platform/metron-writer/src/main/java/org/apache/metron/writer/hdfs/HdfsWriter.java
 ---
@@ -74,17 +91,43 @@ public BulkWriterResponse write(String sourceType
) throws Exception
   {
 BulkWriterResponse response = new BulkWriterResponse();
-SourceHandler handler = 
getSourceHandler(configurations.getIndex(sourceType));
+// Currently treating all the messages in a group for pass/failure.
 try {
-  handler.handle(messages);
-} catch(Exception e) {
+  // Messages can all result in different HDFS paths, because of 
Stellar Expressions, so we'll need to iterate through
+  for(JSONObject message : messages) {
+Map val = 
configurations.getSensorConfig(sourceType);
+String path = getHdfsPathExtension(
+sourceType,
+
(String)configurations.getSensorConfig(sourceType).getOrDefault(IndexingConfigurations.OUTPUT_PATH_FUNCTION_CONF,
 ""),
+message
+);
+SourceHandler handler = getSourceHandler(sourceType, path);
+handler.handle(message);
+  }
+} catch (Exception e) {
   response.addAllErrors(e, tuples);
 }
 
 response.addAllSuccesses(tuples);
 return response;
   }
 
+  public String getHdfsPathExtension(String sourceType, String 
stellarFunction, JSONObject message) {
+// If no function is provided, just use the sourceType directly
+if(stellarFunction == null || stellarFunction.trim().isEmpty()) {
+  return sourceType;
+}
+
+StellarCompiler.Expression expression = 
sourceTypeExpressionMap.computeIfAbsent(stellarFunction, s -> 
stellarProcessor.compile(stellarFunction));
+VariableResolver resolver = new MapVariableResolver(message);
--- End diff --

@cestella I'm mostly concerned about the performance of function compile on 
every single message that comes through indexing.

If we keep the current approach, I would be interested in if there's a way 
to make things a little cleaner.

In retrospect, I think this should be an LRU cache, so that we don't keep 
around a given parse forever. Any thoughts on that, assuming performance would 
be enough of a concern to not just use your proposal?  


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #505: METRON-817: Customise output file path p...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/505#discussion_r109432502
  
--- Diff: 
metron-platform/metron-writer/src/main/java/org/apache/metron/writer/hdfs/HdfsWriter.java
 ---
@@ -74,17 +91,43 @@ public BulkWriterResponse write(String sourceType
) throws Exception
   {
 BulkWriterResponse response = new BulkWriterResponse();
-SourceHandler handler = 
getSourceHandler(configurations.getIndex(sourceType));
+// Currently treating all the messages in a group for pass/failure.
 try {
-  handler.handle(messages);
-} catch(Exception e) {
+  // Messages can all result in different HDFS paths, because of 
Stellar Expressions, so we'll need to iterate through
+  for(JSONObject message : messages) {
+Map val = 
configurations.getSensorConfig(sourceType);
+String path = getHdfsPathExtension(
+sourceType,
+
(String)configurations.getSensorConfig(sourceType).getOrDefault(IndexingConfigurations.OUTPUT_PATH_FUNCTION_CONF,
 ""),
+message
+);
+SourceHandler handler = getSourceHandler(sourceType, path);
+handler.handle(message);
+  }
+} catch (Exception e) {
   response.addAllErrors(e, tuples);
 }
 
 response.addAllSuccesses(tuples);
 return response;
   }
 
+  public String getHdfsPathExtension(String sourceType, String 
stellarFunction, JSONObject message) {
+// If no function is provided, just use the sourceType directly
+if(stellarFunction == null || stellarFunction.trim().isEmpty()) {
+  return sourceType;
+}
+
+StellarCompiler.Expression expression = 
sourceTypeExpressionMap.computeIfAbsent(stellarFunction, s -> 
stellarProcessor.compile(stellarFunction));
+VariableResolver resolver = new MapVariableResolver(message);
--- End diff --

Unfortunately, I don't think we can, unless we want to do more work to 
actually look up the function and validate. On top of it, things like MAP_GET 
essentially return Object anyway, so we'd still want to check if it's a String 
afterwards.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #506: METRON-818: Ambari elasticsearch.properties tem...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/506
  
@mmiklavc We probably should edit the Solr config.  That isn't carried 
through Ambari, so we don't have the same concern as here.  However, it does 
look like `storm.auto.credentials=[]` got added to solr.properties and I 
thought it wasn't necessary there.  Can we just drop that config and add the 
'topology.worker.childopts='?

Should I just go ahead and make that chance and add it to this PR?  And if 
I do, do we have any testing plan for Solr or are we just making best effort 
fixes?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #506: METRON-818: Ambari elasticsearch.propert...

GitHub user justinleet opened a pull request:

https://github.com/apache/incubator-metron/pull/506

METRON-818: Ambari elasticsearch.properties template is missing 
topology.worker.childopts

## Contributor Comments
Adding the empty config to the Ambari elasticsearch.properties template.

To test, spin up in a dev environment.  Indexing topology should produce 
results instead of an error in the logs now.

I'm still running this up in dev, but wanted to let people see what's going 
on. will update shortly.

As a workaround, just add this line to a running Ambari instance and 
restart indexing in Ambari to push the configs.

## Pull Request Checklist

Thank you for submitting a contribution to Apache Metron (Incubating).  
Please refer to our [Development 
Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235)
 for the complete guide to follow for contributions.  
Please refer also to our [Build Verification 
Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview)
 for complete smoke testing guides.  


In order to streamline the review of the contribution we ask you follow 
these guidelines and ask you to double check the following:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to 
be created at [Metron 
Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
 
- [x] Does your PR title start with METRON- where  is the JIRA 
number you are trying to resolve? Pay particular attention to the hyphen "-" 
character.
- [x] Has your PR been rebased against the latest commit within the target 
branch (typically master)?


### For code changes:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] 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 incubating-metron folder via:
  ```
  mvn -q clean integration-test install && build_utils/verify_licenses.sh 
  ```

- ~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](http://www.apache.org/legal/resolved.html#category-a)?~
- [ ] Have you verified the basic functionality of the build by building 
and running locally with Vagrant full-dev environment or the equivalent?

### For documentation related changes:
- ~Have you ensured that format looks appropriate for the output in which 
it is rendered by building and verifying the site-book? If not then run the 
following commands and the verify changes~
 Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and submit an update to your PR as soon as possible.
It is also recommened that [travis-ci](https://travis-ci.org) is set up for 
your personal repository such that your branches are built there before 
submitting a pull request.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/justinleet/incubator-metron ambari_config_fix

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-metron/pull/506.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #506


commit d1fd6cf59433747a3cca503df552fcd0f003f488
Author: justinjleet 
Date:   2017-04-03T13:34:50Z

Adding elasticsearch.properties empty config




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #505: METRON-817: Customise output file path p...

GitHub user justinleet opened a pull request:

https://github.com/apache/incubator-metron/pull/505

METRON-817: Customise output file path patterns for HDFS indexing

## Contributor Comments
Primarily this affects HdfsWriter by changing the output path from a set 
path (`/apps/metron/.../`), and allow it to be defined via a Stellar 
Function.  Specifically, the base path is still defined the same (The 
`/apps/metron/.../` portion), but the `` portion is dropped and can now 
be defined by a Stellar function.  By default, the original behavior of 
`` is used.  This is defined in the `.json` file as indicated 
in the new README.md for metron-writer.

### Notes
- This requires adding tracking things a bit more carefully (and if you're 
reviewing, please validate that it happens correctly).  When the outputFile is 
closed, we remove the sourceHandler from HdfsWriter's map.
  - I'm slightly concerned about the correctness of the implementation, but 
it seems necessary to ensure that we don't leave a bunch of SourceHandlers 
lying around as data changes (and we don't want an enormous number of output 
files being written to).
  - If there's a cleaner way to manage this, I'd love to hear it and can 
refactor pretty easily. It throws off the rotation count (because we kill the 
SourceHandler from the map itself), but I doubt we care about that since it 
really only shows up in the output filename anyway.
- This also adds an argument for max open files.  This is a flux level 
config. I defaulted this to 500.  500 was chosen because it was an arbitrary 
round number that wasn't enormous.
  - If someone has a default with any real reasoning behind it, I'll go 
ahead and change it.
- In HdfsWriter, we iterate through the messages, apply the Stellar 
function and then call the relevant handler. The entire group of message is 
treated as one single pass/fail (which is the same as the old behavior), rather 
than individually. The try/catch could potentially be moved into the for loop, 
but I don't think there's an explicit link between the message and the tuples 
that we can exploit to fail per message.  I don't think it needs to be 
addressed here, but I'm curious if there's thought on this.

### Testing
Unit tests are added to pretty much cover HdfsWriter, and this can be spun 
up in a dev environment.

To test in dev

- Spin up a dev environment
- Validate that the output matches the old format in HDFS (Nothing has an 
output function defined)
  ```
  [hdfs@node1 vagrant]$ hdfs dfs -ls /apps/metron/indexing/indexed/
  Found 3 items
  drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/bro
  drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/error
  drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/snort
  ```
- Edit the indexing config for Bro to include an outputPathFunction in the 
hdfs section, e.g. in `/usr/metron/0.3.1/config/zookeeper/indexing/bro.json`
  ```
  {
"hdfs" : {
  "index": "bro",
  "batchSize": 5,
  "enabled" : true,
  "outputPathFunction": "FORMAT('ipsrc-%s', ip_src_addr)"
},
"elasticsearch" : {
  "index": "bro",
  "batchSize": 5,
  "enabled" : true
},
"solr" : {
  "index": "bro",
  "batchSize": 5,
  "enabled" : true
}
  }
  ```
- Push the config configs to ZooKeeper: 
`/usr/metron/0.3.1/bin/zk_load_configs.sh -z node1:2181 -m PUSH -i 
/usr/metron/0.3.1/config/zookeeper/`
- Let some more data run through and check the output folders, e.g.
  ```
[hdfs@node1 vagrant]$ hdfs dfs -ls /apps/metron/indexing/indexed/
Found 5 items
drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/bro
drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/error
drwxrwxr-x   - storm hadoop  0 2017-04-03 13:14 
/apps/metron/indexing/indexed/ipsrc-192.168.138.158
drwxrwxr-x   - storm hadoop  0 2017-04-03 13:14 
/apps/metron/indexing/indexed/ipsrc-192.168.66.1
drwxrwxr-x   - storm hadoop  0 2017-04-03 13:11 
/apps/metron/indexing/indexed/snort
[hdfs@node1 vagrant]$ hdfs dfs -ls 
/apps/metron/indexing/indexed/ipsrc-192.168.138.158
Found 1 items
-rw-r--r--   1 storm hadoop 223182 2017-04-03 13:14 
/apps/metron/indexing/indexed/ipsrc-192.168.138.158/enrichment-null-0-0-1491225291377.json
  ```

## Pull Request Checklist

Thank you for submitting a contributio

[GitHub] incubator-metron issue #486: METRON-793: Migrate to storm-kafka-client kafka...

2017-03-27 Thread justinleet

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/486
  
@cestella My +1 stands with the testing issues ironed out.  Thanks for 
looking into it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #488: METRON-796: Mpack uses wrong group for owning H...

2017-03-26 Thread justinleet

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/488
  
https://issues.apache.org/jira/browse/METRON-349 is updated to be more 
complete and reflect the current state of things.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #488: METRON-796: Mpack uses wrong group for owning H...

2017-03-26 Thread justinleet

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/488
  
@dlyle65535 Failure mode is that HDFS writes from Storm fail.  The 
directories are owned by metron:metron with 775.  Storm isn't in the metron 
group, so it fails to write.  The perms exception is thrown in the bolt and no 
output file is created.  Writes to ES work as expected.  Validating this is 
pretty easy, just run up full dev and see if the perms error shows up and if 
HDFS gets any files.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #488: METRON-796: Mpack uses wrong group for owning H...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/488
  
@mattf-horton Permissions on the other items are generally 775, so they can 
be read as needed (and should be scaled back once we have everything lined up 
with the user as @simonellistonball mentions). I just wanted to touch things as 
little as possible to get them back in a working state.

We should either expand out or replace METRON-349 (including closing off 
permissions) as the actual solution to the problem.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #488: METRON-796: Mpack uses wrong group for o...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/488#discussion_r107965896
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/params/params_linux.py
 ---
@@ -39,7 +39,7 @@
 tmp_dir = Script.get_tmp_dir()
 
 hostname = config['hostname']
-metron_group = config['configurations']['cluster-env']['metron_group']
+hadoop_group = config['configurations']['cluster-env']['user_group']
--- End diff --

Updated with a comment to clarify things a bit.  Let me know if you think 
there's anything else we want to add or clarify.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #488: METRON-796: Mpack uses wrong group for owning H...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/488
  
For anybody looking, Matt's review comment is still relevant to discussion, 
but unfortunately hidden by GitHub thinking it's outdated after the last commit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #488: METRON-796: Mpack uses wrong group for o...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/488#discussion_r107961358
  
--- Diff: 
metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/params/params_linux.py
 ---
@@ -39,7 +39,7 @@
 tmp_dir = Script.get_tmp_dir()
 
 hostname = config['hostname']
-metron_group = config['configurations']['cluster-env']['metron_group']
+hadoop_group = config['configurations']['cluster-env']['user_group']
--- End diff --

I'll go ahead and move the config.

On the group issue, that is the group named 'hadoop'.  The cluster level 
config is named 'user_group', I have absolutely no idea why. I only called it 
'hadoop_group' here, so it was more obvious it shouldn't be killed in the 
future.  If there are objections to calling it 'hadoop_group', I could also 
carry it through as user_group and add a comment about the meaning in the 
params file.

For example, in HDP stack 
https://github.com/apache/ambari/blob/trunk/ambari-server/src/main/resources/stacks/HDP/2.0.6/configuration/cluster-env.xml#L158
```
user_group
Hadoop Group
hadoop
GROUP
Hadoop user group.
```

This declaration carried through a couple other stack definitions that I 
looked at.

The use of this group also seems fairly common, e.g. in 
https://github.com/apache/ambari/blob/trunk/ambari-server/src/main/resources/common-services/HDFS/2.1.0.2.0/package/scripts/hdfs.py#L60
```
  if "hadoop-policy" in params.config['configurations']:
XmlConfig("hadoop-policy.xml",
  conf_dir=params.hadoop_conf_dir,
  
configurations=params.config['configurations']['hadoop-policy'],
  
configuration_attributes=params.config['configuration_attributes']['hadoop-policy'],
  owner=params.hdfs_user,
  group=params.user_group
)
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #486: METRON-793: Migrate to storm-kafka-client kafka...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/486
  
@cestella Just noticed Travis after I commented.  I'm moderately surprised 
that the most recent PR would break it, do you know what the issue is?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #486: METRON-793: Migrate to storm-kafka-client kafka...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/486
  
+1, was able to follow Mike's instructions, with a couple caveats.

- Group authorization command was missing
```
/usr/hdp/current/kafka-broker/bin/kafka-acls.sh --authorizer 
kafka.security.auth.SimpleAclAuthorizer --authorizer-properties 
zookeeper.connect=node1:2181 --add --allow-principal User:storm-metron_cluster 
--allow-principal User:justin --group jsonMap_parser
```
- Topic authorization command on the enrichments topic side was missing.
```
 /usr/hdp/current/kafka-broker/bin/kafka-acls.sh --authorizer 
kafka.security.auth.SimpleAclAuthorizer --authorizer-properties 
zookeeper.connect=node1:2181 --add --allow-principal User:storm-metron_cluster 
--allow-principal User:justin --topic enrichments
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107913632
  
--- Diff: 
metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/topology/ParserTopologyBuilder.java
 ---
@@ -106,19 +105,22 @@ public static TopologyBuilder build(String 
zookeeperUrl,
   /**
* Create a spout that consumes tuples from a Kafka topic.
*
-   * @param zookeeperUrlZookeeper URL
+   * @param zkQuorum Zookeeper URL
* @param sensorType  Type of sensor
-   * @param offset  Kafka topic offset where the topology 
will start; BEGINNING, END, WHERE_I_LEFT_OFF
-   * @param kafkaSpoutConfigOptions Configuration options for the kafka 
spout
+   * @param kafkaConfigOptional Configuration options for the kafka 
spout
* @param parserConfigConfiguration for the parser
* @return
*/
-  private static KafkaSpout createKafkaSpout(String zookeeperUrl, String 
sensorType, SpoutConfig.Offset offset, EnumMap 
kafkaSpoutConfigOptions, SensorParserConfig parserConfig) {
-
+  private static StormKafkaSpout createKafkaSpout(String zkQuorum, String 
sensorType, Optional> kafkaConfigOptional, 
SensorParserConfig parserConfig) {
--- End diff --

StormKafkaSpout's return type here will actually be StormKafkaSpout, right?  Can we make that explicit, rather than untyped (and also drop 
the Object.class from the creates assuming the other typing change)?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107912514
  
--- Diff: 
metron-platform/metron-storm-kafka/src/main/java/org/apache/metron/storm/kafka/flux/SimpleStormKafkaBuilder.java
 ---
@@ -0,0 +1,234 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.storm.kafka.flux;
+
+import com.google.common.base.Joiner;
+import org.apache.kafka.clients.consumer.Consumer;
+import org.apache.kafka.clients.consumer.ConsumerRecord;
+import org.apache.kafka.common.serialization.ByteArrayDeserializer;
+import org.apache.metron.common.utils.KafkaUtils;
+import org.apache.storm.kafka.spout.*;
+import org.apache.storm.spout.SpoutOutputCollector;
+import org.apache.storm.topology.OutputFieldsDeclarer;
+import org.apache.storm.topology.OutputFieldsGetter;
+import org.apache.storm.tuple.Fields;
+import org.apache.storm.tuple.Values;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+
+/**
+ * This is a convenience layer on top of the KafkaSpoutConfig.Builder 
available in storm-kafka-client.
+ * The justification for this class is two-fold.  First, there are a lot 
of moving parts and a simplified
+ * approach to constructing spouts is useful.  Secondly, and perhaps more 
importantly, the Builder pattern
+ * is decidedly unfriendly to use inside of Flux.  Finally, we can make 
things a bit more friendly by only requiring
+ * zookeeper and automatically figuring out the brokers for the bootstrap 
server.
+ *
+ * @param  The kafka key type
+ * @param  The kafka value type
+ */
+public class SimpleStormKafkaBuilder extends 
KafkaSpoutConfig.Builder {
+  final static String STREAM = "default";
+
+  /**
+   * The fields exposed by the kafka consumer.  These will show up in the 
Storm tuple.
+   */
+  public enum FieldsConfiguration {
+KEY("key", record -> record.key()),
+VALUE("value", record -> record.value()),
+PARTITION("partition", record -> record.partition()),
+TOPIC("topic", record -> record.topic())
+;
+String fieldName;
+Function recordExtractor;
+
+FieldsConfiguration(String fieldName, Function 
recordExtractor) {
+  this.recordExtractor = recordExtractor;
+  this.fieldName = fieldName;
+}
+
+/**
+ * Return a list of the enums
+ * @param configs
+ * @return
+ */
+public static List toList(String... configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Return a list of the enums from their string representation.
+ * @param configs
+ * @return
+ */
+public static List toList(List configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Construct a Fields object from an iterable of enums.  These fields 
are the fields
+ * exposed in the Storm tuple emitted from the spout.
+ * @param configs
+ * @return
+ */
+public static Fields getFields(Iterable configs) {
+  List fields = new ArrayList<>();
+  for(FieldsConfiguration config : configs) {
+fields.add(config.fieldName);
+  }
+  return new Fields(fields);
+}
+  }
+
+  /**
+   * Build a tuple given the fields and the topic.  We want to use our 
FieldsConfiguration enum
+   * to define what this tuple looks like.
+   * @param  T

[GitHub] incubator-metron issue #488: METRON-796: Mpack uses wrong group for owning H...

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/488
  
I agree the topologies should run as the metron user, but this is just to 
get things back in a working state again (and it already used to be this way, 
so this isn't opening things up more than it was a couple weeks ago).

I actually thought there was a separate Jira for running as the Metron 
user, but the one I was thinking of is 
https://issues.apache.org/jira/browse/METRON-349.  The ticket should really be 
to consolidate everything under the metron user with appropriate ownership.  I 
don't have a preference for updating that ticket or closing it as a new one 
(and I'm not sure which the community would prefer).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #488: METRON-796: Mpack uses wrong group for o...

GitHub user justinleet opened a pull request:

https://github.com/apache/incubator-metron/pull/488

METRON-796: Mpack uses wrong group for owning HDFS directories

## Contributor Comments
Reverts the group owner of a couple HDFS directories to be the hadoop 
group, rather than the metron group (which is just metron).  Right now, the 
topologies run as the storm user (which belongs to the hadoop group), and 
therefore didn't have permission to write to HDFS (including in quick and full 
dev).  This sets HDFS ownership to metron:hadoop, which lets it be handled 
appropriately.

Other items, such as configs and installation files, were just left as the 
metron group.

To test, just run up a dev environment and ensure files are being written 
and ownership makes sense (/apps/metron/indexing/indexed is metron:hadoop with 
755 perms).  The individual sensors will be owned by storm:hadoop (proving that 
writes work).

For example:
```
[vagrant@node1 ~]$ hdfs dfs -ls /apps/metron/indexing
Found 1 items
drwxrwxr-x   - metron hadoop  0 2017-03-24 12:57 
/apps/metron/indexing/indexed
[vagrant@node1 ~]$ hdfs dfs -ls /apps/metron/indexing/indexed
Found 3 items
drwxrwxr-x   - storm hadoop  0 2017-03-24 13:01 
/apps/metron/indexing/indexed/bro
drwxrwxr-x   - storm hadoop  0 2017-03-24 13:01 
/apps/metron/indexing/indexed/error
drwxrwxr-x   - storm hadoop  0 2017-03-24 13:01 
/apps/metron/indexing/indexed/snort
[vagrant@node1 ~]$ hdfs dfs -ls /apps/metron/indexing/indexed/bro
Found 1 items
-rw-r--r--   1 storm hadoop 211393 2017-03-24 13:01 
/apps/metron/indexing/indexed/bro/enrichment-null-0-0-1490360489968.json
```

As a note, metron_group existed twice in params_linux.py, so only the first 
instance is changed to hadoop_group and pulled appropriately.  The second is 
left as-is.

## Pull Request Checklist

Thank you for submitting a contribution to Apache Metron (Incubating).  
Please refer to our [Development 
Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235)
 for the complete guide to follow for contributions.  
Please refer also to our [Build Verification 
Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview)
 for complete smoke testing guides.  


In order to streamline the review of the contribution we ask you follow 
these guidelines and ask you to double check the following:

### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to 
be created at [Metron 
Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
 
- [x] Does your PR title start with METRON- where  is the JIRA 
number you are trying to resolve? Pay particular attention to the hyphen "-" 
character.
- [x] Has your PR been rebased against the latest commit within the target 
branch (typically master)?


### For code changes:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] Have you ensured that the full suite of tests and checks have been 
executed in the root incubating-metron folder via:
  ```
  mvn -q clean integration-test install && build_utils/verify_licenses.sh 
  ```

- [x] Have you verified the basic functionality of the build by building 
and running locally with Vagrant full-dev environment or the equivalent?

 Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and submit an update to your PR as soon as possible.
It is also recommened that [travis-ci](https://travis-ci.org) is set up for 
your personal repository such that your branches are built there before 
submitting a pull request.



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/justinleet/incubator-metron METRON-796

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-metron/pull/488.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #488


commit ec7d070524334603e1712b4649e5600ded450284
Author: justinjleet 
Date:   2017-03-24T00:55:23Z

Updating group perms




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

2017-03-23 Thread justinleet

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107683392
  
--- Diff: 
metron-platform/metron-pcap-backend/src/main/java/org/apache/metron/spout/pcap/HDFSWriterCallback.java
 ---
@@ -96,16 +109,18 @@ public HDFSWriterCallback withConfig(HDFSWriterConfig 
config) {
 this.config = config;
 return this;
 }
+
 @Override
 public List apply(List tuple, EmitContext context) {
-
-List keyValue = (List) tuple.get(0);
-LongWritable ts = (LongWritable) keyValue.get(0);
-BytesWritable rawPacket = (BytesWritable)keyValue.get(1);
+byte[] key = (byte[]) tuple.get(0);
+byte[] value = (byte[]) tuple.get(1);
+if(!config.getDeserializer().deserializeKeyValue(key, value, 
KeyValue.key.get(), KeyValue.value.get())) {
+LOG.debug("Dropping malformed packet...");
--- End diff --

I'm good with that. I had mostly discarded the worry about size because 
this is in debugging statements anyway and typically with storm you're setting 
reasonable timeouts on logging levels anyway.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #487: METRON-792: Quick Dev should remove/replace RPM...

2017-03-23 Thread justinleet

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/487
  
I'm +1 by inspection


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron issue #486: METRON-793: Migrate to storm-kafka-client kafka...

2017-03-23 Thread justinleet

Github user justinleet commented on the issue:

https://github.com/apache/incubator-metron/pull/486
  
@cestella I'm good with keeping the extension points especially after the 
points you made. I think the TODOs are valuable, I just wanted to know the 
thought behind potentially building it out.

Given the API instability, unfortunately it seems like our dependencies 
aren't going to provide that insulation layer.  I'd rather have that be 
provided in a stable manner upstream from us, but that's not something we have 
any control over.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107438798
  
--- Diff: 
metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/integration/components/ParserTopologyComponent.java
 ---
@@ -97,6 +99,19 @@ public void start() throws UnableToStartException {
   public void stop() {
 if(stormCluster != null) {
   stormCluster.shutdown();
+  if(new File("logs/workers-artifacts").exists()) {
+Path rootPath = Paths.get("logs");
+Path destPath = Paths.get("target/logs");
+try {
+  Files.move(rootPath, destPath);
+  Files.walk(destPath)
--- End diff --

Same deal with FileUtils.deleteDirectory() here


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107522830
  
--- Diff: 
metron-platform/metron-pcap-backend/src/main/java/org/apache/metron/spout/pcap/HDFSWriterCallback.java
 ---
@@ -96,16 +109,18 @@ public HDFSWriterCallback withConfig(HDFSWriterConfig 
config) {
 this.config = config;
 return this;
 }
+
 @Override
 public List apply(List tuple, EmitContext context) {
-
-List keyValue = (List) tuple.get(0);
-LongWritable ts = (LongWritable) keyValue.get(0);
-BytesWritable rawPacket = (BytesWritable)keyValue.get(1);
+byte[] key = (byte[]) tuple.get(0);
+byte[] value = (byte[]) tuple.get(1);
+if(!config.getDeserializer().deserializeKeyValue(key, value, 
KeyValue.key.get(), KeyValue.value.get())) {
+LOG.debug("Dropping malformed packet...");
--- End diff --

Is it reasonable to include the key and value we're having issues with in 
the debug statement?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107524059
  
--- Diff: 
metron-platform/metron-pcap-backend/src/main/java/org/apache/metron/spout/pcap/deserializer/Deserializers.java
 ---
@@ -0,0 +1,59 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.spout.pcap.deserializer;
+
+import org.apache.metron.common.utils.timestamp.TimestampConverters;
+import org.apache.metron.common.utils.timestamp.TimestampConverter;
+
+import java.util.function.Function;
+
+/**
+ * Deserializers take the raw bytes from kafka key and value and construct 
the timestamp and raw bytes for PCAP.
+ */
+public enum Deserializers {
+  /**
+   * Extract the timestamp from the key and the raw packet 
(global-headerless) from the value
+   */
+   FROM_KEY( converter -> new FromKeyDeserializer(converter))
+  /**
+   * Ignore the key and pull the timestamp directly from the packet 
itself.  Also, assume that the packet isn't global-headerless.
+   */
+  ,FROM_PACKET(converter -> new FromPacketDeserializer());
+  ;
+  Function creator;
+  Deserializers(Function creator)
+  {
+this.creator = creator;
+  }
+
+  public static KeyValueDeserializer create(String scheme, 
TimestampConverter converter) {
+try {
+  Deserializers ts = Deserializers.valueOf(scheme.toUpperCase());
+  return ts.creator.apply(converter);
+}
+catch(IllegalArgumentException iae) {
+  return Deserializers.FROM_KEY.creator.apply(converter);
+}
+  }
+
+  public static KeyValueDeserializer create(String scheme, String 
converter) {
+return create(scheme, 
TimestampConverters.valueOf(converter.toUpperCase()));
--- End diff --

Shouldn't this be a call to TimestampConverters.getConverter()?

And if we're uppercasing things, shouldn't it be in the TimestampConverter 
(given that it's meant to match an Enum value)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107521465
  
--- Diff: 
metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/topology/ParserTopologyBuilder.java
 ---
@@ -106,19 +105,26 @@ public static TopologyBuilder build(String 
zookeeperUrl,
   /**
* Create a spout that consumes tuples from a Kafka topic.
*
-   * @param zookeeperUrlZookeeper URL
+   * @param zkQuorum Zookeeper URL
* @param sensorType  Type of sensor
-   * @param offset  Kafka topic offset where the topology 
will start; BEGINNING, END, WHERE_I_LEFT_OFF
-   * @param kafkaSpoutConfigOptions Configuration options for the kafka 
spout
+   * @param kafkaConfigOptional Configuration options for the kafka 
spout
* @param parserConfigConfiguration for the parser
* @return
*/
-  private static KafkaSpout createKafkaSpout(String zookeeperUrl, String 
sensorType, SpoutConfig.Offset offset, EnumMap 
kafkaSpoutConfigOptions, SensorParserConfig parserConfig) {
-
+  private static StormKafkaSpout createKafkaSpout(String zkQuorum, String 
sensorType, Optional> kafkaConfigOptional, 
SensorParserConfig parserConfig) {
+Map kafkaSpoutConfigOptions = 
kafkaConfigOptional.orElse(new HashMap<>());
 String inputTopic = parserConfig.getSensorTopic() != null ? 
parserConfig.getSensorTopic() : sensorType;
-SpoutConfig spoutConfig = new SpoutConfig(new ZkHosts(zookeeperUrl), 
inputTopic, "", inputTopic).from(offset);
-SpoutConfigOptions.configure(spoutConfig, kafkaSpoutConfigOptions);
-return new KafkaSpout(spoutConfig);
+
if(!kafkaSpoutConfigOptions.containsKey(SpoutConfiguration.FIRST_POLL_OFFSET_STRATEGY.key))
 {
--- End diff --

Can you make these putIfAbsent calls()?

e.g.
```

kafkaSpoutConfigOptions.putIfAbsent(SpoutConfiguration.FIRST_POLL_OFFSET_STRATEGY.key,

KafkaSpoutConfig.FirstPollOffsetStrategy.UNCOMMITTED_EARLIEST.toString());
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107534534
  
--- Diff: pom.xml ---
@@ -67,20 +67,44 @@
 
 1.0.1
 1.0.1
-0.10.0.1
+0.10.0
 2.7.1
 1.1.1
-1.8.0
 1.5.2
 
+1.8.0
 4.5
 3.7
 2.7.1
 3.3
-${base_storm_version}
+1.0.3
+
+
1.0.1.2.5.0.0-1245
--- End diff --

I'm sure you've already thought of this, but assuming we do go with this, 
please make sure this gets a JIRA associated with it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107531858
  
--- Diff: 
metron-platform/metron-storm-kafka/src/main/java/org/apache/metron/storm/kafka/flux/SimpleStormKafkaBuilder.java
 ---
@@ -0,0 +1,234 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.storm.kafka.flux;
+
+import com.google.common.base.Joiner;
+import org.apache.kafka.clients.consumer.Consumer;
+import org.apache.kafka.clients.consumer.ConsumerRecord;
+import org.apache.kafka.common.serialization.ByteArrayDeserializer;
+import org.apache.metron.common.utils.KafkaUtils;
+import org.apache.storm.kafka.spout.*;
+import org.apache.storm.spout.SpoutOutputCollector;
+import org.apache.storm.topology.OutputFieldsDeclarer;
+import org.apache.storm.topology.OutputFieldsGetter;
+import org.apache.storm.tuple.Fields;
+import org.apache.storm.tuple.Values;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+
+/**
+ * This is a convenience layer on top of the KafkaSpoutConfig.Builder 
available in storm-kafka-client.
+ * The justification for this class is two-fold.  First, there are a lot 
of moving parts and a simplified
+ * approach to constructing spouts is useful.  Secondly, and perhaps more 
importantly, the Builder pattern
+ * is decidedly unfriendly to use inside of Flux.  Finally, we can make 
things a bit more friendly by only requiring
+ * zookeeper and automatically figuring out the brokers for the bootstrap 
server.
+ *
+ * @param  The kafka key type
+ * @param  The kafka value type
+ */
+public class SimpleStormKafkaBuilder extends 
KafkaSpoutConfig.Builder {
+  final static String STREAM = "default";
+
+  /**
+   * The fields exposed by the kafka consumer.  These will show up in the 
Storm tuple.
+   */
+  public enum FieldsConfiguration {
+KEY("key", record -> record.key()),
+VALUE("value", record -> record.value()),
+PARTITION("partition", record -> record.partition()),
+TOPIC("topic", record -> record.topic())
+;
+String fieldName;
+Function recordExtractor;
+
+FieldsConfiguration(String fieldName, Function 
recordExtractor) {
+  this.recordExtractor = recordExtractor;
+  this.fieldName = fieldName;
+}
+
+/**
+ * Return a list of the enums
+ * @param configs
+ * @return
+ */
+public static List toList(String... configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Return a list of the enums from their string representation.
+ * @param configs
+ * @return
+ */
+public static List toList(List configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Construct a Fields object from an iterable of enums.  These fields 
are the fields
+ * exposed in the Storm tuple emitted from the spout.
+ * @param configs
+ * @return
+ */
+public static Fields getFields(Iterable configs) {
+  List fields = new ArrayList<>();
+  for(FieldsConfiguration config : configs) {
+fields.add(config.fieldName);
+  }
+  return new Fields(fields);
+}
+  }
+
+  /**
+   * Build a tuple given the fields and the topic.  We want to use our 
FieldsConfiguration enum
+   * to define what this tuple looks like.
+   * @param  T

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...

Github user justinleet commented on a diff in the pull request:

https://github.com/apache/incubator-metron/pull/486#discussion_r107533535
  
--- Diff: 
metron-platform/metron-storm-kafka/src/main/java/org/apache/metron/storm/kafka/flux/SimpleStormKafkaBuilder.java
 ---
@@ -0,0 +1,234 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.metron.storm.kafka.flux;
+
+import com.google.common.base.Joiner;
+import org.apache.kafka.clients.consumer.Consumer;
+import org.apache.kafka.clients.consumer.ConsumerRecord;
+import org.apache.kafka.common.serialization.ByteArrayDeserializer;
+import org.apache.metron.common.utils.KafkaUtils;
+import org.apache.storm.kafka.spout.*;
+import org.apache.storm.spout.SpoutOutputCollector;
+import org.apache.storm.topology.OutputFieldsDeclarer;
+import org.apache.storm.topology.OutputFieldsGetter;
+import org.apache.storm.tuple.Fields;
+import org.apache.storm.tuple.Values;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+
+/**
+ * This is a convenience layer on top of the KafkaSpoutConfig.Builder 
available in storm-kafka-client.
+ * The justification for this class is two-fold.  First, there are a lot 
of moving parts and a simplified
+ * approach to constructing spouts is useful.  Secondly, and perhaps more 
importantly, the Builder pattern
+ * is decidedly unfriendly to use inside of Flux.  Finally, we can make 
things a bit more friendly by only requiring
+ * zookeeper and automatically figuring out the brokers for the bootstrap 
server.
+ *
+ * @param  The kafka key type
+ * @param  The kafka value type
+ */
+public class SimpleStormKafkaBuilder extends 
KafkaSpoutConfig.Builder {
+  final static String STREAM = "default";
+
+  /**
+   * The fields exposed by the kafka consumer.  These will show up in the 
Storm tuple.
+   */
+  public enum FieldsConfiguration {
+KEY("key", record -> record.key()),
+VALUE("value", record -> record.value()),
+PARTITION("partition", record -> record.partition()),
+TOPIC("topic", record -> record.topic())
+;
+String fieldName;
+Function recordExtractor;
+
+FieldsConfiguration(String fieldName, Function 
recordExtractor) {
+  this.recordExtractor = recordExtractor;
+  this.fieldName = fieldName;
+}
+
+/**
+ * Return a list of the enums
+ * @param configs
+ * @return
+ */
+public static List toList(String... configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Return a list of the enums from their string representation.
+ * @param configs
+ * @return
+ */
+public static List toList(List configs) {
+  List ret = new ArrayList<>();
+  for(String config : configs) {
+ret.add(FieldsConfiguration.valueOf(config.toUpperCase()));
+  }
+  return ret;
+}
+
+/**
+ * Construct a Fields object from an iterable of enums.  These fields 
are the fields
+ * exposed in the Storm tuple emitted from the spout.
+ * @param configs
+ * @return
+ */
+public static Fields getFields(Iterable configs) {
+  List fields = new ArrayList<>();
+  for(FieldsConfiguration config : configs) {
+fields.add(config.fieldName);
+  }
+  return new Fields(fields);
+}
+  }
+
+  /**
+   * Build a tuple given the fields and the topic.  We want to use our 
FieldsConfiguration enum
+   * to define what this tuple looks like.
+   * @param  T

[GitHub] incubator-metron pull request #486: METRON-793: Migrate to storm-kafka-clien...