Create Ingress or Route for Jaeger UI #255

andreasgerstmayr · 2023-02-23T17:34:51Z

if jaegerQuery is enabled, create an Ingress or Route object by default, depending on the OpenShift.Route feature gate
show an error if Ingress or Route is enabled but jaegerQuery is disabled

TODO:

Should we auto-detect OpenShift by querying ServerGroups, like jaeger-operator does? This would make the OpenShift.Route feature gate obsolete and would be a bit more user friendly imho

iblancasa

Could we add a test for OpenShift Routes and Ingresses?

apis/config/v1alpha1/projectconfig_types.go

apis/tempo/v1alpha1/microservices_types.go

config/rbac/role.yaml

internal/manifests/queryfrontend/query_frontend.go

codecov-commenter · 2023-02-27T12:24:54Z

Codecov Report

Patch coverage: 78.80% and project coverage change: +0.89 🎉

Comparison is base (d4f1b0f) 60.69% compared to head (1263485) 61.58%.

❗ Current head 1263485 differs from pull request most recent head df75673. Consider uploading reports for the commit df75673 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   60.69%   61.58%   +0.89%     
==========================================
  Files          39       39              
  Lines        2984     3113     +129     
==========================================
+ Hits         1811     1917     +106     
- Misses       1076     1094      +18     
- Partials       97      102       +5

Flag	Coverage Δ
unittests	`61.58% <78.80%> (+0.89%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
apis/tempo/v1alpha1/microservices_types.go	`100.00% <ø> (ø)`
controllers/tempo/microservices_controller.go	`43.91% <0.00%> (-1.44%)`	⬇️
apis/tempo/v1alpha1/microservices_webhook.go	`65.46% <77.08%> (+3.43%)`	⬆️
internal/manifests/queryfrontend/query_frontend.go	`88.72% <88.88%> (-0.08%)`	⬇️
cmd/generate/main.go	`21.05% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

* if jaegerQuery is enabled, create an Ingress or Route object by default, depending on the OpenShift.Route feature gate * show an error if Ingress or Route is enabled but jaegerQuery is disabled Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

…ftRoute Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

…me, check if route is enabled but feature gate is disabled

…S by default Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

apis/tempo/v1alpha1/ingress_types.go

apis/config/v1alpha1/projectconfig_types.go

andreasgerstmayr · 2023-02-28T14:13:41Z

Could we add a test for OpenShift Routes and Ingresses?

I added an assertion for the ingress in the smoketest-with-jaeger e2e.

I'm currently thinking how to make one for Route - the problem is we need to enable the OpenShiftRoutes feature gate in the test. I'm unsure if I should really update the operator configmap and restart the operator inside a kuttl test? Parallel kuttl tests will then fail of course, and imho the tests should not restart or update the operator's configuration.

Alternatively we could remove this feature gate and auto-detect the availability of openshift route. wdyt?

iblancasa · 2023-02-28T15:18:49Z

I'm currently thinking how to make one for Route - the problem is we need to enable the OpenShiftRoutes feature gate in the test. I'm unsure if I should really update the operator configmap and restart the operator inside a kuttl test? Parallel kuttl tests will then fail of course, and imho the tests should not restart or update the operator's configuration.

That is one of the reasons why I don't like running the tests in parallel.

I agree about the modification of the operator for the tests but... it is a common use case to test the different operator features. The last step of the test can be reverting back the changes done to the operator.

Alternatively we could remove this feature gate and auto-detect the availability of openshift route. wdyt?

I don't have a strong opinion but, during some discussions, I understand the team prefers to use feature gates because it makes it easier to maintain the operators (because you enable or disable features instead of just detecting them). I don't have a strong opinion about this. @pavolloffay, @frzifus or @rubenvp8510 will provide a better answer than me about this topic.

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

…ress-or-route

andreasgerstmayr · 2023-03-01T18:01:27Z

I've now changed the webhook to not enable any type of ingress by default due to security reasons as @frzifus mentioned.

I updated the Makefile to install the OpenShift router, and enable the feature gate (because now in e2e tests we always have the Route CRD available) and added a new test for creating an ingress with type: route.

Not sure why the "upload coverage report" step fails, the unit tests pass though.

frzifus

💪

frzifus · 2023-03-02T12:28:08Z

cmd/root.go

 	utilruntime.Must(tempov1alpha1.AddToScheme(scheme))
-	utilruntime.Must(configtempov1alpha1.AddToScheme(scheme))
+	utilruntime.Must(configv1alpha1.AddToScheme(scheme))
 	utilruntime.Must(routev1.Install(scheme))
 	utilruntime.Must(openshiftoperatorv1.Install(scheme))
 	utilruntime.Must(configv1.Install(scheme))


Do we need to register routes here too?

They're already registered: utilruntime.Must(routev1.Install(scheme)) (afaics it was part of some gateway PR).

andreasgerstmayr force-pushed the create-jaegerui-ingress-or-route branch 4 times, most recently from ef41639 to aa2f1c9 Compare February 23, 2023 18:30

iblancasa requested changes Feb 27, 2023

View reviewed changes

apis/config/v1alpha1/projectconfig_types.go Outdated Show resolved Hide resolved

apis/tempo/v1alpha1/microservices_types.go Show resolved Hide resolved

config/rbac/role.yaml Show resolved Hide resolved

internal/manifests/queryfrontend/query_frontend.go Outdated Show resolved Hide resolved

andreasgerstmayr added 5 commits February 27, 2023 14:26

Rename featureGates.openshift.route -> featureGates.openshift.openshi…

4d93f36

…ftRoute Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Add validation: check if ingress and route are enabled at the same ti…

f13b8b8

…me, check if route is enabled but feature gate is disabled

Route: add option to specify host, annotations and TLS config; use TL…

7b1c984

…S by default Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Combine Route and Ingress spec in a single spec

832c858

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

andreasgerstmayr force-pushed the create-jaegerui-ingress-or-route branch 2 times, most recently from ac57238 to 1263485 Compare February 27, 2023 18:06

Specify routes/custom-host to allow custom hosts for the Route object

851ec26

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

andreasgerstmayr force-pushed the create-jaegerui-ingress-or-route branch from 1263485 to 851ec26 Compare February 27, 2023 18:35

frzifus reviewed Feb 28, 2023

View reviewed changes

apis/tempo/v1alpha1/ingress_types.go Outdated Show resolved Hide resolved

apis/config/v1alpha1/projectconfig_types.go Show resolved Hide resolved

andreasgerstmayr mentioned this pull request Mar 1, 2023

Existence and object type of gateway's ingress should be configurable #265

Closed

2 tasks

andreasgerstmayr added 3 commits March 1, 2023 16:05

Don't enable ingress by default due to security concerns

7a90410

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Add route e2e test

3b3ad9d

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>

Merge remote-tracking branch 'upstream/main' into create-jaegerui-ing…

df75673

…ress-or-route

andreasgerstmayr requested a review from iblancasa March 1, 2023 18:31

frzifus reviewed Mar 2, 2023

View reviewed changes

frzifus approved these changes Mar 2, 2023

View reviewed changes

iblancasa approved these changes Mar 2, 2023

View reviewed changes

rubenvp8510 approved these changes Mar 2, 2023

View reviewed changes

andreasgerstmayr merged commit 9d5ec2d into grafana:main Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Ingress or Route for Jaeger UI #255

Create Ingress or Route for Jaeger UI #255

andreasgerstmayr commented Feb 23, 2023

iblancasa left a comment

codecov-commenter commented Feb 27, 2023 •

edited by codecov bot

Loading

andreasgerstmayr commented Feb 28, 2023

iblancasa commented Feb 28, 2023

andreasgerstmayr commented Mar 1, 2023

frzifus left a comment

frzifus Mar 2, 2023

andreasgerstmayr Mar 2, 2023

frzifus Mar 2, 2023

Create Ingress or Route for Jaeger UI #255

Create Ingress or Route for Jaeger UI #255

Conversation

andreasgerstmayr commented Feb 23, 2023

iblancasa left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2023 • edited by codecov bot Loading

Codecov Report

andreasgerstmayr commented Feb 28, 2023

iblancasa commented Feb 28, 2023

andreasgerstmayr commented Mar 1, 2023

frzifus left a comment

Choose a reason for hiding this comment

frzifus Mar 2, 2023

Choose a reason for hiding this comment

andreasgerstmayr Mar 2, 2023

Choose a reason for hiding this comment

frzifus Mar 2, 2023

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2023 •

edited by codecov bot

Loading