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

utils: improve envoyutils and curl requestutils #9335

Merged
merged 27 commits into from
Apr 9, 2024

Conversation

sam-heilbron
Copy link
Contributor

@sam-heilbron sam-heilbron commented Apr 7, 2024

Description

Improve some more utils that can be widely used in the codebase

Context

Background

I am in the process of building Kubernetes e2e tests for our integration with the Kubernetes Gateway API. To do that, I need to rely on some shared utilities. Some of the previous work in support of this is:

I also intend to add a way to export a report from an installation (#9327), but this PR is an attempt to introduce less code all at once.

curl requestutils

We previously had a CurlRequestBuilder, which I added as part of #9316. As I tried to use it more, I found two challenges with it:

  1. I wanted to use it in non-test code
  2. I wanted to easily define default curl arguments and modify them

To address both of these challenges I:

  1. Moved the utility into a utils/requestutils directory
  2. Moved to use functional arguments, so you can easily append function modifies

envoyutils

We interact with running instances of Envoy quite frequently:

  • During our e2e tests when we want to inspect state of the Envoy server
  • During our kube2e tests when we want to inspect the config_dump, logs or statistics after a test failure
  • Within our CLI to return data about the running Envoy

We end up repeating code to perform these requests. To improve this, I created a simple Admin Client. Under the hood, it relies on the Cmd utilities added as part of #9316

test flake reduction

In this PR I also attempt to fix 2 test flakes which have occurred recently. I intentionally mark the fix as resolvesIssue=false as I have not done the due diligence to prove that it will fix the flake entirely. However, in both situations I introduced some retry logic to help alleviate the flake frequency.

I propose that we merge the changes as they are, and then monitor the flakes, and if they do not occur for a week/two we can resolve the issue. In the meantime, I will just assign the issue tickets to myself so I can track the impact of this change:

Interesting decisions

Testing steps

  • I introduced unit tests for the utilities
  • I utilized the utilities in test code, which is passing CI
  • I ran the following steps to validate that basic e2e tests work with the envoy admin client changes:
  1. Focus the example_test
 VERSION=1.0.0-sam make gloo-envoy-wrapper-docker -B
ENVOY_IMAGE_TAG=1.0.0-sam TEST_PKG=test/e2e make test

Notes for reviewers

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Apr 7, 2024
@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/5723

@sam-heilbron sam-heilbron marked this pull request as ready for review April 7, 2024 03:44
Copy link

github-actions bot commented Apr 7, 2024

Visit the preview URL for this PR (updated for commit 2413c70):

https://gloo-edge--pr9335-sam-gg-envoy-admin-c-en7gahg2.web.app

(expires Mon, 15 Apr 2024 21:25:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/5723
#9291

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/5723
#9291
#9306

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/5723
#9291
#9306
#9292

@sam-heilbron sam-heilbron requested a review from nfuden April 7, 2024 15:06
pkg/utils/envoyutils/admincli/client.go Show resolved Hide resolved
pkg/utils/requestutils/curl/option.go Show resolved Hide resolved
pkg/utils/requestutils/curl/option.go Outdated Show resolved Hide resolved
pkg/utils/requestutils/curl/option.go Outdated Show resolved Hide resolved
pkg/utils/requestutils/curl/request.go Outdated Show resolved Hide resolved
pkg/utils/cmdutils/local.go Outdated Show resolved Hide resolved
pkg/utils/envoyutils/admincli/client.go Outdated Show resolved Hide resolved
pkg/utils/envoyutils/admincli/client_test.go Show resolved Hide resolved
pkg/utils/requestutils/curl/request.go Outdated Show resolved Hide resolved
test/services/envoy/instance.go Outdated Show resolved Hide resolved
jbohanon
jbohanon previously approved these changes Apr 8, 2024
nfuden
nfuden previously approved these changes Apr 8, 2024
@sam-heilbron sam-heilbron dismissed stale reviews from nfuden and jbohanon via 84e4bea April 8, 2024 20:16
@sam-heilbron
Copy link
Contributor Author

Copy link
Contributor

@jenshu jenshu left a comment

Choose a reason for hiding this comment

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

approving on behalf of nathan

@soloio-bulldozer soloio-bulldozer bot merged commit c8aa6c3 into main Apr 9, 2024
20 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the sam/gg-envoy-admin-cli branch April 9, 2024 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants