Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Elastic Agent] Fix issues with dynamic inputs and conditions #23886

Merged
merged 5 commits into from
Feb 11, 2021

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Feb 5, 2021

What does this PR do?

Fixes 2 issues that where affecting the usage of dynamic inputs and conditions.

  • Loading of the configuration was not excluding the inputs from the go-ucfg variable expansion. Inside of inputs on variable expansion is done by the dynamic inputs code.
  • EQL string methods where too strict on type checks, this lowers that to try to convert any type to a string before doing the comparison. This is important in cases where a variable results to an un-expected type, like *eql.null.

Why is it important?

Usage of dynamic inputs work as expected.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@blakerouse blakerouse added bug needs_backport PR is waiting to be backported to other branches. Team:Elastic-Agent Label for the Agent team labels Feb 5, 2021
@blakerouse blakerouse self-assigned this Feb 5, 2021
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 5, 2021
@blakerouse blakerouse marked this pull request as ready for review February 5, 2021 19:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@ph ph added the review label Feb 5, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 5, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #23886 updated

  • Start Time: 2021-02-10T17:06:24.295+0000

  • Duration: 26 min 9 sec

  • Commit: df7a3c5

Test stats 🧪

Test Results
Failed 0
Passed 5862
Skipped 16
Total 5878

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 5862
Skipped 16
Total 5878

Copy link
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

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

tested ok with standalone config, code lgtm
looks like config from #23685 works

return newConfigFrom(c), err
}

c, err := ucfg.NewFrom(from, opts...)
return newConfigFrom(c), err
var c map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we generalize this a bit? e.g have a list of keywords where we want to skip ucfg resolution.
i think extraction to func and var list of keys would be enough

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@michalpristas
Copy link
Contributor

/pacakge

@ph
Copy link
Contributor

ph commented Feb 8, 2021

/package

1 similar comment
@blakerouse
Copy link
Contributor Author

/package

@blakerouse
Copy link
Contributor Author

@michalpristas I cleaned up the config loading more. See what you think.

Copy link
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

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

I think it looks ok. i like having load defines inside config package

}
local := &options{}
for _, o := range localOpts {
o(local)
Copy link
Contributor

Choose a reason for hiding this comment

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

with multiple VarSkipKeys last one wins right? should we allow multiple entries by combining them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes last wins. If we combine them, then no way to overwrite.... idk its a choice

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm bringing this up just to verify it's a decision not a bug

@ChrsMark
Copy link
Member

ChrsMark commented Feb 9, 2021

Thank you for fixing this one @blakerouse . Could you provide an example of valid configuration based on your fixes (dynamic input + env vars), or point me to a docs page which includes such examples? It's still not clear to me what is the proper way to combine dynamic inputs and env vars, do you still need env. prefix when refer to env variables in configuration like at

data_stream:
  dataset: kubernetes.container
  type: metrics
  metricsets:
  - container
  add_metadata: true
  bearer_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
  hosts:
  - 'https://${env.NODE_NAME}:10250'

?

@blakerouse
Copy link
Contributor Author

@ChrsMark Yes you still need to use ${env.NODE_NODE}. Inside of inputs is only resolved by the providers so it must start with the name of the provider first. So env.* in the case you want to get it from the environment variable.

@blakerouse
Copy link
Contributor Author

/package

@ChrsMark
Copy link
Member

Hey, is there anything missing with this so as to have it in and unblock #23679?

@blakerouse
Copy link
Contributor Author

/package

@blakerouse
Copy link
Contributor Author

Tested locally with same built package and both Fleet mode and standalone work. So E2E test failed for an unknown reason.

@blakerouse blakerouse merged commit de7906b into elastic:master Feb 11, 2021
@blakerouse blakerouse deleted the fix-agent-dynamic-inputs branch February 11, 2021 14:50
@blakerouse blakerouse added v7.12.0 and removed needs_backport PR is waiting to be backported to other branches. labels Feb 11, 2021
blakerouse added a commit to blakerouse/beats that referenced this pull request Feb 11, 2021
…c#23886)

* Fix loading of configurations to not parse variables in inputs. Fix issue with EQL string methods.

* Run fmt.

* Add changelog entry.

* Cleanup the config loading more.

* Fix NewConfigFrom to handle maps.

(cherry picked from commit de7906b)
blakerouse added a commit to blakerouse/beats that referenced this pull request Feb 11, 2021
…c#23886)

* Fix loading of configurations to not parse variables in inputs. Fix issue with EQL string methods.

* Run fmt.

* Add changelog entry.

* Cleanup the config loading more.

* Fix NewConfigFrom to handle maps.

(cherry picked from commit de7906b)
blakerouse added a commit that referenced this pull request Feb 11, 2021
#24004)

* Fix loading of configurations to not parse variables in inputs. Fix issue with EQL string methods.

* Run fmt.

* Add changelog entry.

* Cleanup the config loading more.

* Fix NewConfigFrom to handle maps.

(cherry picked from commit de7906b)
blakerouse added a commit that referenced this pull request Feb 11, 2021
#24005)

* Fix loading of configurations to not parse variables in inputs. Fix issue with EQL string methods.

* Run fmt.

* Add changelog entry.

* Cleanup the config loading more.

* Fix NewConfigFrom to handle maps.

(cherry picked from commit de7906b)
@ChrsMark ChrsMark mentioned this pull request Feb 16, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants