-
Notifications
You must be signed in to change notification settings - Fork 180
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
NE-1208: Gateway API E2E Testing #1023
base: master
Are you sure you want to change the base?
Conversation
@candita: This pull request references NE-1208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test verify |
/test e2e-aws-operator |
b8b5ff7
to
9deaecf
Compare
/test verify |
@candita: This pull request references NE-1208 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
/test e2e-aws-operator |
9deaecf
to
42f74d6
Compare
/retest |
42f74d6
to
73e6cb1
Compare
/retest-required |
/assign @rfredette |
Seems to be a network issue that made e2e-aws-operator test fail:
/test e2e-aws-operator |
CI may depend on #1037 |
/retest-required |
Maybe transient:
/test e2e-aws-operator |
This is failing again? See if it happens repeatedly.
/test e2e-aws-operator |
@candita: all tests passed! Full PR test history. Your PR dashboard. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a few comments that are mainly nit-picks, but overall, this look good to me.
testGatewayAPIResources(t) | ||
testGatewayAPIObjects(t) | ||
testGatewayAPIIstioInstallation(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testGatewayAPIResources(t) | |
testGatewayAPIObjects(t) | |
testGatewayAPIIstioInstallation(t) | |
t.Run("testGatewayAPIResources", testGatewayAPIResources) | |
t.Run("testGatewayAPIObjects", testGatewayAPIObjects) | |
t.Run("testGatewayAPIIstioInstallation", testGatewayAPIIstioInstallation) |
If testGatewayAPIResources, testGatewayAPIObjects, and testGatewayAPIIstioInstallation are intended to be treated as separate tests, using t.Run
to run them as subtests would help separate their output and success/failure conditions
t.Fatalf("failed to find expected CatalogSource %s", expectedCatalogSourceName) | ||
} | ||
if err := assertOSSMOperator(t); err != nil { | ||
t.Fatalf("failed to find expected Istio operator") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use t.Fatal()
when there are no printf-style arguments
t.Fatalf("failed to find expected Istio operator") | ||
} | ||
if err := assertIstiodProxy(t); err != nil { | ||
t.Fatalf("failed to find expected Istiod proxy") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above; use t.Fatal()
} | ||
// TODO - In OSSM 3.x the configuration object to check will be different. | ||
if err := assertSMCP(t); err != nil { | ||
t.Fatalf("failed to find expected SMCP: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatalf("failed to find expected SMCP: %s", err.Error()) | |
t.Fatalf("failed to find expected SMCP: %v", err) |
This is a nit, but it brings it in line with the way the majority of error messages are printed in these tests.
ok, gatewayClass := assertCanCreateGatewayClass(t, gatewayclass.OpenShiftDefaultGatewayClassName, gatewayclass.OpenShiftGatewayClassControllerName) | ||
if !ok { | ||
return fmt.Errorf("feature gate was enabled, but gateway class object could not be created") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, gatewayClass := assertCanCreateGatewayClass(t, gatewayclass.OpenShiftDefaultGatewayClassName, gatewayclass.OpenShiftGatewayClassControllerName) | |
if !ok { | |
return fmt.Errorf("feature gate was enabled, but gateway class object could not be created") | |
} | |
gatewayClass, err := createGatewayClass(t, gatewayclass.OpenShiftDefaultGatewayClassName, gatewayclass.OpenShiftGatewayClassControllerName) | |
if err != nil { | |
return fmt.Errorf("feature gate was enabled, but gateway class object could not be created: %v", err) | |
} |
I'm not sure the assertCanCreate...
functions are useful. Wouldn't this essentially be the same?
if err = kclient.Get(context.TODO(), name, gateway); err == nil { | ||
return gateway, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This returns gateway, nil
if the get is successful, but if it fails (which I'm sure is very unlikely), it still ends up returning gateway, nil
if err = kclient.Get(context.TODO(), name, gateway); err == nil { | |
return gateway, nil | |
} | |
if err = kclient.Get(context.TODO(), name, gateway); err == nil { | |
return gateway, nil | |
} else { | |
return nil, err | |
} |
} | ||
|
||
// getClusterVersion returns the ClusterVersion if found. If one is not found, it returns an error. | ||
func getClusterVersion() (*configv1.ClusterVersion, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be gateway API specific. I think it'd be great to put it in util_test.go
for better visibility.
} | ||
|
||
// assertSubscription checks if the Subscription of the given name exists and returns an error if not. | ||
func assertSubscription(t *testing.T, namespace, subName string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above; this could go in util_test.go
ok, testGateway = assertCanCreateGateway(t, gatewayClass, testGatewayName, openshiftIngressNamespace, domain) | ||
if !ok { | ||
return fmt.Errorf("feature gate was enabled, but gateway object could not be created") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, testGateway = assertCanCreateGateway(t, gatewayClass, testGatewayName, openshiftIngressNamespace, domain) | |
if !ok { | |
return fmt.Errorf("feature gate was enabled, but gateway object could not be created") | |
} | |
testGateway, err = createGateway(gatewayClass, testGatewayName, openshiftIngressNamespace, domain) | |
if err != nil { | |
return fmt.Errorf("feature gate was enabled, but gateway object could not be created: %w", err) | |
} |
Same as above
ok, _ = assertCanCreateHttpRoute(t, ns.Name, "test-httproute", openshiftIngressNamespace, default_routename, testGatewayName+"-"+gatewayclass.OpenShiftDefaultGatewayClassName, testGateway) | ||
if !ok { | ||
return fmt.Errorf("feature gate was enabled, but http route object could not be created") | ||
// The http route is automatically deleted because its namespace is automatically deleted. | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, _ = assertCanCreateHttpRoute(t, ns.Name, "test-httproute", openshiftIngressNamespace, default_routename, testGatewayName+"-"+gatewayclass.OpenShiftDefaultGatewayClassName, testGateway) | |
if !ok { | |
return fmt.Errorf("feature gate was enabled, but http route object could not be created") | |
// The http route is automatically deleted because its namespace is automatically deleted. | |
} | |
_, err = createHttpRoute(ns.Name, "test-httproute", openshiftIngressNamespace, default_routename, testGatewayName+"-"+gatewayclass.OpenShiftDefaultGatewayClassName, testGateway) | |
if err != nil { | |
return fmt.Errorf("feature gate was enabled, but http route object could not be created: %w", err) | |
// The http route is automatically deleted because its namespace is automatically deleted. | |
} |
Same as above
It seems like this test relies on OSSM 2.5 and the Gateway API v0.6.2 CRDs in #1018. When I ran it against a cluster on the latest nightly, the tests failed waiting for /hold until #1018 is merged. |
Add three new tests and a few helper functions for testing GatewayAPI.
Currently, the FeatureGate for GatewayAPI must be enabled first.
Working on a way to fake that for testing because it roll nodes and the router pods.