E2E cases for APF #94925

yue9944882 · 2020-09-21T07:44:43Z

this pull adds a E2E test that verifies the response headers should contain the information (for now which is object UID) corresponding flow-schema and priority-level upon enabling the APIPriorityAndFairness feature.

NONE

fedebongio · 2020-09-22T20:09:06Z

/cc @lavalamp

lavalamp · 2020-09-23T16:27:20Z

staging/src/k8s.io/apiserver/pkg/server/filters/priority-and-fairness.go

-	responseHeaderMatchedPriorityLevelConfigurationUID = "X-Kubernetes-PF-PriorityLevel-UID"
-	responseHeaderMatchedFlowSchemaUID                 = "X-Kubernetes-PF-FlowSchema-UID"
+	ResponseHeaderMatchedPriorityLevelConfigurationUID = "X-Kubernetes-PF-PriorityLevel-UID"
+	ResponseHeaderMatchedFlowSchemaUID                 = "X-Kubernetes-PF-FlowSchema-UID"


We should move these definitions to an api package, people should not be importing this package to get them.

lavalamp · 2020-09-23T16:29:28Z

test/e2e/apimachinery/flowcontrol.go

+		}
+		if plUID := resp.Header.Get(filters.ResponseHeaderMatchedPriorityLevelConfigurationUID); plUID != string(createdPriorityLevel.ObjectMeta.UID) {
+			framework.Failf("uid mismatch from the testing flow-schema")
+		}


Also check that a non-matching request doesn't get the same headers?

adtac · 2020-10-29T17:09:33Z

test/e2e/apimachinery/flowcontrol.go

+					Name: testingFlowSchemaName,
+				},
+				Spec: flowcontrolv1alpha1.FlowSchemaSpec{
+					MatchingPrecedence: 1000, // a rather higher precedence to ensure it make effect


is this so that default flow schemas don't interfere?

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

test/e2e/apimachinery/flowcontrol.go

adtac · 2020-10-30T19:55:01Z

/test pull-kubernetes-e2e-gce-alpha-features

test/e2e/apimachinery/flowcontrol.go

adtac · 2020-10-30T22:40:18Z

the test passes with the following diff applied:

diff --git a/test/e2e/apimachinery/flowcontrol.go b/test/e2e/apimachinery/flowcontrol.go
index 3c357cdb191..b017e84e5ca 100644
--- a/test/e2e/apimachinery/flowcontrol.go
+++ b/test/e2e/apimachinery/flowcontrol.go
@@ -97,23 +97,23 @@ var _ = SIGDescribe("[Feature:APIPriorityAndFairness][Alpha] response header sho
 
                ginkgo.By("response headers should contain flow-schema/priority-level uid")
 
-               if !testResponseHeaderMatches(f.ClientConfig(), matchingUsername, string(createdPriorityLevel.UID), string(createdFlowSchema.UID)) {
+               if !testResponseHeaderMatches(f, matchingUsername, string(createdPriorityLevel.UID), string(createdFlowSchema.UID)) {
                        framework.Failf("matching user doesnt received UID for the testing priority-level and flow-schema")
                }
-               if !testResponseHeaderMatches(f.ClientConfig(), nonMatchingUsername, string(createdPriorityLevel.UID), string(createdFlowSchema.UID)) {
+               if testResponseHeaderMatches(f, nonMatchingUsername, string(createdPriorityLevel.UID), string(createdFlowSchema.UID)) {
                        framework.Failf("non-matching user unexpectedly received UID for the testing priority-level and flow-schema")
                }
        })
 
 })
 
-func testResponseHeaderMatches(clientCfg *rest.Config, impersonatingUser, plUID, fsUID string) bool {
-       config := rest.CopyConfig(clientCfg)
+func testResponseHeaderMatches(f *framework.Framework, impersonatingUser, plUID, fsUID string) bool {
+       config := rest.CopyConfig(f.ClientConfig())
        config.Impersonate.UserName = impersonatingUser
        roundTripper, err := rest.TransportFor(config)
        framework.ExpectNoError(err)
 
-       req, err := http.NewRequest(http.MethodGet, "/version", nil)
+       req, err := http.NewRequest(http.MethodGet, f.ClientSet.CoreV1().RESTClient().Get().AbsPath("version").URL().String(), nil)
        framework.ExpectNoError(err)
 
        response, err := roundTripper.RoundTrip(req)

yue9944882 · 2020-11-02T02:50:30Z

@adtac thanks for the diff, applied to the latest commit

adtac · 2020-11-02T15:39:59Z

/test pull-kubernetes-e2e-gce-alpha-features

test/e2e/apimachinery/flowcontrol.go

ahg-g · 2020-11-02T18:05:37Z

/milestone v1.20

MikeSpreitzer · 2020-11-02T18:34:32Z

@MikeSpreitzer

ahg-g · 2020-11-02T19:17:54Z

/retest

lavalamp · 2020-11-02T19:27:28Z

/lgtm
/approve

k8s-ci-robot · 2020-11-02T19:28:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, yue9944882

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:

~~staging/src/k8s.io/api/OWNERS~~ [lavalamp]
~~staging/src/k8s.io/apiserver/OWNERS~~ [lavalamp]
~~test/e2e/apimachinery/OWNERS~~ [lavalamp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adtac · 2020-11-02T19:58:58Z

/test pull-kubernetes-e2e-gce-alpha-features

tkashem · 2020-11-02T20:25:02Z

test/e2e/apimachinery/flowcontrol.go

+		if !testResponseHeaderMatches(f, matchingUsername, string(createdPriorityLevel.UID), string(createdFlowSchema.UID)) {
+			framework.Failf("matching user doesnt received UID for the testing priority-level and flow-schema")
+		}
+		if testResponseHeaderMatches(f, nonMatchingUsername, string(createdPriorityLevel.UID), string(createdPriorityLevel.UID)) {
+			framework.Failf("non-matching user unexpectedly received UID for the testing priority-level and flow-schema")
+		}


Nit: if we decompose the testResponseHeaderMatches function, we can report on the specific header that does not match.

{ ginkgo.By("invoke API impersonating the matching user") response, err := invokeAPI(f, matchingUsername) if err != nil { framework.Failf("Failed to invoke API with matching user impersonation") } ginkgo.By("response headers should contain flow-schema/priority-level uid for matching user") framework.ExpectEqual(fsUID, getMatchedFlowSchemaUID(response), "FlowSchema UID does not match") framework.ExpectEqual(plUID, getMatchedPriorityLevelConfigurationUID(response), "PriorityLevelConfiguration UID does not match") ginkgo.By("invoke API impersonating the non-matching user") // non matching follows here. } func invokeAPI(f *framework.Framework, impersonatingUser string) (http.Response, error) { config := rest.CopyConfig(f.ClientConfig()) config.Impersonate.UserName = impersonatingUser roundTripper, err := rest.TransportFor(config) framework.ExpectNoError(err) req, err := http.NewRequest(http.MethodGet, f.ClientSet.CoreV1().RESTClient().Get().AbsPath("version").URL().String(), nil) framework.ExpectNoError(err) return roundTripper.RoundTrip(req) } func getMatchedFlowSchemaUID(response *http.Response) string { return response.Header.Get(flowcontrolv1alpha1.ResponseHeaderMatchedFlowSchemaUID) } func getMatchedPriorityLevelConfigurationUID(response *http.Response) string { return response.Header.Get(flowcontrolv1alpha1.ResponseHeaderMatchedPriorityLevelConfigurationUID) }

tkashem · 2020-11-02T20:45:47Z

@MikeSpreitzer @yue9944882 It's failing with the following error:

Nov  2 16:59:12.623: FAIL: non-matching user unexpectedly received UID for the testing priority-level and flow-schema

The non matching user will be matched via the global-default or catch-all, right?

adtac · 2020-11-02T22:10:49Z

Nov 2 16:59:12.623: FAIL: non-matching user unexpectedly received UID for the testing priority-level and flow-schema

@tkashem that error is from an older version of the commit.

k8s-ci-robot requested review from liggitt and sttts September 21, 2020 07:45

k8s-ci-robot added area/apiserver area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 21, 2020

yue9944882 force-pushed the apf-e2e branch 2 times, most recently from c411305 to c599dd9 Compare September 21, 2020 08:29

k8s-ci-robot requested a review from lavalamp September 22, 2020 20:09

lavalamp reviewed Sep 23, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2020

adtac reviewed Oct 29, 2020

View reviewed changes

adtac mentioned this pull request Oct 29, 2020

add APIPriorityAndFairness to presubmit and periodic alpha jobs kubernetes/test-infra#19757

Merged

adtac reviewed Oct 29, 2020

View reviewed changes

staging/src/k8s.io/apiserver/pkg/features/kube_features.go Outdated Show resolved Hide resolved

yue9944882 force-pushed the apf-e2e branch from c599dd9 to cb3c23a Compare October 30, 2020 02:22

yue9944882 force-pushed the apf-e2e branch 3 times, most recently from b4ade4f to 698c501 Compare October 30, 2020 03:22

liggitt removed their request for review October 30, 2020 17:00

adtac reviewed Oct 30, 2020

View reviewed changes

test/e2e/apimachinery/flowcontrol.go Outdated Show resolved Hide resolved

adtac reviewed Oct 30, 2020

View reviewed changes

test/e2e/apimachinery/flowcontrol.go Outdated Show resolved Hide resolved

adtac reviewed Oct 30, 2020

View reviewed changes

test/e2e/apimachinery/flowcontrol.go Show resolved Hide resolved

yue9944882 force-pushed the apf-e2e branch 2 times, most recently from 4d3ecd5 to c37de0b Compare November 2, 2020 02:47

yue9944882 force-pushed the apf-e2e branch from c37de0b to 7e4cadb Compare November 2, 2020 02:58

ahg-g reviewed Nov 2, 2020

View reviewed changes

test/e2e/apimachinery/flowcontrol.go Outdated Show resolved Hide resolved

adtac mentioned this pull request Nov 2, 2020

API priority and fairness: beta graduation checklist #96119

Closed

12 tasks

k8s-ci-robot added this to the v1.20 milestone Nov 2, 2020

e2e cases for apf

c28036e

yue9944882 force-pushed the apf-e2e branch from 7e4cadb to c28036e Compare November 2, 2020 18:28

k8s-ci-robot assigned lavalamp Nov 2, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2020

tkashem reviewed Nov 2, 2020

View reviewed changes

adtac mentioned this pull request Nov 2, 2020

e2e: add APF flowcontrol request drown-out test #96124

Merged

k8s-ci-robot merged commit e947440 into kubernetes:master Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E cases for APF #94925

E2E cases for APF #94925

yue9944882 commented Sep 21, 2020 •

edited

fedebongio commented Sep 22, 2020

lavalamp Sep 23, 2020

lavalamp Sep 23, 2020

yue9944882 Oct 30, 2020

adtac Oct 29, 2020

yue9944882 Oct 30, 2020

adtac commented Oct 30, 2020

adtac commented Oct 30, 2020

yue9944882 commented Nov 2, 2020

adtac commented Nov 2, 2020

ahg-g commented Nov 2, 2020

MikeSpreitzer commented Nov 2, 2020

ahg-g commented Nov 2, 2020

lavalamp commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

adtac commented Nov 2, 2020

tkashem Nov 2, 2020 •

edited

tkashem commented Nov 2, 2020

adtac commented Nov 2, 2020

E2E cases for APF #94925

E2E cases for APF #94925

Conversation

yue9944882 commented Sep 21, 2020 • edited

fedebongio commented Sep 22, 2020

lavalamp Sep 23, 2020

Choose a reason for hiding this comment

lavalamp Sep 23, 2020

Choose a reason for hiding this comment

yue9944882 Oct 30, 2020

Choose a reason for hiding this comment

adtac Oct 29, 2020

Choose a reason for hiding this comment

yue9944882 Oct 30, 2020

Choose a reason for hiding this comment

adtac commented Oct 30, 2020

adtac commented Oct 30, 2020

yue9944882 commented Nov 2, 2020

adtac commented Nov 2, 2020

ahg-g commented Nov 2, 2020

MikeSpreitzer commented Nov 2, 2020

ahg-g commented Nov 2, 2020

lavalamp commented Nov 2, 2020

k8s-ci-robot commented Nov 2, 2020

adtac commented Nov 2, 2020

tkashem Nov 2, 2020 • edited

Choose a reason for hiding this comment

tkashem commented Nov 2, 2020

adtac commented Nov 2, 2020

yue9944882 commented Sep 21, 2020 •

edited

tkashem Nov 2, 2020 •

edited