From bbb448b63693a14e8f519a96a0d8f3b94ce93727 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Thu, 31 Aug 2023 19:05:34 +0200 Subject: [PATCH 1/5] Make OpenShift routes work with missing hostname Signed-off-by: Pavol Loffay --- internal/manifests/collector/route.go | 7 ------ tests/e2e-openshift/route/00-assert.yaml | 25 +++++++++++++++++-- tests/e2e-openshift/route/00-install.yaml | 2 +- .../route/01-report-empty-otlphttp-spans.yaml | 9 +++++++ ...ml => 01-report-empty-otlphttp-spans.yaml} | 0 5 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 tests/e2e-openshift/route/01-report-empty-otlphttp-spans.yaml rename tests/e2e/ingress-subdomains/{01-report-http-spans.yaml => 01-report-empty-otlphttp-spans.yaml} (100%) diff --git a/internal/manifests/collector/route.go b/internal/manifests/collector/route.go index d84fa1854b..1251552625 100644 --- a/internal/manifests/collector/route.go +++ b/internal/manifests/collector/route.go @@ -66,11 +66,6 @@ func Routes(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetr routes := make([]*routev1.Route, len(ports)) for i, p := range ports { portName := naming.PortName(p.Name, p.Port) - path := "/" - // passthrough termination does not support paths - if otelcol.Spec.Ingress.Route.Termination == v1alpha1.TLSRouteTerminationTypePassthrough { - path = "" - } routes[i] = &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: naming.Route(otelcol.Name, p.Name), @@ -83,8 +78,6 @@ func Routes(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetr }, }, Spec: routev1.RouteSpec{ - Host: fmt.Sprintf("%s.%s", portName, otelcol.Spec.Ingress.Hostname), - Path: path, To: routev1.RouteTargetReference{ Kind: "Service", Name: naming.Service(otelcol.Name), diff --git a/tests/e2e-openshift/route/00-assert.yaml b/tests/e2e-openshift/route/00-assert.yaml index 18f1e0265f..f0366fa187 100644 --- a/tests/e2e-openshift/route/00-assert.yaml +++ b/tests/e2e-openshift/route/00-assert.yaml @@ -22,11 +22,32 @@ metadata: kind: OpenTelemetryCollector name: simplest spec: - host: otlp-grpc.example.com - path: / port: targetPort: otlp-grpc to: kind: Service name: simplest-collector wildcardPolicy: None +--- +apiVersion: route.openshift.io/v1 +kind: Route +metadata: + annotations: + something.com: "true" + labels: + app.kubernetes.io/managed-by: opentelemetry-operator + app.kubernetes.io/name: otlp-http-simplest-route + name: otlp-http-simplest-route + ownerReferences: + - apiVersion: opentelemetry.io/v1alpha1 + blockOwnerDeletion: true + controller: true + kind: OpenTelemetryCollector + name: simplest +spec: + port: + targetPort: otlp-http + to: + kind: Service + name: simplest-collector + wildcardPolicy: None diff --git a/tests/e2e-openshift/route/00-install.yaml b/tests/e2e-openshift/route/00-install.yaml index b2f47baafe..44bc7aeca5 100644 --- a/tests/e2e-openshift/route/00-install.yaml +++ b/tests/e2e-openshift/route/00-install.yaml @@ -7,7 +7,6 @@ spec: mode: "deployment" ingress: type: route - hostname: "example.com" annotations: something.com: "true" route: @@ -18,6 +17,7 @@ spec: otlp: protocols: grpc: + http: exporters: logging: diff --git a/tests/e2e-openshift/route/01-report-empty-otlphttp-spans.yaml b/tests/e2e-openshift/route/01-report-empty-otlphttp-spans.yaml new file mode 100644 index 0000000000..406d66cef2 --- /dev/null +++ b/tests/e2e-openshift/route/01-report-empty-otlphttp-spans.yaml @@ -0,0 +1,9 @@ +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +commands: + - script: | + #!/bin/bash + set -ex + # Export empty payload and check of collector accepted it with 2xx status code + otlp_http_host=$(kubectl get route otlp-http-simplest-route -n $NAMESPACE -o jsonpath='{.spec.host}') + for i in {1..40}; do curl --fail -ivX POST http://${otlp_http_host}:80/v1/traces -H "Content-Type: application/json" -d '{}' && break || sleep 1; done diff --git a/tests/e2e/ingress-subdomains/01-report-http-spans.yaml b/tests/e2e/ingress-subdomains/01-report-empty-otlphttp-spans.yaml similarity index 100% rename from tests/e2e/ingress-subdomains/01-report-http-spans.yaml rename to tests/e2e/ingress-subdomains/01-report-empty-otlphttp-spans.yaml From fbca2702abee896edbab6474684906a9aece4e92 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Thu, 31 Aug 2023 19:19:33 +0200 Subject: [PATCH 2/5] Fix Signed-off-by: Pavol Loffay --- .chloggen/route-use-defaults.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100755 .chloggen/route-use-defaults.yaml diff --git a/.chloggen/route-use-defaults.yaml b/.chloggen/route-use-defaults.yaml new file mode 100755 index 0000000000..ca4df7bbd5 --- /dev/null +++ b/.chloggen/route-use-defaults.yaml @@ -0,0 +1,16 @@ +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: breaking + +# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) +component: operator + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Make OpenShift routes work with missing hostname + +# One or more tracking issues related to the change +issues: [2074] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: The OpenShift route hostnames change to `--route--basedomain` e.g. from `otlp-http.apps-crc.testing` to `otlp-http-otel-route-ploffay.apps-crc.testing`. From 0f6bde6706cefda715bcc2996fe57d4a2da561fb Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Fri, 1 Sep 2023 10:39:56 +0200 Subject: [PATCH 3/5] Fix Signed-off-by: Pavol Loffay --- internal/manifests/collector/route.go | 6 ++++ internal/manifests/collector/route_test.go | 42 ++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/internal/manifests/collector/route.go b/internal/manifests/collector/route.go index 1251552625..d8e8899a94 100644 --- a/internal/manifests/collector/route.go +++ b/internal/manifests/collector/route.go @@ -66,6 +66,11 @@ func Routes(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetr routes := make([]*routev1.Route, len(ports)) for i, p := range ports { portName := naming.PortName(p.Name, p.Port) + host := "" + if otelcol.Spec.Ingress.Hostname != "" { + host = fmt.Sprintf("%s.%s", portName, otelcol.Spec.Ingress.Hostname) + } + routes[i] = &routev1.Route{ ObjectMeta: metav1.ObjectMeta{ Name: naming.Route(otelcol.Name, p.Name), @@ -78,6 +83,7 @@ func Routes(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTelemetr }, }, Spec: routev1.RouteSpec{ + Host: host, To: routev1.RouteTargetReference{ Kind: "Service", Name: naming.Service(otelcol.Name), diff --git a/internal/manifests/collector/route_test.go b/internal/manifests/collector/route_test.go index 2accf03108..395f54c67e 100644 --- a/internal/manifests/collector/route_test.go +++ b/internal/manifests/collector/route_test.go @@ -21,6 +21,7 @@ import ( routev1 "github.com/openshift/api/route/v1" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" @@ -137,6 +138,47 @@ func TestDesiredRoutes(t *testing.T) { }, }, got) }) + t.Run("hostname is set", func(t *testing.T) { + params, err := newParams("something:tag", testFileIngress) + if err != nil { + t.Fatal(err) + } + + params.Instance.Namespace = "test" + params.Instance.Spec.Ingress = v1alpha1.Ingress{ + Hostname: "example.com", + Type: v1alpha1.IngressTypeRoute, + Route: v1alpha1.OpenShiftRoute{ + Termination: v1alpha1.TLSRouteTerminationTypeInsecure, + }, + } + + routes := Routes(params.Config, params.Log, params.Instance) + require.Equal(t, 3, len(routes)) + assert.Equal(t, "web.example.com", routes[0].Spec.Host) + assert.Equal(t, "otlp-grpc.example.com", routes[1].Spec.Host) + assert.Equal(t, "otlp-test-grpc.example.com", routes[2].Spec.Host) + }) + t.Run("hostname is not set", func(t *testing.T) { + params, err := newParams("something:tag", testFileIngress) + if err != nil { + t.Fatal(err) + } + + params.Instance.Namespace = "test" + params.Instance.Spec.Ingress = v1alpha1.Ingress{ + Type: v1alpha1.IngressTypeRoute, + Route: v1alpha1.OpenShiftRoute{ + Termination: v1alpha1.TLSRouteTerminationTypeInsecure, + }, + } + + routes := Routes(params.Config, params.Log, params.Instance) + require.Equal(t, 3, len(routes)) + assert.Equal(t, "", routes[0].Spec.Host) + assert.Equal(t, "", routes[1].Spec.Host) + assert.Equal(t, "", routes[2].Spec.Host) + }) } func TestRoutes(t *testing.T) { From e8d48f1df28e7cb68046ad94422971df1070fba5 Mon Sep 17 00:00:00 2001 From: Pavol Loffay Date: Mon, 4 Sep 2023 12:16:00 +0200 Subject: [PATCH 4/5] Fix the changelog entry Signed-off-by: Pavol Loffay --- .chloggen/route-use-defaults.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.chloggen/route-use-defaults.yaml b/.chloggen/route-use-defaults.yaml index ca4df7bbd5..0323367b60 100755 --- a/.chloggen/route-use-defaults.yaml +++ b/.chloggen/route-use-defaults.yaml @@ -1,5 +1,5 @@ # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' -change_type: breaking +change_type: enhancement # The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action) component: operator @@ -13,4 +13,4 @@ issues: [2074] # (Optional) One or more lines of additional information to render under the primary note. # These lines will be padded with 2 spaces and then inserted directly into the document. # Use pipe (|) for multiline entries. -subtext: The OpenShift route hostnames change to `--route--basedomain` e.g. from `otlp-http.apps-crc.testing` to `otlp-http-otel-route-ploffay.apps-crc.testing`. +subtext: If the ingeress hostname is not specified OpenShift route hostname is set to `--route--basedomain`. From 4de161449b127b4dc384f28069b7ce6f9c5ce2c5 Mon Sep 17 00:00:00 2001 From: Ben B Date: Mon, 4 Sep 2023 13:32:40 +0200 Subject: [PATCH 5/5] Update .chloggen/route-use-defaults.yaml Co-authored-by: Israel Blancas --- .chloggen/route-use-defaults.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.chloggen/route-use-defaults.yaml b/.chloggen/route-use-defaults.yaml index 0323367b60..f14593125a 100755 --- a/.chloggen/route-use-defaults.yaml +++ b/.chloggen/route-use-defaults.yaml @@ -13,4 +13,4 @@ issues: [2074] # (Optional) One or more lines of additional information to render under the primary note. # These lines will be padded with 2 spaces and then inserted directly into the document. # Use pipe (|) for multiline entries. -subtext: If the ingeress hostname is not specified OpenShift route hostname is set to `--route--basedomain`. +subtext: If the Ingress hostname is not specified OpenShift route hostname is set to `--route--basedomain`.