-
Notifications
You must be signed in to change notification settings - Fork 83
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
agent-mode loads output from policy #3411
Changes from 17 commits
1bf87b1
9bbe8ca
e46ab6c
6c615be
a5aa0d4
d66eb89
0f9c8f9
8d465fd
98c13f3
389a444
1c030e3
756556b
efa0eaf
45e234d
6989552
fb6d15c
737a4e0
fa72499
555aa30
fa4a0f4
0e70530
91531a0
317ae3d
2c2469a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
# Kind can be one of: | ||
# - breaking-change: a change to previously-documented behavior | ||
# - deprecation: functionality that is being removed in a later release | ||
# - bug-fix: fixes a problem in a previous version | ||
# - enhancement: extends functionality but does not break or fix existing behavior | ||
# - feature: new functionality | ||
# - known-issue: problems that we are aware of in a given version | ||
# - security: impacts on the security of a product or a user’s deployment. | ||
# - upgrade: important information for someone upgrading from a prior version | ||
# - other: does not fit into any of the other categories | ||
kind: feature | ||
|
||
# Change summary; a 80ish characters long description of the change. | ||
summary: Use policy outputs when running in agent-mode | ||
|
||
# Long description; in case the summary is not enough to describe the change | ||
# this field accommodate a description without length limits. | ||
# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. | ||
description: | | ||
Fleet-server will retrieve and use the output from the policy when running in agent-mode. | ||
This allows the fleet-server to connect to multiple Elasticsearch hosts if it is successful when | ||
connecting to the host provided at enrollment/installation. | ||
We expect that the host provided during enrollment/installation is never removed as a valid output. | ||
fleet-server does not persist output settings it retrieves locally so it must always be able to connect | ||
with options specified at enrollment/installation. | ||
|
||
# Affected component; a word indicating the component this changeset affects. | ||
component: | ||
|
||
# PR URL; optional; the PR number that added the changeset. | ||
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. | ||
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. | ||
# Please provide it if you are adding a fragment for a different PR. | ||
pr: 3411 | ||
|
||
# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). | ||
# If not present is automatically filled by the tooling with the issue linked to the PR number. | ||
issue: https://github.com/elastic/elastic-agent/issues/2784 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -382,3 +382,170 @@ func setTestEnv(t *testing.T, env map[string]string) { | |
t.Setenv(k, v) | ||
} | ||
} | ||
|
||
func TestMergeElasticsearchFromPolicy(t *testing.T) { | ||
cfg := Elasticsearch{ | ||
Protocol: "http", | ||
Hosts: []string{"elasticsearch:9200"}, | ||
ServiceToken: "token", | ||
Timeout: time.Second, | ||
MaxRetries: 1, | ||
MaxConnPerHost: 1, | ||
MaxContentLength: 1, | ||
} | ||
tests := []struct { | ||
name string | ||
pol Elasticsearch | ||
res Elasticsearch | ||
}{{ | ||
name: "default policy", | ||
pol: Elasticsearch{ | ||
Hosts: []string{"localhost:9200"}, | ||
Timeout: DefaultElasticsearchTimeout, | ||
MaxRetries: DefaultElasticsearchMaxRetries, | ||
MaxConnPerHost: DefaultElasticsearchMaxConnPerHost, | ||
MaxContentLength: DefaultElasticsearchMaxContentLength, | ||
}, | ||
res: Elasticsearch{ | ||
Protocol: "http", | ||
Hosts: []string{"elasticsearch:9200"}, | ||
ServiceToken: "token", | ||
Timeout: time.Second, | ||
MaxRetries: 1, | ||
MaxConnPerHost: 1, | ||
MaxContentLength: 1, | ||
}, | ||
}, { | ||
name: "hosts differ", | ||
pol: Elasticsearch{ | ||
Protocol: "https", | ||
Hosts: []string{"elasticsearch:9200", "other:9200"}, | ||
Timeout: DefaultElasticsearchTimeout, | ||
MaxRetries: DefaultElasticsearchMaxRetries, | ||
MaxConnPerHost: DefaultElasticsearchMaxConnPerHost, | ||
MaxContentLength: DefaultElasticsearchMaxContentLength, | ||
}, | ||
res: Elasticsearch{ | ||
Protocol: "https", | ||
Hosts: []string{"elasticsearch:9200", "other:9200"}, | ||
ServiceToken: "token", | ||
Timeout: time.Second, | ||
MaxRetries: 1, | ||
MaxConnPerHost: 1, | ||
MaxContentLength: 1, | ||
}, | ||
}, { | ||
name: "all non tls attributes differ", | ||
pol: Elasticsearch{ | ||
Protocol: "https", | ||
Hosts: []string{"elasticsearch:9200", "other:9200"}, | ||
Headers: map[string]string{"custom": "value"}, | ||
ProxyURL: "http://proxy:8080", | ||
ProxyDisable: false, | ||
ProxyHeaders: map[string]string{"proxyhead": "proxyval"}, | ||
Timeout: time.Second * 2, | ||
MaxRetries: 2, | ||
MaxConnPerHost: 3, | ||
MaxContentLength: 4, | ||
}, | ||
res: Elasticsearch{ | ||
Protocol: "https", | ||
Hosts: []string{"elasticsearch:9200", "other:9200"}, | ||
Headers: map[string]string{"custom": "value"}, | ||
ProxyURL: "http://proxy:8080", | ||
ProxyDisable: false, | ||
ProxyHeaders: map[string]string{"proxyhead": "proxyval"}, | ||
ServiceToken: "token", | ||
Timeout: 2 * time.Second, | ||
MaxRetries: 2, | ||
MaxConnPerHost: 3, | ||
MaxContentLength: 4, | ||
}, | ||
}} | ||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
res := MergeElasticsearchFromPolicy(cfg, tc.pol) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @michel-laterman why do we need to merge the policy here? cannot use what come from the config? the proxy and tls settings should be configured there too no? otherwise there is no way for user to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the elastic-agent injection will not replace keys that are already present in local config: So the agent can set |
||
assert.Equal(t, tc.res.Protocol, res.Protocol) | ||
require.Len(t, res.Hosts, len(tc.res.Hosts)) | ||
for i, host := range tc.res.Hosts { | ||
assert.Equalf(t, host, res.Hosts[i], "host %d does not match", i) | ||
} | ||
require.Len(t, res.Headers, len(tc.res.Headers)) | ||
for k, v := range tc.res.Headers { | ||
assert.Equal(t, v, res.Headers[k]) | ||
} | ||
assert.Equal(t, tc.res.ServiceToken, res.ServiceToken) | ||
assert.Equal(t, tc.res.ServiceTokenPath, res.ServiceTokenPath) | ||
assert.Equal(t, tc.res.ProxyURL, res.ProxyURL) | ||
assert.Equal(t, tc.res.ProxyDisable, res.ProxyDisable) | ||
require.Len(t, res.ProxyHeaders, len(tc.res.ProxyHeaders)) | ||
for k, v := range tc.res.ProxyHeaders { | ||
assert.Equal(t, v, res.ProxyHeaders[k]) | ||
} | ||
assert.Nil(t, res.TLS) | ||
assert.Equal(t, tc.res.MaxRetries, res.MaxRetries) | ||
assert.Equal(t, tc.res.MaxConnPerHost, res.MaxConnPerHost) | ||
assert.Equal(t, tc.res.Timeout, res.Timeout) | ||
assert.Equal(t, tc.res.MaxContentLength, res.MaxContentLength) | ||
}) | ||
} | ||
} | ||
|
||
func TestMergeElasticsearchTLS(t *testing.T) { | ||
enabled := true | ||
disabled := false | ||
t.Run("both nil", func(t *testing.T) { | ||
res := mergeElasticsearchTLS(nil, nil) | ||
assert.Nil(t, res) | ||
}) | ||
t.Run("cfg not nil", func(t *testing.T) { | ||
res := mergeElasticsearchTLS(&tlscommon.Config{ | ||
Enabled: &enabled, | ||
VerificationMode: tlscommon.VerifyFull, | ||
}, nil) | ||
require.NotNil(t, res) | ||
assert.True(t, *res.Enabled) | ||
assert.Equal(t, tlscommon.VerifyFull, res.VerificationMode) | ||
}) | ||
t.Run("pol not nil", func(t *testing.T) { | ||
res := mergeElasticsearchTLS(nil, &tlscommon.Config{ | ||
Enabled: &enabled, | ||
VerificationMode: tlscommon.VerifyFull, | ||
}) | ||
require.NotNil(t, res) | ||
assert.True(t, *res.Enabled) | ||
assert.Equal(t, tlscommon.VerifyFull, res.VerificationMode) | ||
}) | ||
t.Run("both not nil", func(t *testing.T) { | ||
res := mergeElasticsearchTLS(&tlscommon.Config{ | ||
Enabled: &disabled, | ||
VerificationMode: tlscommon.VerifyFull, | ||
}, &tlscommon.Config{ | ||
Enabled: &enabled, | ||
VerificationMode: tlscommon.VerifyCertificate, | ||
Versions: []tlscommon.TLSVersion{tlscommon.TLSVersion13}, | ||
CipherSuites: []tlscommon.CipherSuite{tlscommon.CipherSuite(tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA)}, | ||
CAs: []string{"/path/to/ca.crt"}, | ||
Certificate: tlscommon.CertificateConfig{ | ||
Certificate: "/path/to/cert.crt", | ||
Key: "/path/to/key.crt", | ||
}, | ||
CASha256: []string{"casha256val"}, | ||
CATrustedFingerprint: "fingerprint", | ||
}) | ||
require.NotNil(t, res) | ||
assert.True(t, *res.Enabled) | ||
assert.Equal(t, tlscommon.VerifyCertificate, res.VerificationMode) | ||
require.Len(t, res.Versions, 1) | ||
assert.Equal(t, tlscommon.TLSVersion13, res.Versions[0]) | ||
require.Len(t, res.CipherSuites, 1) | ||
assert.Equal(t, tlscommon.CipherSuite(tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA), res.CipherSuites[0]) | ||
require.Len(t, res.CAs, 1) | ||
assert.Equal(t, "/path/to/ca.crt", res.CAs[0]) | ||
assert.Equal(t, "/path/to/cert.crt", res.Certificate.Certificate) | ||
assert.Equal(t, "/path/to/key.crt", res.Certificate.Key) | ||
require.Len(t, res.CASha256, 1) | ||
assert.Equal(t, "casha256val", res.CASha256[0]) | ||
assert.Equal(t, "fingerprint", res.CATrustedFingerprint) | ||
}) | ||
} |
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.
Should we raise an ingest-docs issue to document this feature?
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, we should