-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add integration tests using a proxy with mTLS for control plane with Elastic Defend installed #5889
Add integration tests using a proxy with mTLS for control plane with Elastic Defend installed #5889
Conversation
2ffe8aa
to
a16d046
Compare
This pull request is now in conflicts. Could you fix it? 🙏
|
33d59b8
to
403e023
Compare
8c96584
to
c7df66a
Compare
6ad71d0
to
563b607
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
… into 5716-5491-mtls-integration-test
c33f868
to
d673f76
Compare
folks (@blakerouse, @pchila, @kaanyalti), all the tests are passing now. In case you were waiting the tests to pass to review it |
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.
Overall this looks good.
I do question the change from returning errors to using require.NoError
directly. Why was that change made?
to follow the convention we have on our tests. The majority of the tests use this approach |
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.
A few comments here and there, mostly about readability
@@ -894,3 +901,536 @@ func TestForceInstallOverProtectedPolicy(t *testing.T) { | |||
out, err := fixture.Exec(ctx, args) | |||
require.Errorf(t, err, "No error detected, command output: %s", out) | |||
} | |||
|
|||
func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { |
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.
Nit: To help my brain tokenize the string maybe we can write it this way?
func TestInstallDefendWithMTLSandEncCertKey(t *testing.T) { | |
func TestInstallDefendWithmTlsAndEncCertKey(t *testing.T) { |
(I know that TLS is an acronym but I think that in this case the go linter will let it slide 😄 )
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.
what about mTLS -> TestInstallDefendWithmTLSAndEncCertKey
?
buff := &strings.Builder{} | ||
assert.Eventuallyf(t, func() bool { | ||
buff.Reset() | ||
|
||
got, err := f.ExecInspect(ctx) | ||
if err != nil { | ||
buff.WriteString(fmt.Sprintf("error running inspect cmd: %v", err)) | ||
return false | ||
} | ||
|
||
return proxyPolicymTLS.URL == got.Fleet.ProxyURL | ||
}, time.Minute, time.Second, "inspect never showed proxy from policy: %s", buff) |
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.
buff
is really the last error encountered in the assert.Eventually, maybe we can define and treat it as such:
buff := &strings.Builder{} | |
assert.Eventuallyf(t, func() bool { | |
buff.Reset() | |
got, err := f.ExecInspect(ctx) | |
if err != nil { | |
buff.WriteString(fmt.Sprintf("error running inspect cmd: %v", err)) | |
return false | |
} | |
return proxyPolicymTLS.URL == got.Fleet.ProxyURL | |
}, time.Minute, time.Second, "inspect never showed proxy from policy: %s", buff) | |
// feel free to change the variable name | |
var buff error | |
assert.Eventuallyf(t, func() bool { | |
got, err := f.ExecInspect(ctx) | |
if err != nil { | |
buff = fmt.Errorf("error running inspect cmd: %w", err) | |
return false | |
} | |
// (optional) reset the error | |
buff = nil | |
return proxyPolicymTLS.URL == got.Fleet.ProxyURL | |
}, time.Minute, time.Second, "inspect never showed proxy from policy: %s", buff) |
Quality Gate passedIssues Measures |
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.
LGTM
…Elastic Defend installed (#5889)
…or control plane with Elastic Defend installed (#5943) * add integration tests using a proxy with mTLS for control plane with Elastic Defend installed (#5889) (cherry picked from commit a338543) * update elastic-agent-libs so it works with 8.x stacks --------- Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>
…for control plane with Elastic Defend installed (#5942) * add integration tests using a proxy with mTLS for control plane with Elastic Defend installed (#5889) (cherry picked from commit a338543) # Conflicts: # testing/proxytest/https.go # testing/proxytest/proxytest.go * fix conflicts: use proxytest.go from main * update elastic-agent-libs to v0.17.3 --------- Co-authored-by: Anderson Queiroz <anderson.queiroz@elastic.co>
What does this PR do?
Add integration tests for the Elastic Agent running Elastic Defend with a mTLS proxy
See #5716 for tje covered test cases
Why is it important?
To have automated tests covering mTLS and Elastic Defend
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files[ ] I have added an entry in(https://github.com/elastic/elastic-agent#changelog)./changelog/fragments
using the [changelog tool]Disruptive User Impact
How to test this PR locally
In short, manually reproduce the tests described on #5716.
The long version:
run 3 proxies:
set up the proxies:
testing/integration/endpoint_security_test.go
to configure and run the needed proxies:[%s] certificates saved on: %s
. they will show where the certificates are.Related issues
--elastic-agent-cert-key-passphrase
with Elastc Defend installed #5491Questions to ask yourself