-
Notifications
You must be signed in to change notification settings - Fork 218
NE-1277: status: Add Gateway API objects to relatedObjects #933
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ import ( | |
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/google/go-cmp/cmp/cmpopts" | ||
| sailv1 "github.com/istio-ecosystem/sail-operator/api/v1" | ||
| operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| operatorv1 "github.com/openshift/api/operator/v1" | ||
|
|
@@ -26,9 +28,12 @@ import ( | |
| "k8s.io/apimachinery/pkg/types" | ||
| utilclock "k8s.io/utils/clock" | ||
|
|
||
| gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
|
|
||
| "sigs.k8s.io/controller-runtime/pkg/cache" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller" | ||
| "sigs.k8s.io/controller-runtime/pkg/event" | ||
| "sigs.k8s.io/controller-runtime/pkg/handler" | ||
| "sigs.k8s.io/controller-runtime/pkg/manager" | ||
| "sigs.k8s.io/controller-runtime/pkg/predicate" | ||
|
|
@@ -92,11 +97,33 @@ func New(mgr manager.Manager, config Config) (controller.Controller, error) { | |
| if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.ClusterOperator{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.NewPredicateFuncs(isIngressClusterOperator))); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if config.GatewayAPIEnabled { | ||
| if err := c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.Subscription{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.Funcs{ | ||
| CreateFunc: func(e event.CreateEvent) bool { | ||
| return e.Object.GetNamespace() == operatorcontroller.OpenshiftOperatorNamespace | ||
| }, | ||
| UpdateFunc: func(e event.UpdateEvent) bool { | ||
| return false | ||
| }, | ||
| DeleteFunc: func(e event.DeleteEvent) bool { | ||
| return e.Object.GetNamespace() == operatorcontroller.OpenshiftOperatorNamespace | ||
| }, | ||
| GenericFunc: func(e event.GenericEvent) bool { | ||
| return false | ||
| }, | ||
| })); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
|
||
| return c, nil | ||
| } | ||
|
|
||
| // Config holds all the things necessary for the controller to run. | ||
| type Config struct { | ||
| // GatewayAPIEnabled indicates that the "GatewayAPI" featuregate is enabled. | ||
| GatewayAPIEnabled bool | ||
| IngressControllerImage string | ||
| CanaryImage string | ||
| OperatorReleaseVersion string | ||
|
|
@@ -159,6 +186,10 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( | |
| related = append(related, configv1.ObjectReference{ | ||
| Resource: "namespaces", | ||
| Name: state.IngressNamespace.Name, | ||
| }, configv1.ObjectReference{ | ||
| Group: iov1.GroupVersion.Group, | ||
| Resource: "dnsrecords", | ||
| Namespace: state.IngressNamespace.Name, | ||
| }) | ||
| } | ||
| if state.CanaryNamespace != nil { | ||
|
|
@@ -167,6 +198,33 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( | |
| Name: state.CanaryNamespace.Name, | ||
| }) | ||
| } | ||
| if r.config.GatewayAPIEnabled { | ||
| related = append(related, configv1.ObjectReference{ | ||
| Group: gatewayapiv1.GroupName, | ||
| Resource: "gatewayclasses", | ||
| }) | ||
| if state.haveOSSMSubscription { | ||
| subscriptionName := operatorcontroller.ServiceMeshOperatorSubscriptionName() | ||
| related = append(related, configv1.ObjectReference{ | ||
| Group: operatorsv1alpha1.GroupName, | ||
| Resource: "subscriptions", | ||
| Namespace: subscriptionName.Namespace, | ||
| Name: subscriptionName.Name, | ||
| }) | ||
| if state.IngressNamespace != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible for a gateway to be installed outside the IngressNamespace and still be a related object? Also, how about HTTPRoutes, GRPCRoutes, and ReferenceGrants? Is there a way to make sure the pod that runs conformance tests ends up as a related object so we can check the logs in artifacts?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think it would be appropriate to include objects that constitute user workload, and for component-owned objects, the precedent is that the component that owns the object specifies it in its
We don't have any. Again, we wouldn't want to include other components' objects or objects related to user workload. This is analogous to how the ingress clusteroperator specifies ingresscontrollers but not routes in
I think that that would be misuse of |
||
| related = append(related, configv1.ObjectReference{ | ||
| Group: sailv1.GroupVersion.Group, | ||
| Resource: "istios", | ||
| Namespace: state.IngressNamespace.Name, | ||
| }) | ||
| related = append(related, configv1.ObjectReference{ | ||
| Group: gatewayapiv1.GroupName, | ||
| Resource: "gateways", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't have our own HTTPRoutes, GRPCRoutes, or ReferenceGrants, we also don't have our own Gateways. Gateways are also user workload objects, aren't they?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The delineation here is a little blurry. Gateways are akin to ingresscontrollers, in that the cluster-admin creates them to configure infrastructure for workload, so it's infrastructure but with some overlap with workload. I think it's appropriate for must-gather to include gateways because they could be useful in diagnosing customer issues, and they shouldn't have anything too sensitive (for example, the certificate is in a secret that the gateway references, not in the gateway itself). |
||
| Namespace: state.IngressNamespace.Name, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| co.Status.RelatedObjects = related | ||
|
|
||
|
|
@@ -236,6 +294,8 @@ type operatorState struct { | |
| CanaryNamespace *corev1.Namespace | ||
| IngressControllers []operatorv1.IngressController | ||
| DNSRecords []iov1.DNSRecord | ||
|
|
||
| haveOSSMSubscription bool | ||
| } | ||
|
|
||
| // getOperatorState gets and returns the resources necessary to compute the | ||
|
|
@@ -268,6 +328,18 @@ func (r *reconciler) getOperatorState(ingressNamespace, canaryNamespace string) | |
| state.IngressControllers = ingressList.Items | ||
| } | ||
|
|
||
| if r.config.GatewayAPIEnabled { | ||
| var subscription operatorsv1alpha1.Subscription | ||
| subscriptionName := operatorcontroller.ServiceMeshOperatorSubscriptionName() | ||
| if err := r.cache.Get(context.TODO(), subscriptionName, &subscription); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return state, fmt.Errorf("failed to get subscription %q: %v", subscriptionName, err) | ||
| } | ||
| } else { | ||
| state.haveOSSMSubscription = true | ||
| } | ||
| } | ||
|
|
||
| return state, nil | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,65 +36,66 @@ import ( | |
| func TestCanaryRoute(t *testing.T) { | ||
| kubeConfig, err := config.GetConfig() | ||
| if err != nil { | ||
| t.Fatalf("failed to get kube config: %v", err) | ||
| t.Fatalf("Failed to get kube config: %v", err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If any more test changes are needed, could you please put them in a separate PR?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean additional changes to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can split the other test fixes off into separate PRs and Jira issues if you prefer. I've just been adding them as commits in this PR as I diagnose CI failures that impact this PR. |
||
| } | ||
|
|
||
| client, err := kubernetes.NewForConfig(kubeConfig) | ||
| if err != nil { | ||
| t.Fatalf("failed to create kube client: %v", err) | ||
| t.Fatalf("Failed to create kube client: %v", err) | ||
| } | ||
|
|
||
| // check that the default ingress controller is ready. | ||
| t.Log("Checking that the default ingresscontroller is ready...") | ||
| def := &operatorv1.IngressController{} | ||
| if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, defaultName, defaultAvailableConditions...); err != nil { | ||
| t.Fatalf("failed to observe expected conditions: %v", err) | ||
| t.Fatalf("Failed to observe expected conditions: %v", err) | ||
| } | ||
|
|
||
| if err := kclient.Get(context.TODO(), defaultName, def); err != nil { | ||
| t.Fatalf("failed to get default ingresscontroller: %v", err) | ||
| t.Fatalf("Failed to get the default ingresscontroller: %v", err) | ||
| } | ||
|
|
||
| // Get default ingress controller deployment. | ||
| t.Log("Getting the default ingresscontroller deployment...") | ||
| deployment := &appsv1.Deployment{} | ||
| if err := kclient.Get(context.TODO(), controller.RouterDeploymentName(def), deployment); err != nil { | ||
| t.Fatalf("failed to get ingresscontroller deployment: %v", err) | ||
| t.Fatalf("Failed to get the router deployment: %v", err) | ||
| } | ||
|
|
||
| // Get canary route. | ||
| t.Log("Getting the canary route...") | ||
| canaryRoute := &routev1.Route{} | ||
| name := controller.CanaryRouteName() | ||
| err = wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) { | ||
| if err := kclient.Get(context.TODO(), name, canaryRoute); err != nil { | ||
| t.Logf("failed to get canary route %s: %v", name, err) | ||
| t.Logf("Failed to get route %s: %v", name, err) | ||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("failed to observe canary route: %v", err) | ||
| t.Fatalf("Failed to observe canary route: %v", err) | ||
| } | ||
|
|
||
| canaryRouteHost := getRouteHost(canaryRoute, defaultName.Name) | ||
| if canaryRouteHost == "" { | ||
| t.Fatalf("failed to find host name for the %q router in route %s/%s: %#v", defaultName.Name, name.Namespace, name.Name, canaryRoute) | ||
| t.Fatalf("Failed to find host name for the %q router in route %s: %+v", defaultName.Name, name, canaryRoute) | ||
| } | ||
|
|
||
| image := deployment.Spec.Template.Spec.Containers[0].Image | ||
| clientPod := buildCanaryCurlPod("canary-route-check", canaryRoute.Namespace, image, canaryRouteHost) | ||
| if err := kclient.Create(context.TODO(), clientPod); err != nil { | ||
| t.Fatalf("failed to create pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) | ||
| t.Fatalf("Failed to create pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) | ||
| } | ||
| t.Cleanup(func() { | ||
| if err := kclient.Delete(context.TODO(), clientPod); err != nil { | ||
| if errors.IsNotFound(err) { | ||
| return | ||
| } | ||
| t.Errorf("failed to delete pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) | ||
| t.Errorf("Failed to delete pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) | ||
| } | ||
| }) | ||
|
|
||
| // Test canary route and verify that the hello-openshift echo pod is running properly. | ||
| t.Log("Curl the canary route and verify that it sends the expected response...") | ||
| var lines []string | ||
| err = wait.PollImmediate(1*time.Second, 5*time.Minute, func() (bool, error) { | ||
| readCloser, err := client.CoreV1().Pods(clientPod.Namespace).GetLogs(clientPod.Name, &corev1.PodLogOptions{ | ||
| Container: "curl", | ||
|
|
@@ -103,14 +104,17 @@ func TestCanaryRoute(t *testing.T) { | |
| if err != nil { | ||
| return false, nil | ||
| } | ||
|
|
||
| scanner := bufio.NewScanner(readCloser) | ||
| t.Cleanup(func() { | ||
| defer func() { | ||
| if err := readCloser.Close(); err != nil { | ||
| t.Errorf("failed to close reader for pod %s: %v", clientPod.Name, err) | ||
| t.Errorf("Failed to close reader for logs from pod %s/%s: %v", clientPod.Namespace, clientPod.Name, err) | ||
| } | ||
| }) | ||
| }() | ||
|
|
||
| foundBody := false | ||
| foundRequestPortHeader := false | ||
| lines = []string{} | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if strings.Contains(line, canarycontroller.CanaryHealthcheckResponse) { | ||
|
|
@@ -119,14 +123,21 @@ func TestCanaryRoute(t *testing.T) { | |
| if strings.Contains(strings.ToLower(line), "x-request-port:") { | ||
| foundRequestPortHeader = true | ||
| } | ||
| if foundBody && foundRequestPortHeader { | ||
| return true, nil | ||
| } | ||
| lines = append(lines, line) | ||
| } | ||
|
|
||
| if foundBody && foundRequestPortHeader { | ||
| t.Log("Found the expected response body and header") | ||
|
|
||
| return true, nil | ||
| } | ||
|
|
||
| return false, nil | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("failed to observe the expected canary response body: %v", err) | ||
| t.Logf("Got pods logs:\n%s", strings.Join(lines, "\n")) | ||
|
|
||
| t.Fatalf("Failed to observe the expected canary response body: %v", err) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -219,24 +219,49 @@ func TestClusterOperatorStatusRelatedObjects(t *testing.T) { | |||||
| Resource: "namespaces", | ||||||
| Name: "openshift-ingress", | ||||||
| }, | ||||||
| { | ||||||
| Group: iov1.GroupVersion.Group, | ||||||
| Resource: "dnsrecords", | ||||||
| Namespace: "openshift-ingress", | ||||||
| }, | ||||||
|
Comment on lines
+222
to
+226
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| { | ||||||
| Resource: "namespaces", | ||||||
| Name: "openshift-ingress-canary", | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| if gatewayAPIEnabled, err := isFeatureGateEnabled(features.FeatureGateGatewayAPI); err != nil { | ||||||
| t.Fatalf("Failed to look up %q featuregate: %v", features.FeatureGateGatewayAPI, err) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The capitalized beginning isn't consistent with other test logging.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same issue came up recently here: #1152 (comment) |
||||||
| } else if gatewayAPIEnabled { | ||||||
| expected = append(expected, configv1.ObjectReference{ | ||||||
| Group: "gateway.networking.k8s.io", | ||||||
| Resource: "gatewayclasses", | ||||||
| }) | ||||||
| // This test runs before TestGatewayAPI, so we do *not* expect | ||||||
| // to see subscriptions, istios, or gateways in relatedObjects. | ||||||
|
Comment on lines
+240
to
+241
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this runs before TestGatewayAPI, what created the gatewayClass?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The gatewayapi controller creates the CRD if the featuregate is enabled. We only need the CRD to exist in order to add the gatewayclasses resource to |
||||||
| } | ||||||
|
|
||||||
| coName := controller.IngressClusterOperatorName() | ||||||
| err := wait.PollImmediate(1*time.Second, 5*time.Minute, func() (bool, error) { | ||||||
| co := &configv1.ClusterOperator{} | ||||||
| if err := kclient.Get(context.TODO(), coName, co); err != nil { | ||||||
| t.Logf("failed to get ingress cluster operator %s: %v", coName, err) | ||||||
| t.Logf("Failed to get clusteroperator %q: %v", coName.Name, err) | ||||||
|
|
||||||
| return false, nil | ||||||
| } | ||||||
|
|
||||||
| return reflect.DeepEqual(expected, co.Status.RelatedObjects), nil | ||||||
| if !reflect.DeepEqual(expected, co.Status.RelatedObjects) { | ||||||
| t.Logf("Expected %+v, found %+v", expected, co.Status.RelatedObjects) | ||||||
|
|
||||||
| return false, nil | ||||||
| } | ||||||
|
|
||||||
| t.Log("Found the expected status.relatedObjects") | ||||||
|
|
||||||
| return true, nil | ||||||
| }) | ||||||
| if err != nil { | ||||||
| t.Errorf("did not get expected status related objects: %v", err) | ||||||
| t.Errorf("Did not get expected status related objects: %v", err) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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 didn't expect the addition of dnsRecords as a Gateway API related object. Why is this needed?
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.
The gateway-service-dns controller puts dnsrecords in that namespace, so that it can put set owner reference on services in the same namespace.
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.
Why isn't this under the
condition?
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.
It doesn't need to be as the CRD exists anyway. Furthermore, it makes sense to gather dnsrecords if they do somehow exist in the "openshift-ingress" namespace even without the featuregate enabled because the logic in the dns controller that reconciles dnsrecords in the "openshift-ingress" namespace isn't behind the featuregate.