-
Notifications
You must be signed in to change notification settings - Fork 707
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
Fix EnterpriseSearch upgrade with TLS disabled #6224
Fix EnterpriseSearch upgrade with TLS disabled #6224
Conversation
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 the only thing I am not happy with is that we never test this scenario. But I admit that setting up a service mesh based test would be separate task. So let's fix the bug first.
I'm not satisfied either. We could update diff --git a/test/e2e/ent/ent_test.go b/test/e2e/ent/ent_test.go
index f70144451..1feccc4db 100644
--- a/test/e2e/ent/ent_test.go
+++ b/test/e2e/ent/ent_test.go
@@ -89,11 +89,19 @@ func TestEnterpriseSearchVersionUpgradeToLatest8x(t *testing.T) {
WithVersion(srcVersion).
WithRestrictedSecurityContext()
+ entNoTLS := enterprisesearch.NewBuilder(name).
+ WithElasticsearchRef(es.Ref()).
+ WithNodeCount(1).
+ WithVersion(srcVersion).
+ WithTLSDisabled(true).
+ WithRestrictedSecurityContext()
+
esUpgraded := es.WithVersion(dstVersion).WithMutatedFrom(&es)
entUpgraded := ent.WithVersion(dstVersion).WithMutatedFrom(&ent)
+ entNoTLSUpgraded := entNoTLS.WithVersion(dstVersion).WithMutatedFrom(&entNoTLS)
// During the version upgrade, the operator will toggle Enterprise Search read-only mode.
// We don't verify this behaviour here. Instead, we just check Enterprise Search eventually
// runs fine in the new version: it would fail to run if read-only mode wasn't toggled.
- test.RunMutations(t, []test.Builder{es, ent}, []test.Builder{esUpgraded, entUpgraded})
+ test.RunMutations(t, []test.Builder{es, ent, entNoTLS}, []test.Builder{esUpgraded, entUpgraded, entNoTLSUpgraded}) |
My proposal doesn't work. It seems that one Elasticsearch shared between two different Enterprise Search makes problem. The test runner received 401 error when reaching the API of the second enterpriseSearch using the API key auth method. |
I opted to turn |
run/e2e-tests match=TestEnterpriseSearchTLSDisabledVersionUpgradeToLatest8x |
Correctly create the HTTP client to call the EnterpriseSearch API when TLS is disabled at the EnterpriseSearch resource level.
Resolves #6185.
Since the HTTP client is mocked in the unit test, I didn't update it. If we want to cover this with a test, one option would be to extract the client creation in another method and test that one. It didn't seem worth it to me.
I made a pass on the places where
apmhttp.WrapClient
andcommonhttp.Client
are called and I did not find any other occurrence of this bug.