OCPBUGS-16089: Set spec.subdomain on the canary route #965

Miciah · 2023-07-20T21:06:58Z

`Test_desiredCanaryRoute`: Fix owner reference check

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

`Test_desiredCanaryRoute`: Use testify

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set `spec.subdomain` on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):
pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.
pkg/operator/controller/canary/route.go (canaryRouteChanged): Check whether spec.host or spec.subdomain have changed, and update them if they have.
(desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.
pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_canaryRouteChanged): Verify that canaryRouteChanged checks and updates the spec.host and spec.subdomain fields.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.
test/e2e/canary_test.go (TestCanaryRoute):
test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):
test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):
test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.
test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

~~/hold~~
I'll need to rebase after #880 merges.

openshift-ci-robot · 2023-07-20T21:07:03Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is invalid:

expected the bug to target the "4.14.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

TestDesiredCanaryRoute: Fix owner reference check

Change TestDesiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, TestDesiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Fix checks for owner references.

TestDesiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/route.go (desiredCanaryService): Specify spec.subdomain.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.

/hold
I'll need to rebase after #880 merges.

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.

Miciah · 2023-07-20T21:07:49Z

/jira refresh

openshift-ci-robot · 2023-07-20T21:08:00Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

/jira refresh

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.

Miciah · 2023-07-20T22:17:34Z

e2e-aws-ovn-serial failed because AWS is full:

 level=error msg=Error: creating EC2 Instance: InsufficientInstanceCapacity: We currently do not have sufficient m6a.xlarge capacity in the Availability Zone you requested (us-east-1d). Our system will be working on provisioning additional capacity. You can currently get m6a.xlarge capacity by not specifying an Availability Zone in your request or choosing us-east-1a, us-east-1b, us-east-1c, us-east-1f.
level=error msg=	status code: 500, request id: 91fc067a-7acd-4a15-a8f3-00cef8eda440

/test e2e-aws-ovn-serial

e2e-azure-operator failed because the "eastus" region went missing:

 level=error msg=failed to fetch Master Machines: failed to load asset "Install Config": failed to create install config: platform.azure.region: Invalid value: "eastus": region "eastus" is not valid or not available for this account

From Slack discussion, it seems like this sort of thing might be a "hiccup".
/test e2e-azure-operator

Miciah · 2023-07-20T23:14:58Z

https://github.com/openshift/cluster-ingress-operator/compare/3f326ea54a6fba8cd5dc4e34925c9d573d23638b..7315ba0b63dfd9150e6294d681f5f936a5f02d5a fixes some places in the code that used spec.host instead of checking the canary route's status.

openshift-ci-robot · 2023-07-20T23:16:07Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

TestDesiredCanaryRoute: Fix owner reference check

Change TestDesiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, TestDesiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Fix checks for owner references.

TestDesiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

/hold
I'll need to rebase after #880 merges.

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-robot · 2023-07-21T04:49:53Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

TestDesiredCanaryRoute: Fix owner reference check

Change TestDesiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, TestDesiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Fix checks for owner references.

TestDesiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (TestDesiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

test/e2e/canary_test.go (TestCanaryRoute):

test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):

test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):

test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.

test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

/hold
I'll need to rebase after #880 merges.

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.

Miciah · 2023-07-21T04:50:04Z

https://github.com/openshift/cluster-ingress-operator/compare/7315ba0b63dfd9150e6294d681f5f936a5f02d5a..8ac3ec1e5504c69ed960973a8cd18ee7b53855cc fixes some E2E tests that used spec.host instead of the route status.

Miciah · 2023-07-21T18:55:28Z

https://github.com/openshift/cluster-ingress-operator/compare/8ac3ec1e5504c69ed960973a8cd18ee7b53855cc..05464949e91c275a80f65c107d9649da0ad6c5f5 revises the previous fix for E2E tests to take into account that some E2E tests do not use the default IngressController.

Miciah · 2023-07-21T21:43:27Z

e2e-azure-operator failed because TestHAProxyTimeouts failed:

TestAll/parallel/TestHAProxyTimeouts 
=== RUN   TestAll/parallel/TestHAProxyTimeouts
=== PAUSE TestAll/parallel/TestHAProxyTimeouts
=== CONT  TestAll/parallel/TestHAProxyTimeouts
=== NAME  TestAll/parallel/TestHAProxyTimeouts
    haproxy_timeouts_test.go:169: Error executing grep -oP tcp-request\s+inspect-delay\s+\K2147483647ms /var/lib/haproxy/conf/haproxy.config: etcdserver: leader changed
    haproxy_timeouts_test.go:170: stderr: 
=== NAME  TestAll/parallel/TestHAProxyTimeouts
    haproxy_timeouts_test.go:181: deleted ingresscontroller haproxy-timeout

TestHAProxyTimeouts only checks global timeout settings in haproxy.config; the test doesn't use or check routes at all. This fact and the "etcdserver" error lead me to believe that this is a novel flake caused by cluster instability unrelated to ingress.
/test e2e-azure-operator

e2e-gcp-operator failed because TestUnmanagedDNSToManagedDNSInternalIngressController failed. This is probably the same issue as OCPBUGS-13106.
/test e2e-gcp-operator

e2e-aws-operator failed because must-gather failed.
/test e2e-aws-operator

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value. Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value. * pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

* pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

openshift-ci-robot · 2023-07-24T13:29:19Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

Test_desiredCanaryRoute: Fix owner reference check

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

Test_desiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

test/e2e/canary_test.go (TestCanaryRoute):

test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):

test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):

test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.

test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

~~/hold~~
I'll need to rebase after #880 merges.

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.

Miciah · 2023-07-24T13:29:34Z

https://github.com/openshift/cluster-ingress-operator/compare/05464949e91c275a80f65c107d9649da0ad6c5f5..649435d3a9157535243c51d5bbe4d6a6a15040bc rebases for #880.

Miciah · 2023-07-24T13:29:41Z

/hold cancel

candita · 2023-07-26T15:46:41Z

/assign @gcs278

gcs278

Mainly just question, but generally looks good to me.

gcs278 · 2023-07-28T21:01:42Z

pkg/operator/controller/canary/route.go

Should https://github.com/openshift/cluster-ingress-operator/pull/965/files#diff-aabcb3468eb7e3e07cce919ec78adc2f6012ed230991bc7f40f62868ada392d7R100 canaryRouteChanged now reconcile spec.subdomain?

I notice I can change the subdomain field and it won't get reconciled back.

I'm ambivalent about that since (a) we don't check spec.host, (b) leaving spec.host and spec.subdomain on their existing values on upgrade seems lowest risk, and (c) it isn't necessary to change canaryRouteChanged to fix the reported bug (which concerns the behavior when someone deletes the route). However, reconciling spec.host and spec.subdomain would be more consistent with the guiding principals for the platform. I can go ahead and make the change.

Done in https://github.com/openshift/cluster-ingress-operator/compare/649435d3a9157535243c51d5bbe4d6a6a15040bc..530d326013c9169d1efee77387f33c9ec13108d4.

gcs278 · 2023-07-28T21:07:58Z

test/e2e/router_compression_test.go

 		contentEncoding := header.Get("Content-Encoding")
 		if contentEncoding != expectedContentEncoding {
-			return false, fmt.Errorf("compression error: expected %q, got %q for %s route", expectedContentEncoding, contentEncoding, route.Name)
+			return false, fmt.Errorf("compression error: expected %q, got %q for %s route", expectedContentEncoding, contentEncoding, routeHost)


I think this is fine, but curious if you meant to change route.Name to the hostname of the route?

It was an intentional change; I needed routeHost elsewhere in the function, and it seemed simpler to use it in this log message rather than pass in both routeHost and route.

Ack, sounds good to me.

gcs278 · 2023-07-28T21:12:00Z

test/e2e/util_test.go

+// function does not check the "Admitted" status condition, so it will return
+// the host name if it is set even if the IngressController has rejected the
+// route.
+func getRouteHost(t *testing.T, route *routev1.Route, router string) string {


Would switching canary.getRouteHost to public be a better option than duplicating?

Or is this intentional because it's just E2E test code?

The function is simple and yet specialized enough that it seemed simpler to me to duplicate a few lines rather than convert the other getRouteHost into a public interface.

gcs278 · 2023-07-28T21:15:48Z

pkg/operator/controller/canary/controller.go

Any concerns with downgrading? I think route.spec.subdomain was introduced in 4.11, but I'm not sure.

Right, spec.subdomain was implemented in 4.11. On downgrade from 4.14 to 4.13, the route could have spec.subdomain set and spec.host empty, and this would be fine because 4.13 doesn't reconcile either of those fields and both are implemented in 4.13.

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route. Before this commit, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail. For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This commit ensures that the canary route continues to work in this example. This commit fixes OCPBUGS-16089. https://issues.redhat.com/browse/OCPBUGS-16089 * pkg/operator/controller/canary/controller.go (startCanaryRoutePolling): * pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function. * pkg/operator/controller/canary/route.go (canaryRouteChanged): Check whether spec.host or spec.subdomain have changed, and update them if they have. (desiredCanaryService): Specify spec.subdomain. (getRouteHost): New function. Return the host name of the route for the default IngressController. * pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain. (Test_canaryRouteChanged): Verify that canaryRouteChanged checks and updates the spec.host and spec.subdomain fields. (Test_getRouteHost): New test. Verify that getRouteHost behaves correctly. * test/e2e/canary_test.go (TestCanaryRoute): * test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs): * test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture): * test/e2e/router_compression_test.go (TestRouterCompressionOperation) (testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function. * test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the named IngressController.

openshift-ci-robot · 2023-08-03T06:03:14Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

Test_desiredCanaryRoute: Fix owner reference check

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

Test_desiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (desiredCanaryService): Specify spec.subdomain.(desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_canaryRouteChanged): Verify that canaryRouteChanged checks and updates the spec.host and spec.subdomain fields.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

test/e2e/canary_test.go (TestCanaryRoute):

test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):

test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):

test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.

test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

~~/hold~~
I'll need to rebase after #880 merges.

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-robot · 2023-08-03T06:05:18Z

@Miciah: This pull request references Jira Issue OCPBUGS-16089, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @jitendar-singh

Details

In response to this:

Test_desiredCanaryRoute: Fix owner reference check

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

Test_desiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (canaryRouteChanged): Check whether spec.host or spec.subdomain have changed, and update them if they have.
(desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_canaryRouteChanged): Verify that canaryRouteChanged checks and updates the spec.host and spec.subdomain fields.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

test/e2e/canary_test.go (TestCanaryRoute):

test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):

test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):

test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.

test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

~~/hold~~
I'll need to rebase after #880 merges.

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.

Miciah · 2023-08-03T06:05:51Z

https://github.com/openshift/cluster-ingress-operator/compare/649435d3a9157535243c51d5bbe4d6a6a15040bc..530d326013c9169d1efee77387f33c9ec13108d4 updates canaryRouteChanged and Test_canaryRouteChanged to check and update the spec.host and spec.subdomain fields.

gcs278 · 2023-08-07T18:00:38Z

Thanks for the detailed responses. Looks good to me.
/approve
/lgtm

openshift-ci · 2023-08-07T18:01:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gcs278

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [gcs278]

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

openshift-ci-robot · 2023-08-07T18:12:31Z

/retest-required

Remaining retests: 0 against base HEAD dbbf314 and 2 for PR HEAD 530d326 in total

Miciah · 2023-08-08T13:43:36Z

e2e-hypershift failed because a bunch of tests failed: TestCreateClusterProxy/ValidateMetricsAreExposed, TestNodePool/NodePool_Tests_Group/TestNTOMachineConfigGetsRolledOut, TestNodePool/NodePool_Tests_Group/TestNodepoolMachineconfigGetsRolledout, TestNodePool/NodePool_Tests_Group/TestNodePoolReplaceUpgrade, TestNodePool/NodePool_Tests_Group/TestNodePoolAutoRepair, TestCreateCluster/ValidateMetricsAreExposed, TestCreateClusterCustomConfig, TestAutoscaling/ValidateMetricsAreExposed, and TestCreateClusterPrivate/ValidateMetricsAreExposed.
/test e2e-hypershift

Miciah · 2023-08-09T13:02:21Z

e2e-aws-operator failed because must-gather failed.
/test e2e-aws-operator

e2e-azure-operator failed because the kube-apiserver operator was slow to roll out changes, with etcd logging "apply request took too long"; Vadim tells me this is "a classic case of slow Azure disks".
/test e2e-azure-operator

openshift-ci · 2023-08-09T15:03:55Z

@Miciah: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci-robot · 2023-08-09T15:08:23Z

@Miciah: Jira Issue OCPBUGS-16089: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#965

Jira Issue OCPBUGS-16089 has been moved to the MODIFIED state.

Details

In response to this:

Test_desiredCanaryRoute: Fix owner reference check

Change Test_desiredCanaryRoute to check both the route's and the service's owner references, and print the correct error message if either the route or the service has an unexpected value.

Before this change, Test_desiredCanaryRoute checked the route's owner references but erroneously referred to the service in the failure message if the route had an unexpected value.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Fix checks for owner references.

Test_desiredCanaryRoute: Use testify

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Set spec.subdomain on the canary route

Set spec.subdomain on the canary route so that the route is exposed using the respective domain of any shard that exposes the route.

Before this change, the canary route specified neither spec.subdomain nor spec.host, and so the API server would set a default value for spec.host using the cluster ingress domain. If the cluster ingress domain didn't match the default IngressController's domain, then this would cause canary checks to fail.

For example, the cluster-admin could end up in this situation by first using the cluster ingress config's appsDomain option to set a custom domain different from the domain of the default IngressController and then deleting the canary route so that it would be recreated with the custom domain. This change ensures that the canary route continues to work in this example.

pkg/operator/controller/canary/controller.go (startCanaryRoutePolling):

pkg/operator/controller/canary/http.go (probeRouteEndpoint): Use the new getRouteHost helper function.

pkg/operator/controller/canary/route.go (canaryRouteChanged): Check whether spec.host or spec.subdomain have changed, and update them if they have.
(desiredCanaryService): Specify spec.subdomain.
(getRouteHost): New function. Return the host name of the route for the default IngressController.

pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Verify that the route has the expected value for spec.subdomain.
(Test_canaryRouteChanged): Verify that canaryRouteChanged checks and updates the spec.host and spec.subdomain fields.
(Test_getRouteHost): New test. Verify that getRouteHost behaves correctly.

test/e2e/canary_test.go (TestCanaryRoute):

test/e2e/client_tls_test.go (TestClientTLS, TestMTLSWithCRLs):

test/e2e/operator_test.go (TestHTTPHeaderCapture, TestHTTPCookieCapture):

test/e2e/router_compression_test.go (TestRouterCompressionOperation)
(testCompressionPolicy, getHttpHeaders): Use the new getRouteHost test helper function.

test/e2e/util_test.go (getRouteHost): New test helper function. Return the host name of the route for the default IngressController.

~~/hold~~
I'll need to rebase after #880 merges.

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-robot added the jira/severity-moderate Referenced Jira bug's severity is moderate for the branch this PR is targeting. label Jul 20, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 20, 2023

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Jul 20, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jul 20, 2023

openshift-ci bot requested review from candita, jitendar-singh and knobunc July 20, 2023 21:08

Miciah force-pushed the OCPBUGS-16089-set-spec.subdomain-on-the-canary-route branch from 3f326ea to 7315ba0 Compare July 20, 2023 23:14

Miciah force-pushed the OCPBUGS-16089-set-spec.subdomain-on-the-canary-route branch from 7315ba0 to 8ac3ec1 Compare July 21, 2023 04:49

Miciah force-pushed the OCPBUGS-16089-set-spec.subdomain-on-the-canary-route branch from 8ac3ec1 to 0546494 Compare July 21, 2023 18:55

Miciah added 2 commits July 24, 2023 09:26

Test_desiredCanaryRoute: Use testify

ca2e8fa

* pkg/operator/controller/canary/route_test.go (Test_desiredCanaryRoute): Use assert.Equal and assert.True from the testify package.

Miciah force-pushed the OCPBUGS-16089-set-spec.subdomain-on-the-canary-route branch from 0546494 to 649435d Compare July 24, 2023 13:28

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2023

openshift-ci bot assigned gcs278 Jul 26, 2023

gcs278 reviewed Jul 28, 2023

View reviewed changes

Miciah force-pushed the OCPBUGS-16089-set-spec.subdomain-on-the-canary-route branch from 649435d to 530d326 Compare August 3, 2023 06:03

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2023

openshift-merge-robot merged commit 8f2f035 into openshift:master Aug 9, 2023

Miciah mentioned this pull request Aug 9, 2023

OCPBUGS-17359: test/e2e: Don't use openshift/origin-node #970

Merged

This was referenced Oct 25, 2023

[release-4.13] OCPBUGS-22402: test/e2e: Don't use openshift/origin-node #991

Merged

[release-4.12] OCPBUGS-22432: test/e2e: Don't use openshift/origin-node #992

Merged

[release-4.11] OCPBUGS-22433: test/e2e: Don't use openshift/origin-node #993

Merged

Miciah mentioned this pull request Apr 24, 2024

OCPBUGS-32887: Allow operator to update Route spec.subdomain #1047

Merged

This was referenced Jul 3, 2024

[release-4.15] OCPBUGS-36466: Allow operator to update Route spec.subdomain #1100

Merged

[release-4.14] OCPBUGS-36467: Allow operator to update Route spec.subdomain #1101

Merged

OCPBUGS-16089: Set spec.subdomain on the canary route #965

OCPBUGS-16089: Set spec.subdomain on the canary route #965

Uh oh!

Conversation

Miciah commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test_desiredCanaryRoute: Fix owner reference check

Test_desiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

openshift-ci-robot commented Jul 20, 2023

TestDesiredCanaryRoute: Fix owner reference check

TestDesiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

Miciah commented Jul 20, 2023

Uh oh!

openshift-ci-robot commented Jul 20, 2023

Uh oh!

Miciah commented Jul 20, 2023

Uh oh!

Miciah commented Jul 20, 2023

Uh oh!

openshift-ci-robot commented Jul 20, 2023

TestDesiredCanaryRoute: Fix owner reference check

TestDesiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

openshift-ci-robot commented Jul 21, 2023

TestDesiredCanaryRoute: Fix owner reference check

TestDesiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

Miciah commented Jul 21, 2023

Uh oh!

Miciah commented Jul 21, 2023

Uh oh!

Miciah commented Jul 21, 2023

Uh oh!

openshift-ci-robot commented Jul 24, 2023

Test_desiredCanaryRoute: Fix owner reference check

Test_desiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

Miciah commented Jul 24, 2023

Uh oh!

Miciah commented Jul 24, 2023

Uh oh!

candita commented Jul 26, 2023

Uh oh!

gcs278 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 3, 2023

Test_desiredCanaryRoute: Fix owner reference check

Test_desiredCanaryRoute: Use testify

Set spec.subdomain on the canary route

Uh oh!

Miciah commented Jul 20, 2023 •

edited

Loading

`Test_desiredCanaryRoute`: Fix owner reference check

`Test_desiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`TestDesiredCanaryRoute`: Fix owner reference check

`TestDesiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`TestDesiredCanaryRoute`: Fix owner reference check

`TestDesiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`TestDesiredCanaryRoute`: Fix owner reference check

`TestDesiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`Test_desiredCanaryRoute`: Fix owner reference check

`Test_desiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`Test_desiredCanaryRoute`: Fix owner reference check

`Test_desiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`Test_desiredCanaryRoute`: Fix owner reference check

`Test_desiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route

`Test_desiredCanaryRoute`: Fix owner reference check

`Test_desiredCanaryRoute`: Use testify

Set `spec.subdomain` on the canary route