-
Notifications
You must be signed in to change notification settings - Fork 126
New stack provider for environment variables #2298
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
Conversation
3fca98e
to
cce94bd
Compare
Splitting PR in smaller changes. |
/test |
1a7f1d2
to
ffeb24c
Compare
// TODO: Migrate from serverless variables. | ||
config.Parameters[ParamServerlessLocalStackVersion] = options.StackVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be interesting to be agnostic of Serverless (it could also be a cloud hosted deployment).
If these parameters are migrated done, would it be needed to update also the environment variables ? Thinking about the CI jobs that create Serverless projects in integrations and elastic-package repositories.
As it is working now with the current parameters. Probably, this migration could be left for a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration would need to take into account the old environment variables and settings. Yes, we can do this in a follow up 👍
|
||
// TODO: Include these settings more explicitly in `config`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have those default values in config directly. In any case, I think this could be changed in a follow-up PR.
|
||
ip := net.ParseIP(url.Hostname()) | ||
if url.Hostname() == "localhost" || (ip != nil && ip.IsLoopback()) { | ||
const hostInternal = "host.docker.internal" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a special DNS name used by Docker desktop, would it cause any issue with users running elastic-package on Mac ?
https://docs.docker.com/desktop/features/networking/#i-want-to-connect-from-a-container-to-a-service-on-the-host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am explicitly adding the host in the docker compose with the hope it works the same on all platforms: https://github.com/elastic/elastic-package/pull/2298/files#diff-379a1fb4d0ceacb95a5ce10db57b58cf8c43aa694f7576a1f175f01ef507392fR65
extra_hosts:
- "host.docker.internal:host-gateway"
Though I haven't tested it in Windows or Mac yet.
If there is some conflict with docker desktop maybe another option is to use just another host.
} | ||
|
||
if r.shutdownAgentHandler != nil { | ||
if err := r.shutdownAgentHandler(cleanupCtx); err != nil { | ||
if r.deleteTestPolicyHandler != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reordering this because the agent deployer needs to get the YAML for the agent, and it contains an enrollment token now, that needs to be retrieved for an existing policy.
We could refactor tear down to don't require this token there, but in any case it makes sense to delete the policy after deleting the agents using it.
test integrations |
Created or updated PR in integrations repository to test this version. Check elastic/integrations#12386 |
test serverless |
Triggered serverless pipeline: https://buildkite.com/elastic/elastic-package-test-serverless/builds/276 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good!
Just added some questions and suggestions to use context variables.
client, err := fleetserver.NewClient(address, | ||
fleetserver.APIKey(config.ElasticsearchAPIKey), | ||
fleetserver.CertificateAuthority(config.CACertFile), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be local or a remote Fleet Server, is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right, it can be local for example when starting one for a local kibana development environment, or for a cloud deployment without integrations server, and it can be remote for a serverless project, or a cloud deployment with integrations server.
We could maybe rely on checking the local container for the cases of local deployments, but this approach works the same for both.
⏳ Build in-progress, with failures
Failed CI StepsHistory
cc @jsoriano |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Add a new stack provider that can be used when elastic-package is configured with environment variables. This provider fills the gaps for using elastic-package for testing on clusters that are not originally managed by elastic-package.
To use it, at least
ELASTIC_PACKAGE_KIBANA_HOST
andELASTIC_PACKAGE_ELASTICSEARCH_HOST
environment variables need to be set, other variables can be used to configure authentication, SSL and so on.Then, run
elastic-package stack up --provider environment
.This provider manages:
Most commands should work if the provider is able to setup everything.
Tested with ESS and Serverless using API keys.
Should work also with kibana development environment, but the stack needs to be started with visibility in the docker networks for the local services, that is, to start with:
It might also work if the docker network is previously known, using the IP of the host.
Fixes #1633
Fixes #2359