Skip to content
This repository was archived by the owner on Aug 20, 2025. It is now read-only.

METRON-1339 Stellar Shell functionality to verify stored stellar statements #856

Closed
ottobackwards wants to merge 22 commits intoapache:masterfrom
ottobackwards:stellar_verify_deployed_shell
Closed

METRON-1339 Stellar Shell functionality to verify stored stellar statements #856
ottobackwards wants to merge 22 commits intoapache:masterfrom
ottobackwards:stellar_verify_deployed_shell

Conversation

@ottobackwards
Copy link
Contributor

@ottobackwards ottobackwards commented Nov 30, 2017

This will allow users to check their deployed statements, say after upgrade, when they are at rest ( and would fail on use ).
In other words, they were valid when stored, but are not now because of stellar changes, such as new keywords.

The interface StellarConfiguredStatementReporter, which is @IndexSubclasses ( ClassIndex) marked, allows the shell to discover reporters that can provide statements for validation. This discovery allows de-coupling of stellar and 'hosts' that know about the location of the stored statements, and the configuration structure details.

We do mention the configurations in the shell output at this time.

metron-common implements this interface, and can run through visiting all the configurations.

A management function was added VALIDATE_STELLAR_RULE_CONFIGS()
When executed, the shell

  • discovers the reporters through class index
  • visits the reports, with callbacks for visits or errors
  • per visit ( which is called for a specific stellar statement ) the statement is compiled and errors reported
  • if the entire config fails ( threat triage stellar errors fail on deserialize so we don't get to do ANY enrichment visits in that case ) the error callback handles that

I'm getting this out there, still a couple of things todo:

[x] full dev run. I have been testing with stellar external to full dev iteratively
[x] readme
[x] steps to test
[x] unit test
[x] ThreatTriage Rule Reason

Testing

  • deploy full dev
  • edit the squid parser transformation(s) such that the stellar would not compile, such as adding a dangling = in zookeeper
{ 
"parserClassName": "org.apache.metron.parsers.GrokParser", 
"sensorTopic": "squid", 
"parserConfig": { 
"grokPath": "/patterns/squid", 
"patternLabel": "SQUID_DELIMITED", 
"timestampField": "timestamp" 
}, 
"fieldTransformations" : [ 
{ 
"transformation" : "STELLAR" 
,"output" : [ "full_hostname", "domain_without_subdomains" ] 
,"config" : { 
"full_hostname" : "URL_TO_HOST(url) =" 
,"domain_without_subdomains" : "DOMAIN_REMOVE_SUBDOMAINS(full_hostname)" 
} 
} 
] 
}
  • edit the snort threat triage rules in it's enrichment config in zookeeper ( here with an extra ) )
{ 
"enrichment" : { 
"fieldMap": 
{ 
"geo": ["ip_dst_addr", "ip_src_addr"], 
"host": ["host"] 
} 
}, 
"threatIntel" : { 
"fieldMap": 
{ 
"hbaseThreatIntel": ["ip_src_addr", "ip_dst_addr"] 
}, 
"fieldToTypeMap": 
{ 
"ip_src_addr" : ["malicious_ip"], 
"ip_dst_addr" : ["malicious_ip"] 
}, 
"triageConfig" : { 
"riskLevelRules" : [ 
{ 
"rule" : "not(IN_SUBNET(ip_dst_addr, '192.168.0.0/24')) )", 
"score" : 10 
} 
], 
"aggregator" : "MAX" 
} 
} 
} 

Working with zookeeper

I am not a zk cli maestro, so I took the easy way out and used ZK-WEB.
Following the readme instructions it was very simple to clone, edit the config for full dev, and run from source. If you log in with the creds in the config you can edit the nodes.

Results

When you run the function, it will report the failed stellar statements.
Examples with and without :

NOTE passing a wrap value may be necessary for better visibility as shown

[Stellar]>>> VALIDATE_STELLAR_RULE_CONFIGS()
╔═══════════════════════════════════════════════════════════════════════════════════════════╤═══════════════════════════════════════════════╤═══════╤═══════╗
║ PATH                                                                                      │ RULE                                          │ ERROR │ VALID ║
╠═══════════════════════════════════════════════════════════════════════════════════════════╪═══════════════════════════════════════════════╪═══════╪═══════╣
║ Apache Metron/PARSER/squid/fieldTransformations/0/Field Mapping/full_hostname             │ URL_TO_HOST(url)                              │       │ true  ║
╟───────────────────────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────┼───────┼───────╢
║ Apache Metron/PARSER/squid/fieldTransformations/0/Field Mapping/domain_without_subdomains │ DOMAIN_REMOVE_SUBDOMAINS(full_hostname)       │       │ true  ║
╟───────────────────────────────────────────────────────────────────────────────────────────┼───────────────────────────────────────────────┼───────┼───────╢
║ Apache Metron/ENRICHMENT/snort/threatIntel/triageConfig/riskLevelRules/0/rule             │ not(IN_SUBNET(ip_dst_addr, '192.168.0.0/24')) │       │ true  ║
╚═══════════════════════════════════════════════════════════════════════════════════════════╧═══════════════════════════════════════════════╧═══════╧═══════╝

[Stellar]>>>

[Stellar]>>> VALIDATE_STELLAR_RULE_CONFIGS(40)
╔════════════════════════════════════════════════════════════════════════╤══════════════════════════════════════════╤═══════════════════════════════════════╤═══════╗
║ PATH                                                                   │ RULE                                     │ ERROR                                 │ VALID ║
╠════════════════════════════════════════════════════════════════════════╪══════════════════════════════════════════╪═══════════════════════════════════════╪═══════╣
║ Apache                                                                 │ URL_TO_HOST(url)=                        │ Syntax error @ 1:16 token recognition │ false ║
║ Metron/PARSER/squid/fieldTransformations/0/Field                       │                                          │ error at: '='                         │       ║
║ Mapping/full_hostname                                                  │                                          │                                       │       ║
╟────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────┼───────────────────────────────────────┼───────╢
║ Apache                                                                 │ DOMAIN_REMOVE_SUBDOMAINS(full_hostname)= │ Syntax error @ 1:39 token recognition │ false ║
║ Metron/PARSER/squid/fieldTransformations/0/Field                       │                                          │ error at: '='                         │       ║
║ Mapping/domain_without_subdomains                                      │                                          │                                       │       ║
╟────────────────────────────────────────────────────────────────────────┼──────────────────────────────────────────┼───────────────────────────────────────┼───────╢
║ Apache                                                                 │ not(IN_SUBNET(ip_dst_addr,               │                                       │ true  ║
║ Metron/ENRICHMENT/snort/threatIntel/triageConfig/riskLevelRules/0/rule │ '192.168.0.0/24'))                       │                                       │       ║
╚════════════════════════════════════════════════════════════════════════╧══════════════════════════════════════════╧═══════════════════════════════════════╧═══════╝

For all changes:

  • Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
  • Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
  • Has your PR been rebased against the latest commit within the target branch (typically master)?

For code changes:

  • Have you included steps to reproduce the behavior or problem that is being changed or addressed?

  • Have you included steps or a guide to how the change may be verified and tested manually?

  • Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:

    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?

  • 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 via site-book/target/site/index.html:

    cd site-book
    mvn site
    

This will allow users to check their deployed statements, say after upgrade, when they are at rest ( and would fail on use ).
In other words, they were valid when stored, but are not now because of stellar changes, such as new keywords.

The interface StellarConfiguredStatementReporter, which is @IndexSubclasses marked, allows the shell to discover
reporters that can provide statements for validation.  This discovery allows de-coupling of stellar and 'hosts' that
know about the location of the stored statements, and the configuration structure details.

We do mention the configurations in the shell output at this time.

metron-common implements this interface, and can run through visiting all the configurations.
@cestella
Copy link
Member

Any chance we can add a VALIDATE(str, type) function to the stellar management functions where str is the json blob string for the config and the type is the type of config? Generally the goal is to disallow invalid stellar to get pushed to zookeeper via zk_load_utils.sh, so I suspect a function would be more useful in the situation where you're constructing a config in the REPL via management functions and want to validate it before pushing it.

@cestella
Copy link
Member

Also, it might be useful for %validate_configured_expressions to take a file path so you can validate a set of configs on disk (again, if it gets to zookeeper, zk_load_utils.sh should fail if it's invalid)

@simonellistonball
Copy link
Contributor

@cestella I would say that proposed validate function has to be very much in a namespace. It feels like a name that would be much more useful for a function replacing our current approach to global validation in the future than config validation, other than that it sounds like a good idea.

@ottobackwards
Copy link
Contributor Author

So, the scenario here is checking things that were valid when uploaded, but have been invalidated by external changes ( language changes ). I would like to keep the magic specific.

I think the functionality for the management functions is valid, but can we do that as a separate Jira/PR? I'll do it, I just want to keep this tight. If you create the jira and assign it to me that would be super.

I would do the files on disk using the management functions as well.

So we just need to think of the stellar interface for calling
VALIDATE with a string, and with a file path. Also saying what configuration type it is.

Does that make sense?

@ottobackwards
Copy link
Contributor Author

@simonellistonball, yes, the namespace should be part of the jira and interface design

}
List<String> names = Arrays
.asList(getName(), ENRICHMENT.toString(), topicName, "THREAT_TRIAGE", name);
visitor.visit(names, r.getRule());
Copy link
Contributor

Choose a reason for hiding this comment

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

I like what you are doing with this PR.

There is just one downside that bugs me. How do we ensure that as our configuration evolves over time that we also update this to ensure it gets validated?

We have Stellar expressions as configurations all over the place; parsing, enrichment, triage, indexing, and profiler. I feel like any/all of these will evolve and change over time. I would definitely forget to come here and update this to make sure it gets validated.

As a motivating example, right here you are only validating the rule field. But triage also has the reason field that is a Stellar expression. That would need to be validated also. That was something we added later and likely the scenario where I would forget to add that expression to your validation logic.

Copy link
Contributor

@nickwallen nickwallen Nov 30, 2017

Choose a reason for hiding this comment

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

And let me follow-on with one proposal that might help address this. There are probably other (better?) ways to solve this, but here is one approach to chew on.

All of our configuration gets deserialized from JSON into POJOs; EnrichmentConfig, ProfilerConfig, etc. What if we had an annotation that we marked the fields that are required to be valid Stellar expressions? The annotation would go in those 'config' POJO classes. Based on the annotation we can then validate each of those fields.

Since the annotation is directly in the configuration classes, it is less likely I'm going to forget that annotation. And it is also remains decoupled, which is a good benefit of your current approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to think about this, but I will add the reason check

Copy link
Contributor

Choose a reason for hiding this comment

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

This also makes it simple to validate the configuration, no matter where it rests.

To validate the configuration in Zk, you just read all the configs in from Zk, deserialize into POJOs, iterate all the fields looking for the annotation, if you find the annotation, then run your validation logic on the expression.

To validate the configuration on the file system, you just read it all in from the FS, then repeat the same steps I described in the Zk scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So - if I'm understanding you correctly, as it applies to what I am doing ->
when visiting configs, instead of explicitly validating fields, we would want to 'visit' all the members per pojo by attribute. We would only need to worry about tracking the pojos but not the fields ( we need to keep the context ). Am I understanding you correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we eventually want to get to that point, with a consistent, comprehensive mechanism to validate stellar statements ( short of compiling ;) ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I know how to do it and not have it be terrible. The question is if it should be in this PR or after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can do it such that the stellar-common holds the code for doing the traversals as well as the interfaces and callbacks, and the reporter just provides the configs.

I'm thinking it through, but now that I'm thinking of it I will be miserable until I do it so I'll do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you are really going to like this @nickwallen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but if you complain about scope or the review size @nickwallen I will be very cross

@cestella
Copy link
Member

@simonellistonball Agree to the namespace idea. My bad :)

@ottobackwards
Copy link
Contributor Author

I am glad for the interest in this PR, and that it seems to have sparked some great ideas for continuing on.

What I would like to do is line it up as follows

  1. This PR with it's current scope and focus
  2. New Jira and PR(s) for the Stellar Functions/Namespace that @cestella and @simonellistonball mentioned
  3. Some research and possible prototyping of the Attributed approach @nickwallen has suggested ( which I agree with )

Over the course of that work, and other work identified through working and reviewing it, I will iteratively refactor to a common code and reusable approach.

@ottobackwards
Copy link
Contributor Author

Per the conversation above, i'm going to take a stab at the attributed approach.
I think the Stellar Functions should be a separate Jira.

Although the original implementation was functional, it required maintainence to keep current.
The suggested 'best state' was to have it be possible, maybe through annotations, for the validation
system to be able to handle any config, regarless or composition using annotations.
That would leave it up to the implementor to propertly annotate thier configurations, and allow for support of new fields.

This is an implementation of that.

I have refactored the implemenations and details, but kept the discovery and mechanics ( loading and visitation ) somewhat the same.
Hopefully keeping the good and reworking to a more sustainable solution.

Several annotations where created to marks ceratin stellar configruation objects or scenarios.
A holder object, to hold the configuration object, but knows how to process the annotations and run the visitation was added.
This holder object and the annotations have parameters and handling for several special scenarios, such as 2x nested maps.

This implementation should facilitate follow on work to validate files and streams and blobs by using implementing the StellarValidator interface
and re-using the holder concept ( replacing the providers )
@ottobackwards
Copy link
Contributor Author

ottobackwards commented Dec 1, 2017

65278a6 introduces conceptually what @nickwallen and I have been discussing.
I need to think about reworking the description.

From the commit ->

Refactor based on review and inspiration from review.
Although the original implementation was functional, it required maintainence to keep current.
The suggested 'best state' was to have it be possible, maybe through annotations, for the validation
system to be able to handle any config, regarless or composition using annotations.
That would leave it up to the implementor to propertly annotate thier configurations, and allow for support of new fields.

This is an implementation of that.

I have refactored the implemenations and details, but kept the discovery and mechanics ( loading and visitation ) somewhat the same.
Hopefully keeping the good and reworking to a more sustainable solution.

Several annotations where created to marks certain stellar configuration objects or scenarios.
A holder object, to hold the configuration object, but knows how to process the annotations and run the visitation was added.
This holder object and the annotations have parameters and handling for several special scenarios, such as 2x nested maps.

This implementation should facilitate follow on work to validate files and streams and blobs by using implementing the StellarValidator interface
and re-using the holder concept ( replacing the providers )

@ottobackwards
Copy link
Contributor Author

stellar_validate

@ottobackwards
Copy link
Contributor Author

@cestella @simonellistonball
With the new implementation, doing the blob or file check should be a piece of cake.... would you prefer it as part of this or as a new issue?

@ottobackwards
Copy link
Contributor Author

@nickwallen @mattf-horton I think we can use the annotation approach to resolve METRON-989 as well.

Thoughts?

@ottobackwards
Copy link
Contributor Author

Closing to test build in travis

@ottobackwards ottobackwards reopened this Dec 8, 2017
@ottobackwards
Copy link
Contributor Author

Bump?

@ottobackwards
Copy link
Contributor Author

@nickwallen any feedback, does the annotated approach match what you imagined?

@nickwallen
Copy link
Contributor

@ottobackwards Yes, definitely. I like it at a 50k foot level. The only thing that struck me was the need for the different annotation types. But I haven't had a chance to dig into it yet.

@ottobackwards
Copy link
Contributor Author

@nickwallen yeah, we need to cover a bunch a scenarios

@nickwallen
Copy link
Contributor

Why is this validation process driven by a %magic command?

Magics were made for functionality that cannot be implemented directly within a Stellar execution environment. Often for answering questions 'about' the execution environment itself. For example %vars or %functions or %globals all tell us about the Stellar execution environment.

This logic doesn't seem like a good fit for a %magic, unless there is a limitation that I am not understanding. This could be implemented, I would argue more simply, as a regular Stellar function.

@ottobackwards
Copy link
Contributor Author

It did not seem appropriate to me for this to be a stellar function, and %magic is the other way to execute things from the shell. At the time at least.

@ottobackwards
Copy link
Contributor Author

In other words, a stellar function that called the stellar compilation stuff, did not seem correct.

@ottobackwards
Copy link
Contributor Author

Do you feel strongly that this should be a Function? @cestella ? I'm not opposed to changing it if you are. I would like to here some more feedback

@nickwallen
Copy link
Contributor

If that question is to me too, yes, I feel strongly it should be a function. A function that is part of the management functions. That was also suggested previously here.

@nickwallen
Copy link
Contributor

nickwallen commented Dec 20, 2017

The justification that you mentioned just doesn't seem strong enough to me. Unless there is more that I am missing.

IMHO We should only use magic commands for things that can't be accomplished using the preferred extension mechanism; aka defining Stellar functions.

@ottobackwards
Copy link
Contributor Author

Ok, I'll change it. Feels a little crossing the streams, but we'll see

@ottobackwards
Copy link
Contributor Author

I don't think it should be in management necessarily though.

@ottobackwards
Copy link
Contributor Author

@nickwallen I have refactored to a function.
see updated PR description

*
* @return Field Name or empty String
*/
String qualify_with_field() default "";
Copy link
Contributor

Choose a reason for hiding this comment

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

These variables_go_against_the_style convention, no? qualify_with_field, qualify_with_field_type, inner_map_keys

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

try {
children = client.getChildren().forPath(PARSER.getZookeeperRoot());
} catch (Exception nne) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to log and comment here. We are silently eating the exception. Seems especially problematic because of the overly generic Exception declaration that the Curator library gives us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

try {
children = client.getChildren().forPath(ENRICHMENT.getZookeeperRoot());
} catch (Exception nne) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to log and comment here. We are silently eating the exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

List<ExpressionConfigurationHolder> holders = new LinkedList<>();
visitParserConfigs(client, holders, errorConsumer);
visitEnrichmentConfigs(client, holders, errorConsumer);
visitProfilerConfigs(client, holders, errorConsumer);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about indexing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no stellar in indexing is there? It is not in the readme

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right. Would suggest just a comment to clarify that point and maybe help prompt us should that change

// indexing contains no stellar to validate

// discover all the StellarConfigurationProvider
Set<StellarConfigurationProvider> providerSet = new HashSet<>();

for (Class<?> c : ClassIndex.getSubclasses(StellarConfigurationProvider.class,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the following code block doing? Why do we need to discover all of the StellarConfigurationProvider classes?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, it seems that the StellarConfigurationProvider interface allows us to extend where configuration values get pulled in from. In your current default implementation ConfigurationProvider you reach out to Zookeeper to pull in the config.

If I wanted to validate configuration located on a file system, I would just create a FilesystemConfigurationProvider implementation of this interface.

The decision as to whether I want to validate the config in Zookeeper, on the file system or both, needs to be user driven. A user should make that decision based on how they call your new Stellar function.

Based on your discovery logic here, just having a FilesystemConfigurationProvider on the classpath (or any other implementation) will cause the configuration in the file system to get validated. We don't want that to happen. We want the user to control that behavior.

So I don't think we really need this discovery logic, which nicely simplifies things. I think we could just alter the StellarValidater interface to make this relationship simpler and more straight forward. The StellarConfigurationProvider just gets passed in.

StellarValidator {
   ...
   Iterable<ValidationResult> validate(StellarConfigurationProvider provider);
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that not only can we not know the details of the classes that hold the rules, but also, that stellar may be hosted by other things than metron, that 'know' how to provide those configurations.

The problem with this isn't the discovery per se, but in that it is not correct given it's purpose and the implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not following your explanation of why we need the discovery logic. Can you try to explain it again? In the latest commits, I still see the discovery logic in StellarSimpleValidator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to think of a different way to put it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that is where we are missing each other?

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean couple it to metron? We don't want to do that anymore.

I don't see your justification. Maybe another reviewer will understand this better. As is, I am a +0 on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://issues.apache.org/jira/browse/METRON-989
https://issues.apache.org/jira/browse/METRON-876

@mattf-horton @cestella

I have implemented this as to not increase the amount of tie-in between stellar and metron, and support future non-metron configuration sources.

Thus, the sources of the configuration are discovered and not coupled. I believe this is in the spirit if not the letter of the design discussions that we have had.

Can you take a look?

Copy link
Member

Choose a reason for hiding this comment

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

It is true that Stellar should exist, as much as possible, independent from Metron; that was the aim of the effort to move stellar out of metron-common and into its own top level component in the project. I'll look closer at this PR and the (rather long, but seemingly coherent...so congrats ;) comment thread and weigh in later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cestella ping

* {@code ConfigurationProvider} is used to report all of the configured / deployed Stellar statements in
* the system.
*/
public class ConfigurationProvider implements StellarConfigurationProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

This implementation of a StellarConfigurationProvider is one that talks to Zookeeper, right? It retrieves Stellar configuration from Zookeeper.

Should we call it ZookeeperConfigurationProvider or something more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good idea

* StellarConfiguredStatementProviders are used provide stellar statements
* and the context around those statements to the caller
*/
public interface StellarConfiguredStatementContainer {
Copy link
Contributor

Choose a reason for hiding this comment

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

ExpressionConfigurationHolder is an implementation of this interface. That being said, I don't understand the point of this interface.

In all the code that I see, you use the implementation class ExpressionConfigurationHolder rather than this interface. For example, in StellarZookeeperBasedValidator and other places.

We should either use the interface or get rid of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is not using the interface, I'll address that.

* Returns: The String representation of the enrichment config

### Validation Functions
* `VALIDATE_STELLAR_RULE_CONFIGS`
Copy link
Contributor

Choose a reason for hiding this comment

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

This name confuses me, why Rules? VALIDATE_STELLAR seems simple and to the point to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is you search the tree for 'rules' you will see that we call them rule or rules in various places. It 'seemed like the thing to do'.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is reference to "rules" because that was the first use for Stellar. @cestella called them rules when he first implemented. But that is ancient history. We use Stellar everywhere now. Its long ago outgrown that.



@Override
public Iterable<ValidationResult> validate(Optional<LineWriter> writer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a 'writer'? It doesn't make sense to me why we need it nor do we use it. I think it could be removed from the method signature completely in StellarValidator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was going to take this out, this is from the prior shell based integration

Attempt to correct the validator from assuming zookeeper.
It is more correct that there will be multple types of validators

We may support injection of more than one type of validator, make that more clear.
@ottobackwards
Copy link
Contributor Author

Refactored based on feedback for some things, based on making what I was trying for more correct in others.

@ottobackwards
Copy link
Contributor Author

Resolved conflicts, in the event we ever get around to this

@ottobackwards
Copy link
Contributor Author

Should I close this?

@ottobackwards
Copy link
Contributor Author

This has gone from a small thing to at least 'say' we have a way to check if we broke all your stellar stuff after upgrade, to stretching it based on feedback which was a mistake, to a run around to abandoned review status.

I'm going shelve this

@ottobackwards
Copy link
Contributor Author

The next attempt at this, if there is one, should start off with some sort of consensus first. And some agreement on initial scope. This PR would have been smaller and less ambitious, if that were true. Or at least everyone would have been on the same page as to the 'why' of certain things.
Lessons to learn :)

@ottobackwards
Copy link
Contributor Author

It has been long enough that I don't even like this PR any more.

-1

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants