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

Fix EnterpriseSearch upgrade with TLS disabled #6224

Merged
merged 2 commits into from
Dec 20, 2022

Conversation

thbkrkr
Copy link
Contributor

@thbkrkr thbkrkr commented Dec 12, 2022

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.WrapClientand commonhttp.Client are called and I did not find any other occurrence of this bug.

@thbkrkr thbkrkr added >bug Something isn't working v2.7.0 labels Dec 12, 2022
Copy link
Collaborator

@pebrc pebrc left a 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.

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Dec 15, 2022

I'm not satisfied either. We could update TestEnterpriseSearchVersionUpgradeToLatest8x to also test the upgrade of ent without TLS. This way, we don't add a new e2e test just for this and we don't make the existing longer either.

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})

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Dec 19, 2022

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.

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Dec 19, 2022

I opted to turn TestEnterpriseSearchTLSDisabled into TestEnterpriseSearchTLSDisabledVersionUpgradeToLatest8x.

@thbkrkr
Copy link
Contributor Author

thbkrkr commented Dec 19, 2022

run/e2e-tests match=TestEnterpriseSearchTLSDisabledVersionUpgradeToLatest8x

@thbkrkr thbkrkr merged commit 984e278 into elastic:main Dec 20, 2022
@thbkrkr thbkrkr deleted the fix-http-client-tls-for-ent-upgrade branch December 20, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v2.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconciliation error upgrading enterprise search with TLS disabled
2 participants