[Elastic Agent] Add support for variable replacement from providers #20839
Conversation
botelastic
bot
added
needs_team
blakerouse
force-pushed
the
agent-ast-variable-replace
branch
from
6dfddcc to
96bc36c
Compare
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
There was a problem hiding this comment.
- Could you add a log message somewhere that this is still experimental? We probably will need it also per provider so we can decide for each provider if it is GA or not.
- What is the current order / overwrite of variables if multiple composable providers are used?
Yes I will add a log message on startup to document that dynamic inputs are experimental.
What is great about this implementation is you really don't need to worry about order, there is no magic or undefined behavior in ordering.
Lets say that an input wants to allow a local variable override from the That input defines the order of precedence and Elastic Agent doesn't need to worry about collisions or order of precedence, all because its encoded in the input definition by either the user or the integration package. Another powerful this is the fallback from one dynamic provider to the next. This input will create both inputs for discovered docker containers and kubernetes containers. Because contexts from dynamic providers are never overlapped that means when |
|
I like that there is not "magic" around the order but the user can define it. We had initially that only the dynamic providers are prefixed, but it sounds like now all providers use prefixes, so also The part I stumbled over was |
|
@blakerouse Taking handlebars discussion here so it doesn't get lost when changes are pushed. I just realised that we potentially should not use the same syntax as in the packages for these variables as you initially suggested. We use handlebars in the package so we have if/else/loop support. But this is not something we support on the agent, it is only about variable replacements and applying conditions separately. So using Note: I know we had a similar conversation somewhere else too but couldn't find it anymore :-( |
|
Moved back to draft while I work on updating to the newest variable syntax. |
|
PR moved back to "Ready for review". Updated it to use new variable syntax |
| // | ||
| // This must be used to load the Agent configuration, so that variables defined in the inputs are not | ||
| // parsed by go-ucfg. Variables from the inputs should be parsed by the transpiler. | ||
| func LoadConfigFromFile(path string) (*config.Config, error) { |
There was a problem hiding this comment.
Not official supported but it basically means someone can use ${ELASTIC_USERNAME} today in the output config because it is ucfg but in case we also support static providers like keystore in the output in the future, this might break as it needs a prefix?
There was a problem hiding this comment.
Yes this allows the same go-ucfg that we have always allowed anywhere in the config except in the inputs. But if we change that in the future then a prefix would be required to get it from the keystore provider.
There was a problem hiding this comment.
Lets keep it in mind if we ever there but ok for now.
| } | ||
| } | ||
| } | ||
|
|
||
| e.lock.Lock() |
There was a problem hiding this comment.
I keep stumbling over these lines and the ones on 112 and asking myself if we don't hit any race conditions somewhere or if we could do it in a nicer way.
There was a problem hiding this comment.
Being that its internal state to the struct that is called from 2 different go-routines, I don't really have another way of not requiring a lock.
This is also going to be important for the debugging commands to output the current state.
| if err != nil { | ||
| return err | ||
| e.logger.Errorf("Failed to render configuration with latest context from composable controller: %s", err) |
There was a problem hiding this comment.
Would it be an option to return the error instead of logging it here? This could make testing easier.
There was a problem hiding this comment.
It is actually very un-likely that an error will actually occur here, because most errors would come from invalid configuration files, like bad variable syntax or something of that nature. That is why the Update() returns the error, because that is caused when the configuration is parsed and will return an error back to the user or back to Fleet.
In the case that a provider changes a value in a variable or a context is removed it will just cause inputs to be add/update/removed. This can also happen at any time, so really logging it here is the only thing we can do.
| return NewStrValWithProcessors(result+value[lastIndex:], processors), nil | ||
| } | ||
|
|
||
| // validBrackets returns true when all starting {{ have a matching ending }}. |
There was a problem hiding this comment.
| // validBrackets returns true when all starting {{ have a matching ending }}. | |
| // validBrackets returns true when all starting ${ have a matching ending }. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| matchIdxs := varsRegex.FindAllSubmatchIndex([]byte(value), -1) |
There was a problem hiding this comment.
This reminds me we need some rules on what chars are allowed in variable names. Should we restrict it?
There was a problem hiding this comment.
At the moment varsRegex allows any Unicode character.
There was a problem hiding this comment.
Should we support all of them? Or reduce it to a-z and 2-3 special chars?
| Processors: processers, | ||
| } | ||
|
|
||
| //res, err := vars.Replace("${testing.key1}") |
| return nil, errors.New(err, "failed to unpack providers config", errors.TypeConfig) | ||
| return nil, err | ||
| } | ||
| l.Info("EXPERIMENTAL - Dynamic Inputs are currently experimental and should not be used in production") |
There was a problem hiding this comment.
| l.Info("EXPERIMENTAL - Dynamic Inputs are currently experimental and should not be used in production") | |
| l.Info("EXPERIMENTAL - Inputs with variables are currently experimental and should not be used in production") |
There was a problem hiding this comment.
When exactly is this logged? What kind of config needs the user to have to see this?
There was a problem hiding this comment.
This is logged at the start of Elastic Agent when the controller is created. It is always created even without variables in the configuration, so it always logged.
…ne-2.0 * upstream/master: [Metricbeat][test] Disable ec2 flaky test (elastic#20959) Check if tracer is active before starting a transaction (elastic#20852) [Elastic Agent] Add support for variable replacement from providers (elastic#20839) Only request wildcard expansion for hidden indices if supported (elastic#20938) [Ingest Manager] New agent structure (symlinks) (elastic#20400) [Ingest Manager] Print a message confirming shutdown (elastic#20948) Skip flaky test on unix input (elastic#20942) [Ingest Manager] Align introspect-inspect naming in code (elastic#20952) [Filebeat][zeek] Map new x509 fields for ssl module (elastic#20927) [CI] fix regression with variable name (elastic#20930) [Autodiscover] Handle input-not-finished errors in config reload (elastic#20915) [Ingest Manager] Remove Success from fleet contract (elastic#20449)
…-faster * upstream/master: [Metricbeat][test] Disable ec2 flaky test (elastic#20959) Check if tracer is active before starting a transaction (elastic#20852) [Elastic Agent] Add support for variable replacement from providers (elastic#20839) Only request wildcard expansion for hidden indices if supported (elastic#20938) [Ingest Manager] New agent structure (symlinks) (elastic#20400) [Ingest Manager] Print a message confirming shutdown (elastic#20948) Skip flaky test on unix input (elastic#20942) [Ingest Manager] Align introspect-inspect naming in code (elastic#20952) [Filebeat][zeek] Map new x509 fields for ssl module (elastic#20927)
❕ Build Aborted
Expand to view the summary
Build stats
Test stats 🧪
Steps errorsExpand to view the steps failures
Log outputExpand to view the last 100 lines of log output
|
…lastic#20839) * Add ability to replace variables in the parsed AST tree. * More vars replace improvements. * Perform the variable replacement of the elastic-agent configuration. * Clean-up testing and processors. * Add changelog. * Fix import sorting. * Add more validation to variable substitution. * Add log message about dynamic inputs being experimental. * Update to new variable format. Handle replace of complete objects. * Fix config importing to not replace vars in inputs. * Fixes for vet. * Fix fleet config change action to use new LoadConfig. * Fixes from code review. * Ensure processors are prepended to inputs. (cherry picked from commit 121f23b)
…20839) (#20964) * Add ability to replace variables in the parsed AST tree. * More vars replace improvements. * Perform the variable replacement of the elastic-agent configuration. * Clean-up testing and processors. * Add changelog. * Fix import sorting. * Add more validation to variable substitution. * Add log message about dynamic inputs being experimental. * Update to new variable format. Handle replace of complete objects. * Fix config importing to not replace vars in inputs. * Fixes for vet. * Fix fleet config change action to use new LoadConfig. * Fixes from code review. * Ensure processors are prepended to inputs. (cherry picked from commit 121f23b)
…lastic#20839) * Add ability to replace variables in the parsed AST tree. * More vars replace improvements. * Perform the variable replacement of the elastic-agent configuration. * Clean-up testing and processors. * Add changelog. * Fix import sorting. * Add more validation to variable substitution. * Add log message about dynamic inputs being experimental. * Update to new variable format. Handle replace of complete objects. * Fix config importing to not replace vars in inputs. * Fixes for vet. * Fix fleet config change action to use new LoadConfig. * Fixes from code review. * Ensure processors are prepended to inputs.
What does this PR do?
Adds support for replacing
{{local_dynamic.key|local_dynamic.other|'fallback'}}inside of the Elastic Agentinputsconfiguration.Why is it important?
Finishes the core support for #20781
More code is needed to help debugging effort.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.How to test this PR locally
Running the Elastic Agent in local mode, use the following configuration:
Then run the following to inspect the generated configuration:
Related issues