kubernetes-sigs / apiserver-network-proxy

Apache License 2.0
381 stars 182 forks source link

Bump go 1.22.2->1.22.5 and add E2E tests #639

Closed carreter closed 3 months ago

carreter commented 4 months ago

Made some E2E tests that:

Currently do this for two scenarios: 1 agent + 1 server and 3 agents + 3 servers.

Had to run go mod tidy and go mod vendor along the way to pull in some dependencies, couldn't figure out how to do this without bumping the go patch version. Hope this is ok, I can try and rework it if not!

k8s-ci-robot commented 4 months ago

Hi @carreter. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
carreter commented 4 months ago

@avrittrohwer is on vacation this week, is anyone else available to review?

carreter commented 4 months ago

Blocks #635 FYI

cheftako commented 4 months ago

/ok-to-test

carreter commented 4 months ago

@tallclair for visibility. Working on cleaning up the PR to see if we can avoid upgrading the go version and minimize the vendored dependencies!

carreter commented 4 months ago

Seems like sigs.k8s.io/e2e-fromework v0.4.0 requires go 1.22.3 so it is impossible to downgrade:

$ go mod tidy -go 1.22.2
go: sigs.k8s.io/e2e-framework@v0.4.0 requires go@1.22.3, but 1.22.2 is requested

Here are the new packages that are getting pulled into vendor/:

$ git diff master -- vendor/modules.txt | grep -e "+# " | sed -e "s/\+# //" -e "/.*=>.*/d"
github.com/evanphx/json-patch/v5 v5.9.0
github.com/gorilla/websocket v1.5.0
github.com/imdario/mergo v0.3.15
github.com/moby/spdystream v0.2.0
github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f
github.com/vladimirvivien/gexe v0.2.0
sigs.k8s.io/controller-runtime v0.18.2
sigs.k8s.io/e2e-framework v0.4.0
sigs.k8s.io/yaml v1.4.0

@cheftako Where can I check if these are ok to include?

carreter commented 4 months ago

@avrittrohwer Addressed your comments. Working on including this in the GitHub workflows now.

avrittrohwer commented 4 months ago

/retest

carreter commented 4 months ago

Excluded the e2e tests from the main test runners (fast-test and test) in the Makefile. Hopefully that fixes this.

carreter commented 4 months ago

/retest

avrittrohwer commented 4 months ago

Hmm, should we expect to see the kind-e2e job in the checks? I don't see it

carreter commented 4 months ago

Hmm, should we expect to see the kind-e2e job in the checks? I don't see it

I always forget how this works. Might not show up until it gets merged?

avrittrohwer commented 4 months ago

Hmm, should we expect to see the kind-e2e job in the checks? I don't see it

I always forget how this works. Might not show up until it gets merged?

Could you merge this into your fork's main branch then open up a PR again your fork so the new action runs? I just want to have some kind of actual run of the new e2e test so we can be more certain it works

carreter commented 4 months ago

Seems like kind-e2e is included as part of the e2e workflow but needs maintainer approval before running:

image

There's some minor bugs in the workflow I'm ironing out, will ping when they're sorted!

carreter commented 4 months ago

Workflow is working now! I will make it so it runs against the same k8s version matrix as the existing e2e tests.

carreter commented 4 months ago

@avrittrohwer Github action for e2e tests is now working against multiple k8s versions. Unfortunately I can't seem to get make docker-build to load the image built by the build step, and it rebuilds it for each test run. Any clue how to fix this?

avrittrohwer commented 4 months ago

@cheftako @jkh52 could either of you approve the new e2e action check so we can validate this PR?

carreter commented 4 months ago

@avrittrohwer FYI, it's working on my fork of the repo: https://github.com/carreter/apiserver-network-proxy/actions/runs/10012999446

avrittrohwer commented 4 months ago

@avrittrohwer Github action for e2e tests is now working against multiple k8s versions. Unfortunately I can't seem to get make docker-build to load the image built by the build step, and it rebuilds it for each test run. Any clue how to fix this?

Is this because the test-e2e make target is reliant on docker-build target: test-e2e: docker-build?

carreter commented 3 months ago

Latest commit significantly speeds up the kind-e2e runner by not rebuilding the docker images on each iteration of the matrix.

carreter commented 3 months ago

@avrittrohwer @cheftako @ipochi @tallclair how are we feeling about this PR? If there are no major changes to be made, I'll start working on e2e tests for #635. Just want to be sure the e2e framework won't significantly change from under me before I do so!

ipochi commented 3 months ago

@avrittrohwer @cheftako @ipochi @tallclair how are we feeling about this PR? If there are no major changes to be made, I'll start working on e2e tests for #635. Just want to be sure the e2e framework won't significantly change from under me before I do so!

@carreter I'm good.

avrittrohwer commented 3 months ago

https://github.com/kubernetes/test-infra/pull/33097 is merged

avrittrohwer commented 3 months ago

/retest

carreter commented 3 months ago

As per @cheftako and @ipochi 's suggestion, I have changed the StatefulSet out for a Deployment and address the pods directly by their IP.

carreter commented 3 months ago

Hmm, unsure why the tests are failing suddenly. They were working locally. Will do some digging.

carreter commented 3 months ago

/retest

cheftako commented 3 months ago

/lgtm /approve

k8s-ci-robot commented 3 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avrittrohwer, carreter, cheftako, ipochi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/apiserver-network-proxy/blob/master/OWNERS)~~ [cheftako] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment