Query base path should be used to configure correct path in ingress

[objectiser]

If the query UI base path is defined, then the Ingress needs to also use that path (based on a recent PR in Istio helm chart istio/istio#9588).

Need to consider whether this should also be applied to Route?

Signed-off-by: Gary Brown [email protected]

[jpkrohling]

This change is 

[objectiser]

The following image shows accessing the Jaeger UI through port forwarding using the updated all-in-one-with-options.yaml. When accessing through the ingress, the UI URL is the same as before (i.e. just /search).

[codecov]

Codecov Report

Merging #108 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #108      +/-   ##
=========================================
+ Coverage   99.48%   99.5%   +0.01%     
=========================================
  Files          24      24              
  Lines         967    1002      +35     
=========================================
+ Hits          962     997      +35     
  Misses          5       5

Impacted Files	Coverage Δ
pkg/ingress/query.go	100% <100%> (ø)	⬆️
pkg/deployment/collector.go	100% <0%> (ø)	⬆️
pkg/deployment/agent.go	100% <0%> (ø)	⬆️
pkg/util/util.go	100% <0%> (ø)	⬆️
pkg/deployment/query.go	100% <0%> (ø)	⬆️
pkg/deployment/all-in-one.go	100% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45d9731...0819962. Read the comment docs.

[jpkrohling]

This is a feature that would probably deserve an e2e test. It should be relatively easy to build one: the sidecar test makes an HTTP request to the query service, so, you'd probably just have that on your test and check that you are receiving a valid JSON + 200 OK (or anything else that can be used to positively identify the endpoint as being the query)

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

---

pkg/ingress/query.go, line 54 at r1 (raw file):

			Paths: []v1beta1.HTTPIngressPath{
				v1beta1.HTTPIngressPath{
					Path:    i.jaeger.Spec.Query.Options.Map()["query.base-path"],

I found this code a bit hard to digest. Perhaps it would be better to split this logic into its own function? In there, you could probably just create a rule/rule.HTTP, and then append a specific Path to the Paths, based on where the base-path is located.

[objectiser]

@jpkrohling I've included an initial e2e test but it is currently only checking that the ingress uses the same URI (i.e. the base path is not present).

To check the base path, I will need to obtain an address/port that is normally exposed by running a port-forward. Any idea how would do this? If not I will experiment - its just it takes a while to run the e2e tests :)

BTW - might be better if the e2e tests output more than a single line - I only got:

Running end-to-end tests...
ok  	github.com/jaegertracing/jaeger-operator/test/e2e	262.266s

Is there anyway to get it to output per individual test?

[jpkrohling]

To check the base path, I will need to obtain an address/port that is normally exposed by running a port-forward. Any idea how would do this? If not I will experiment

I think the code you have in the PR should already be very close, you just need to call it from JaegerAllInOne. Or you can create a new source file + new test to host this new scenario and then add it to jaeger_test.go.

its just it takes a while to run the e2e tests :)

The tests are regular Go tests, so, you shouldn't be hard to add a new Makefile target that runs a specific test like go test -run TestName. Or you can just comment out the other tests in jaeger_tests.go, which is what lazy-me does.

BTW - might be better if the e2e tests output more than a single line - I only got:

Agree, but that's standard Go and Operator SDK :/ Not sure if we can control that, or how.

[objectiser]

@jpkrohling Ok calling the test - however the problem is that aswell as testing the ingress, I need to test a port directly on the pod, against a URL "/jaeger/search".

[jpkrohling]

I had only a couple of small comments, but nothing blocking. The tests are also passing for me:

ok github.com/jaegertracing/jaeger-operator/test/e2e 167.675s

Reviewed 5 of 5 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @objectiser)

---

pkg/ingress/query.go, line 42 at r4 (raw file):

	}
	if _, ok := i.jaeger.Spec.AllInOne.Options.Map()["query.base-path"]; ok && strings.ToLower(i.jaeger.Spec.Strategy) == "allinone" {
		spec.Rules = append(spec.Rules, *getRule(i.jaeger.Spec.AllInOne.Options, backend))

nit: Do you need to get a reference from getRule()? Your getRule could probably just return a value instead of reference?

---

pkg/ingress/query.go, line 72 at r4 (raw file):

}

func getRule(options v1alpha1.Options, backend *v1beta1.IngressBackend) *v1beta1.IngressRule {

nit: Perhaps change the backend to receive by value, as that's how you consume it?

[objectiser]

@jpkrohling done

[jpkrohling]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved