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(kuma-cp) virtual probes with query #2706

Merged
merged 5 commits into from
Sep 7, 2021

Conversation

lobkovilya
Copy link
Contributor

@lobkovilya lobkovilya commented Aug 30, 2021

Summary

When configuring readiness or liveness httpGet probes in Kubernetes path could consist of path + query, but when configuring path for Envoy route matching it's strictly only path (without query). That's why for probes with query matching didn't work properly.

Full changelog

  • Fix Envoy configuration
  • Add e2e test

Issues resolved

N/A

Documentation

N/A

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • Add backport-to-stable label if the code is backwards compatible. Otherwise, list breaking changes.

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
test/e2e/healthcheck/kubernetes/virtual_probes.go Outdated Show resolved Hide resolved
testserver.WithArgs("--probes"),
testserver.WithHTTPProbes(),
)(k8sCluster)).To(Succeed())
Eventually(testServerReady, "5s", "100ms").Should(BeTrue())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already checked in testserver.Install no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a check framework.WaitPodsAvailable(k.opts.Namespace, k.Name()) which is okay if Pod's containers are started but not ready yet, that's why right after testserver.Instsall the situation is the following:

NAME                           READY   STATUS    RESTARTS   AGE
test-server-7cbcf597ff-t5b2z   0/2     Running   0          6s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include this check in testserver.Install? This can be the problem in every usage of TestServer.

The same way VerifyKuma() should be a part of Kuma() but that's another story.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Do you think HTTP probes should be default for test-server deployment across all e2e tests?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #2706 (a60fa2f) into master (d1301a2) will decrease coverage by 0.05%.
The diff coverage is 3.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2706      +/-   ##
==========================================
- Coverage   51.94%   51.88%   -0.06%     
==========================================
  Files         871      872       +1     
  Lines       49518    49618     +100     
==========================================
+ Hits        25721    25746      +25     
- Misses      21716    21782      +66     
- Partials     2081     2090       +9     
Impacted Files Coverage Δ
test/e2e/healthcheck/kubernetes/virtual_probes.go 0.00% <0.00%> (ø)
...est/framework/deployments/testserver/kubernetes.go 0.00% <0.00%> (ø)
test/framework/setup.go 1.09% <0.00%> (-0.08%) ⬇️
test/server/cmd/echo.go 0.00% <0.00%> (ø)
pkg/xds/generator/probe_generator.go 84.61% <50.00%> (-9.33%) ⬇️
pkg/xds/generator/direct_access_proxy_generator.go 83.90% <0.00%> (+1.14%) ⬆️
pkg/insights/resyncer.go 67.66% <0.00%> (+1.50%) ⬆️
pkg/core/bootstrap/autoconfig.go 54.46% <0.00%> (+8.03%) ⬆️
.../core/managers/apis/ratelimit/ratelimit_manager.go 45.16% <0.00%> (+9.67%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1301a2...a60fa2f. Read the comment docs.

…dReadyE

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
@lobkovilya lobkovilya merged commit 0bacb2a into master Sep 7, 2021
@lobkovilya lobkovilya deleted the fix/virtual-probes-with-query branch September 7, 2021 08:02
mergify bot pushed a commit that referenced this pull request Sep 7, 2021
* fix(kuma-cp) virtual probes with query

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) make check

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) use NamespaceWithSidecarInjection

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) replace WaitUntilPodAvailableE with a custom WaitUntilPodReadyE

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) probes are default for test-server

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
(cherry picked from commit 0bacb2a)
lobkovilya pushed a commit that referenced this pull request Sep 8, 2021
Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
* fix(kuma-cp) virtual probes with query

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) make check

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) use NamespaceWithSidecarInjection

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) replace WaitUntilPodAvailableE with a custom WaitUntilPodReadyE

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) probes are default for test-server

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
nikita15p pushed a commit to nikita15p/kuma that referenced this pull request Sep 28, 2021
* fix(kuma-cp) virtual probes with query

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) make check

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) use NamespaceWithSidecarInjection

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) replace WaitUntilPodAvailableE with a custom WaitUntilPodReadyE

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>

* fix(kuma-cp) probes are default for test-server

Signed-off-by: Ilya Lobkov <ilya.lobkov@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants