Write PodProxyWithPath & ServiceProxyWithPath test - + 12 endpoint coverage #95503
Conversation
k8s-ci-robot
added
release-note-none
|
@Riaankl: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-priority
k8s-ci-robot
added
the
sig/network
/test pull-kubernetes-conformance-kind-ipv6-parallel |
|
/test pull-kubernetes-e2e-kind-ipv6 |
|
pull-kubernetes-e2e-kind-ipv6 #1315806857507377152 |
|
/test |
|
@Riaankl: The
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test all |
|
/assign @spiffxp @johnbelamaric |
|
/test all |
|
/test pull-kubernetes-e2e-kind-ipv6 |
|
/sig-network Can we get a lgtm for this PR - TIA |
|
@cmluciano you were working on this area recently, do you have some time for reviewing it? |
|
|
||
| // All methods for Pod ProxyWithPath return 200 | ||
| httpVerbs := []string{"DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT"} | ||
| for _, httpVerb := range httpVerbs { |
There was a problem hiding this comment.
This test just touches API surfaces and checks the status codes are 200.
Is it fine to do that without checking actual API behaviors?
I mean POST API could create something and the corresponding test needs to check something is created in general. And the same checks should be implemented for the other APIs also to check API behaviors.
There was a problem hiding this comment.
The goal of the test is to confirm that the proxy is passing valid requests through and returns a valid responses back for the list of endpoints below. When planning this test the feedback was to run porter from the standard agnhost test image.
endpoint | path | kind
------------------------------------------------+---------------------------------------------------------+-----------------
connectCoreV1PutNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
connectCoreV1PostNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
connectCoreV1PatchNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
connectCoreV1OptionsNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
connectCoreV1HeadNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
connectCoreV1DeleteNamespacedPodProxyWithPath | /api/v1/namespaces/{namespace}/pods/{name}/proxy/{path} | PodProxyOptions
endpoint | path | kind
----------------------------------------------------|-------------------------------------------------------------|---------------------
connectCoreV1PutNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
connectCoreV1PostNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
connectCoreV1PatchNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
connectCoreV1OptionsNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
connectCoreV1HeadNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
connectCoreV1DeleteNamespacedServiceProxyWithPath | /api/v1/namespaces/{namespace}/services/{name}/proxy/{path} | ServiceProxyOptions
There was a problem hiding this comment.
The goal of the test is to confirm that the proxy is passing valid requests through and returns a valid responses back for the list of endpoints below.
I am raising what is a valid response here.
Current test checks response code (HTTP200) only, doesn't the response body contain any values?
There was a problem hiding this comment.
Currently porter will respond with foo to any request in the response body. Updating the test to check for that as well.
There was a problem hiding this comment.
Is this verifying whether the correct verb is being received by porter? e.g. If I send a POST and it arrives at the container as a GET, that is incorrect.
There was a problem hiding this comment.
At the moment it isn't and I agree that shouldn't happen. Will consider how to update the test for this,
There was a problem hiding this comment.
I did check the above scenario when I was checking the base proxy endpoints for pods. I was just reflecting the response method back to the client.
import (
"fmt"
"github.com/gorilla/mux"
"log"
"net/http"
)
func responseFunction(w http.ResponseWriter, r *http.Request) {
fmt.Fprintf(w, "responseFunction: %v -> %v", r.Method, r.RequestURI)
fmt.Printf("responseFunction: \n%v -> %v\n", r.Method, r.RequestURI)
}
func handleRequests() {
r := mux.NewRouter()
r.HandleFunc("/", responseFunction).Methods("GET", "POST", "PUT", "DELETE", "PATCH")
log.Fatal(http.ListenAndServe(":80", r))
}
func main() {
handleRequests()
}
Should porter be updated to support this requirement? or should the test use another method?
There was a problem hiding this comment.
During the conformance subproject meeting: @johnbelamaric suggested that we update the correct command (or add a new one, or add a flag) to agnhost image to support the above.
There was a problem hiding this comment.
From conformance sub-project meeting today, @johnbelamaric suggested adding a flag option that would support this use case.
Current idea is that it would return a json payload that would include both the http method that porter received and the standard response for that port.
|
What is current status of this PR? |
|
@oomichi we are still waiting for https://prow.k8s.io/job-history/gs/kubernetes-jenkins/logs/post-kubernetes-push-e2e-test-images to run successfully. |
heyste
force-pushed
the
pod-service-proxy-with-path
branch
from
8f6ff2e to
6c1f3b7
Compare
k8s-ci-robot
removed
the
lgtm
|
/test pull-kubernetes-e2e-kind |
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
|
/test pull-kubernetes-e2e-ubuntu-gce-network-policies |
|
/test pull-kubernetes-e2e-ubuntu-gce-network-policies |
|
@Riaankl: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
this is a known issue , fix WIP in #94015 , is not a blocker for this PR |
I am OK for this PR and I already put my |
|
So glad to see the image promotion process finally provide the required updated image. Great work @heyste on both the image and the test! /lgtm |
k8s-ci-robot
added
the
lgtm
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR adds a test to test the following untested endpoints:
Pod:
connectCoreV1PutNamespacedPodProxyWithPath
connectCoreV1PostNamespacedPodProxyWithPath
connectCoreV1PatchNamespacedPodProxyWithPath
connectCoreV1OptionsNamespacedPodProxyWithPath
connectCoreV1HeadNamespacedPodProxyWithPath
connectCoreV1DeleteNamespacedPodProxyWithPath
Service:
connectCoreV1PutNamespacedServiceProxyWithPath
connectCoreV1PostNamespacedServiceProxyWithPath
connectCoreV1PatchNamespacedServiceProxyWithPath
connectCoreV1OptionsNamespacedServiceProxyWithPath
connectCoreV1HeadNamespacedServiceProxyWithPath
connectCoreV1DeleteNamespacedServiceProxyWithPath
Which issue(s) this PR fixes:
Fixes #95500
Testgrid Link:
Testgrid
Special notes for your reviewer:
Adds +12 endpoint test coverage (good for conformance)
Does this PR introduce a user-facing change?:
Release note:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/sig testing
/sig architecture
/area conformance