Bug 2039919: NE-542 Router compression E2E test#679
Bug 2039919: NE-542 Router compression E2E test#679openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
approved
candita
force-pushed
the
NE-542-RouterCompE2ETest
branch
from
24e5731 to
079f89c
Compare
|
evel=error msg=When applying changes to module.vpc.aws_vpc.new_vpc[0], provider /test e2e-aws-operator |
|
=== RUN TestUniqueIdHeader /test e2e-aws-operator |
|
Again: /test e2e-aws-operator |
|
/test e2e-aws-operator |
|
level=error msg=Error: Provider produced inconsistent result after apply /test e2e-aws-operator |
|
Still a terraform error. No answer to query on 4-dev-triage, just trying again in hopes something was fixed but not reported. |
|
Now a must-gather error.
/test e2e-aws-operator |
/test e2e-aws-operator |
test/e2e/router_compression_test.go
Outdated
| } | ||
|
|
||
| if header.Get("Content-Encoding") == "gzip" { | ||
| t.Logf("compression is enabled and shouldn't be for %s", r.Name) |
There was a problem hiding this comment.
Isn't this an error? Shouldn't this branch call t.Errorf or at least return an error?
| if err := wait.PollImmediate(2*time.Second, 5*time.Minute, func() (bool, error) { | ||
| header, code, err := getHttp(client, r, true) | ||
| if err != nil { | ||
| t.Logf("GET %s failed: %v, retrying...", r.Spec.Host, err) | ||
| return false, nil | ||
| } | ||
| if code != http.StatusOK { | ||
| t.Logf("GET %s failed: status %v, expected %v, retrying...", r.Spec.Host, code, http.StatusOK) | ||
| return false, nil // retry on 503 as pod/service may not be ready | ||
| } | ||
|
|
||
| if header.Get("Content-Encoding") != "gzip" { | ||
| t.Fatalf("compression is not enabled but should be for %s", r.Name) | ||
| } | ||
| t.Logf("compression is properly enabled for %s", r.Name) | ||
|
|
||
| return true, nil | ||
| }); err != nil { | ||
| t.Fatalf("failed to get results on GET: %v", err) | ||
| } |
There was a problem hiding this comment.
Consider refactoring this into a helper with, e.g., an "expectedContentEncoding" parameter.
test/e2e/router_compression_test.go
Outdated
| // Get HTTP Headers. If the request includes a Header of "Accept-Encoding: gzip" and the route | ||
| // mimeType matches the ingress setting, we should see "Content-Encoding: gzip" in the response. |
There was a problem hiding this comment.
The godoc suggests that getHttp looks at the response headers to check for Content-Encoding: gzip, but getHttp doesn't look at the response headers at all.
test/e2e/router_compression_test.go
Outdated
| return response.Header, response.StatusCode, nil | ||
| } | ||
|
|
||
| // Update the spec.httpCompressionPolicy with different MIME types |
There was a problem hiding this comment.
This godoc doesn't seem accurate. It isn't clear what testCompressionPolicy is intended to do, but it looks like it verifies that the ingress operator correctly configures OpenShift router with the given compression MIME types by (1) creating a new ingresscontroller with the MIME types and (2) verifying that the ingresscontroller becomes available, which means that HAProxy was able to parse the configuration. Is my understanding correct?
There was a problem hiding this comment.
Yes, will update godoc
test/e2e/router_compression_test.go
Outdated
| } | ||
|
|
||
| // Update the spec.httpCompressionPolicy with different MIME types | ||
| func testCompressionPolicy(t *testing.T, icName types.NamespacedName, domain string, compressionPolicy operatorv1.HTTPCompressionPolicy) error { |
There was a problem hiding this comment.
Call t.Helper() at the start of the function.
test/e2e/router_compression_test.go
Outdated
| p := ic.Spec.HTTPCompression | ||
| var found bool | ||
| for _, given := range compressionPolicy.MimeTypes { | ||
| found = false | ||
| for _, stored := range p.MimeTypes { | ||
| if given == stored { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| t.Fatalf("did not find mimeType: %s, found: %v\n", given, p.MimeTypes) | ||
| } | ||
| } | ||
| t.Logf("successfully created %s with MIME types: %v\n", ic.Name, ingresscontroller.GetMIMETypes(p.MimeTypes)) | ||
| return nil |
There was a problem hiding this comment.
What's the point of this logic? testCompressionPolicy assigns ic.Spec.HTTPCompression = compressionPolicy above, so it seems like this logic is just checking that the value that was assigned to ic.Spec.HTTPCompression matches the value that is in ic.Spec.HTTPCompression, which should be the same value anyway. If you want to verify that the MIME types are set on the router deployment, you could use waitForDeploymentEnvVar.
There was a problem hiding this comment.
Thanks. I'll use waitForDeploymentEnvVar, I hadn't considered that was possible.
test/e2e/router_compression_test.go
Outdated
| // Test some other compression policies for the ingress config | ||
| mimeTypesNormative := []operatorv1.CompressionMIMEType{"text/html", "application/json", "x-custom/allow-custom"} | ||
| compressionPolicyNormative := operatorv1.HTTPCompressionPolicy{MimeTypes: mimeTypesNormative} | ||
|
|
||
| mimeTypesEmpty := []operatorv1.CompressionMIMEType{} | ||
| compressionPolicyEmpty := operatorv1.HTTPCompressionPolicy{MimeTypes: mimeTypesEmpty} | ||
|
|
||
| mimeTypesNeedQuotes := []operatorv1.CompressionMIMEType{`text/html; v="keepquoted"`, `x-custom/allow-custom; specialChar='`} | ||
| compressionPolicyNeedQuotes := operatorv1.HTTPCompressionPolicy{MimeTypes: mimeTypesNeedQuotes} | ||
|
|
||
| mimeTypesErrors := []operatorv1.CompressionMIMEType{"text/", "x- /value", "//"} | ||
| compressionPolicyErrors := operatorv1.HTTPCompressionPolicy{MimeTypes: mimeTypesErrors} | ||
|
|
||
| icName := types.NamespacedName{Namespace: operatorNamespace, Name: "http-compression-1"} | ||
| domain := icName.Name + "." + dnsConfig.Spec.BaseDomain | ||
| if err := testCompressionPolicy(t, icName, domain, compressionPolicyNormative); err != nil { | ||
| log.Fatalf("error testing normal compression policy: %v", err) | ||
| } | ||
|
|
||
| icName = types.NamespacedName{Namespace: operatorNamespace, Name: "http-compression-2"} | ||
| domain = icName.Name + "." + dnsConfig.Spec.BaseDomain | ||
| if err := testCompressionPolicy(t, icName, domain, compressionPolicyEmpty); err != nil { | ||
| log.Fatalf("error testing empty compression policy: %v", err) | ||
| } | ||
|
|
||
| icName = types.NamespacedName{Namespace: operatorNamespace, Name: "http-compression-3"} | ||
| domain = icName.Name + "." + dnsConfig.Spec.BaseDomain | ||
| if err := testCompressionPolicy(t, icName, domain, compressionPolicyNeedQuotes); err != nil { | ||
| log.Fatalf("error testing compression policy that needs quotes: %v", err) | ||
| } | ||
|
|
||
| icName = types.NamespacedName{Namespace: operatorNamespace, Name: "http-compression-4"} | ||
| domain = icName.Name + "." + dnsConfig.Spec.BaseDomain | ||
| if err := testCompressionPolicy(t, icName, domain, compressionPolicyErrors); err == nil { | ||
| log.Fatalf("compression policy with errors should have failed but didn't") | ||
| } |
There was a problem hiding this comment.
It seems like this block is one test, and the lines above this block and the lines below this block are another test. Could this block be moved to the end of the test function or to a separate test function?
test/e2e/router_compression_test.go
Outdated
| icName := types.NamespacedName{Namespace: operatorNamespace, Name: "http-compression-1"} | ||
| domain := icName.Name + "." + dnsConfig.Spec.BaseDomain | ||
| if err := testCompressionPolicy(t, icName, domain, compressionPolicyNormative); err != nil { | ||
| log.Fatalf("error testing normal compression policy: %v", err) |
There was a problem hiding this comment.
Tests usually don't use log. Why not use t here and below?
| log.Fatalf("error testing normal compression policy: %v", err) | |
| t.Errorf("error testing normal compression policy: %v", err) |
candita
force-pushed
the
NE-542-RouterCompE2ETest
branch
from
079f89c to
b56b794
Compare
|
@Miciah I made all changes you requested. Please take a final look. |
|
/retest-required |
candita
force-pushed
the
NE-542-RouterCompE2ETest
branch
from
b56b794 to
5edc437
Compare
|
/retest |
|
/retest-required |
test/e2e/router_compression_test.go
Outdated
| ) | ||
|
|
||
| func TestRouterCompressionParsing(t *testing.T) { | ||
| t.Helper() |
There was a problem hiding this comment.
t.Helper() should be called in the helper functions, not in the test function itself.
test/e2e/router_compression_test.go
Outdated
| pc4, err := getPrivateController(t, "http-compression-4", dnsConfig.Spec.BaseDomain) | ||
| if err != nil { | ||
| t.Fatalf("error getting private controller: %v", err) | ||
| } | ||
| if err := testCompressionPolicy(t, "http-compression-4", compressionPolicyErrors); err == nil { | ||
| t.Errorf("compression policy with errors should have failed but didn't") | ||
| } | ||
| defer assertIngressControllerDeleted(t, kclient, pc4) |
There was a problem hiding this comment.
| pc4, err := getPrivateController(t, "http-compression-4", dnsConfig.Spec.BaseDomain) | |
| if err != nil { | |
| t.Fatalf("error getting private controller: %v", err) | |
| } | |
| if err := testCompressionPolicy(t, "http-compression-4", compressionPolicyErrors); err == nil { | |
| t.Errorf("compression policy with errors should have failed but didn't") | |
| } | |
| defer assertIngressControllerDeleted(t, kclient, pc4) | |
| pc4, err := getPrivateController(t, "http-compression-4", dnsConfig.Spec.BaseDomain) | |
| if err != nil { | |
| t.Fatalf("error getting private controller: %v", err) | |
| } | |
| defer assertIngressControllerDeleted(t, kclient, pc4) | |
| if err := testCompressionPolicy(t, "http-compression-4", compressionPolicyErrors); err == nil { | |
| t.Errorf("compression policy with errors should have failed but didn't") | |
| } |
test/e2e/router_compression_test.go
Outdated
| defer assertIngressControllerDeleted(t, kclient, pc) | ||
| } | ||
|
|
||
| func getPrivateController(t *testing.T, privateName string, privateDomain string) (*operatorv1.IngressController, error) { |
There was a problem hiding this comment.
Consider renaming this function as createPrivateController to convey that it doesn't just get an existing ingresscontroller but rather creates a new one.
test/e2e/router_compression_test.go
Outdated
| ic := newPrivateController(icName, domain) | ||
| // Create a new private Ingress Controller (deletion handled by caller) | ||
| if err := kclient.Create(context.TODO(), ic); err != nil { | ||
| return ic, errors.New(fmt.Sprintf("error creating private ingresscontroller %s: %v", privateName, err)) |
There was a problem hiding this comment.
| return ic, errors.New(fmt.Sprintf("error creating private ingresscontroller %s: %v", privateName, err)) | |
| return ic, fmt.Errorf("error creating private ingresscontroller %s: %v", privateName, err) |
test/e2e/router_compression_test.go
Outdated
| t.Logf("failed to get ingress controller: %v, retrying...", err) | ||
| return false, nil | ||
| } | ||
| ic.Spec.HTTPCompression = compressionPolicy |
There was a problem hiding this comment.
| ic.Spec.HTTPCompression = compressionPolicy | |
| compressionPolicy.DeepCopyInto(&ic.Spec.HTTPCompression) |
See #689 for why this sort of assignment is problematic.
test/e2e/router_compression_test.go
Outdated
|
|
||
| // curl to canary, without the Accept-Encoding header set to gzip | ||
| if err := testContentEncoding(t, client, r, false, ""); err != nil { | ||
| t.Errorf(err.Error()) |
There was a problem hiding this comment.
| t.Errorf(err.Error()) | |
| t.Error(err) |
test/e2e/router_compression_test.go
Outdated
|
|
||
| // curl to canary, WITH the Accept-Encoding header set to gzip | ||
| if err := testContentEncoding(t, client, r, true, "gzip"); err != nil { | ||
| t.Errorf(err.Error()) |
There was a problem hiding this comment.
| t.Errorf(err.Error()) | |
| t.Error(err) |
test/e2e/router_compression_test.go
Outdated
| } | ||
|
|
||
| func TestRouterCompressionOperation(t *testing.T) { | ||
| t.Helper() |
There was a problem hiding this comment.
Don't call t.Helper() in the test function.
test/e2e/router_compression_test.go
Outdated
|
|
||
| contentEncoding := header.Get("Content-Encoding") | ||
| if contentEncoding != expectedContentEncoding { | ||
| return false, errors.New(fmt.Sprintf("compression error: expected \"%s\", got \"%s\" for %s route", expectedContentEncoding, contentEncoding, route.Name)) |
There was a problem hiding this comment.
| return false, errors.New(fmt.Sprintf("compression error: expected \"%s\", got \"%s\" for %s route", expectedContentEncoding, contentEncoding, route.Name)) | |
| return false, fmt.Errorf("compression error: expected %q, got %q for %s route", expectedContentEncoding, contentEncoding, route.Name) |
test/e2e/router_compression_test.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // getHTTPHeaders returns the HTTP Headers, and first adds the request header "Accept-Encoding: gzip" if requested. |
There was a problem hiding this comment.
| // getHTTPHeaders returns the HTTP Headers, and first adds the request header "Accept-Encoding: gzip" if requested. | |
| // getHttpHeaders returns the HTTP Headers, and first adds the request header "Accept-Encoding: gzip" if requested. |
candita
force-pushed
the
NE-542-RouterCompE2ETest
branch
from
5edc437 to
2f3e759
Compare
|
The single-node e2e job is failing during installation about 50% of the runs: /retest |
candita
changed the title
openshift-ci
bot
added
the
bugzilla/severity-medium
|
@candita: This pull request references Bugzilla bug 2039919, which is invalid:
Comment DetailsIn 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. |
openshift-ci
bot
added
the
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci
bot
added
the
bugzilla/valid-bug
|
@candita: This pull request references Bugzilla bug 2039919, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
openshift-ci
bot
removed
the
bugzilla/invalid-bug
|
The single-node e2e job is failing during installation about 50% of the runs: /retest |
test/e2e/router_compression_test.go
Outdated
| if err := wait.PollImmediate(10*time.Second, 1*time.Minute, func() (bool, error) { | ||
| ic, err := getIngressController(t, kclient, defaultName, 1*time.Minute) | ||
| if err != nil { | ||
| t.Fatalf("failed to get ingress controller: %v, retrying...", err) |
There was a problem hiding this comment.
t.Fatalf will cause the test to fail and return immediately.
| t.Fatalf("failed to get ingress controller: %v, retrying...", err) | |
| t.Logf("failed to get ingress controller: %v, retrying...", err) |
test/e2e/router_compression_test.go
Outdated
| if err := testCompressionPolicy(t, name, policy); err != nil { | ||
| t.Errorf(errorMsg, err) | ||
| } | ||
| defer assertIngressControllerDeleted(t, kclient, pc) |
There was a problem hiding this comment.
Tiny suggestion: the deferred cleanup should be grouped with the creation.
| if err := testCompressionPolicy(t, name, policy); err != nil { | |
| t.Errorf(errorMsg, err) | |
| } | |
| defer assertIngressControllerDeleted(t, kclient, pc) | |
| defer assertIngressControllerDeleted(t, kclient, pc) | |
| if err := testCompressionPolicy(t, name, policy); err != nil { | |
| t.Errorf(errorMsg, err) | |
| } |
test/e2e/router_compression_test.go
Outdated
| if strings.Contains(err.Error(), "has been modified") { | ||
| t.Logf("failed to update ingress controller, retrying...") | ||
| return false, nil | ||
| } | ||
| // Return an error if validation failed | ||
| return true, err |
There was a problem hiding this comment.
The "k8s.io/apimachinery/pkg/api/errors" package's IsInvalid function should work, but we can leave this as-is if you prefer. (Actually, maybe this logic should retry if errors.IsConflict(err) is true, and the caller could use errors.IsInvalid(err) on the returned error value if the caller expects an invalid error. Note that you would need to use %w in fmt.Errorf to wrap the error from kclient.Update so that errors.IsConflict could properly introspect it, I think. Anyway, this isn't critical for this PR, so we can leave it as-is.)
candita
force-pushed
the
NE-542-RouterCompE2ETest
branch
from
2f3e759 to
211b9c1
Compare
|
|
@candita: The
The following commands are available to trigger optional jobs:
Use
DetailsIn 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 e2e-upgrade |
|
@candita: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@candita: All pull requests linked via external trackers have merged: Bugzilla bug 2039919 has been moved to the MODIFIED state. DetailsIn 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. |
3 participants
Test the canary route to see if it will return a "Content-Encoding: gzip" header when the "Accept-Encoding: gzip" header is sent, and the new HTTPCompressionPolicy.MimeTypes includes the mime type of the canary route (which is "text/plain; charset=utf-8").
Also clean up a comment in util.go and export the getMIMETypes method in deployment.go for use in tests.