JAVAFICATION: Move env/secret variable substitution to java #9619
Conversation
| @@ -0,0 +1,64 @@ | |||
| package org.logstash.secret; | |||
There was a problem hiding this comment.
Moved from being an inner class in the secret store tests
andrewvc
force-pushed
the
port-substitution-variables
branch
3 times, most recently
from
feef05b to
254557a
Compare
andrewvc
force-pushed
the
port-substitution-variables
branch
2 times, most recently
from
d29e02b to
068a515
Compare
| org.logstash.common.SubstitutionVariables.replacePlaceholders( | ||
| value, ENV, LogStash::Util::SecretStore.get_if_exists | ||
| ); | ||
| rescue org.logstash.common.SubstitutionVariables::MissingSubstitutionVariableError => e |
There was a problem hiding this comment.
@andrewvc are you sure this is caught here? Instinctively I'd say now actually. Maybe simply throw that ConfigurationError on the Java side in the first place?
Syntax for that would be:
throw RubyUtil.RUBY.newRaiseException(
RubyUtil.CONFIGURATION_ERROR_CLASS,
"Your Message"
);:)
There was a problem hiding this comment.
What do you mean by "am I sure that is caught?" As in, is the thrown exception caught, or is the rescue block here doing its work?
I'd prefer not to wrap this with a JRuby ext wrapper that throws ruby exceptions unless we need it.
This does work correctly today and is caught by https://github.com/elastic/logstash/blob/master/logstash-core/lib/logstash/util/settings_helper.rb#L41
I verified this manually.
There was a problem hiding this comment.
@andrewvc yea that's what I meant, all good then :)
|
@andrewvc some strange test failures here in https://logstash-ci.elastic.co/job/elastic+logstash+pull-request+multijob-ruby-unit-tests/629/console |
|
Jenkins test this |
|
@original-brownbear Yup, I'll take a look today. |
|
jenkins, please retest this |
1 similar comment
|
jenkins, please retest this |
andrewvc
force-pushed
the
port-substitution-variables
branch
4 times, most recently
from
e9ad847 to
c10283e
Compare
|
Jenkins, test this please |
|
OK, so I pushed up a new commit that makes us load the secret store once per substitution vs. once per invocation of the substitution function. The secret store, on my laptop, takes 300ms to start up. This is long enough to cause the test to time out. In the future we should find some way to cache it and invalidate it if the file contents change. That's more work than appropriate for this PR however. |
|
jenkins, test this please |
This is a partial port that required writing our own version of ruby's gsub. Java has `String#replaceAll`, but that doesn't let you pass in a function to do the replacement.
These exceptions are non-fatal per the specs
This is very slow and caused some specs to time out
andsel
force-pushed
the
port-substitution-variables
branch
from
4ca7845 to
9664be5
Compare
|
Aligned with |
This is a partial port of the string substitution code that required writing our own version of ruby's gsub.
Java has
String#replaceAll, but that doesn't let you pass in a function to do the replacement.Porting the whole SubstitutionVariables class is a little tricky without also replacing the Settings class.