Add support for OpenShift routes #93
Conversation
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 99.35% 99.39% +0.03%
==========================================
Files 18 19 +1
Lines 776 822 +46
==========================================
+ Hits 771 817 +46
Misses 5 5
Continue to review full report at Codecov.
|
README.adoc
Outdated
| @@ -149,18 +149,13 @@ NAME HOSTS ADDRESS PORTS AGE | |||
| simplest-query * 192.168.122.34 80 3m | |||
| ---- | |||
|
|
|||
| IMPORTANT: an `Ingress` object is *not* created when the operator is started with the `--openshift=true` flag as, such as when using the resource `operator-openshift.yaml`. | |||
There was a problem hiding this comment.
I think the "as" following flag can be removed?
| ---- | ||
|
|
||
| Check the hostname/port with the following command: | ||
| When using the `operator-openshift.yaml` resource, the Operator will automatically create a `Route` object for the query services. Check the hostname/port with the following command: |
There was a problem hiding this comment.
Possibly the route should only be created if ingress.enabled is true? So it has similar behaviour but just has a platform specific outcome.
deploy/operator-openshift.yaml
Outdated
| ports: | ||
| - containerPort: 60000 | ||
| name: metrics | ||
| args: ["start", "--openshift=true"] |
There was a problem hiding this comment.
Is there any way this can be auto-detected? Avoid having separate operator.yaml if possible.
There was a problem hiding this comment.
I have started a discussion in the Operator SDK mailing list about this, but I think we want a flag anyway, to explicitly set a target, bypassing the auto-detection.
pkg/apis/io/v1alpha1/types.go
Outdated
| @@ -32,6 +32,7 @@ type JaegerSpec struct { | |||
| Agent JaegerAgentSpec `json:"agent"` | |||
| Storage JaegerStorageSpec `json:"storage"` | |||
| Ingress JaegerIngressSpec `json:"ingress"` | |||
| Route JaegerRouteSpec `json:"route"` | |||
There was a problem hiding this comment.
As mentioned above - it would be good if route and ingress could be treated as same concept. If at some point in the future there are reasons to have platform specific "ingress" related options, then these could go in platform specific sub-specs?
pkg/cmd/start/main.go
Outdated
| viper.BindPFlag("jaeger-cassandra-schema-image", cmd.Flags().Lookup("jaeger-cassandra-schema-image")) | ||
|
|
||
| cmd.Flags().Bool("openshift", false, "Whether the operator should use OpenShift-specific features, like Routes and OAuth proxy for the UIs") | ||
| viper.BindPFlag("openshift", cmd.Flags().Lookup("openshift")) |
There was a problem hiding this comment.
As mentioned before, would be great if this could be auto-detected - but if not easy to do for now, then might be better to have a platform option, which defaults to kubernetes and openshift is another valid value.
…hift' flag Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
Support-for-OpenShift-Routes
branch
from
848e855
to
78de5ca
Compare
|
This is now ready for a second review round. |
| // FlagPlatformOpenShift represents the value for the 'platform' flag for OpenShift | ||
| FlagPlatformOpenShift = "openshift" | ||
|
|
||
| // FlagPlatformKubernetes represents the value for the 'platform' flag for Kubernetes |
There was a problem hiding this comment.
nit: move kubernetes up as the first option - don't want to show favouritism :)
| qi := ingress.NewQueryIngress(c.jaeger).Get() | ||
| if nil != qi { | ||
| os = append(os, qi) | ||
| if viper.GetString("platform") == v1alpha1.FlagPlatformOpenShift { |
There was a problem hiding this comment.
Is it worth moving this platform specific difference into the pkg/ingress package so reused in production strategy as well - and localises the platform differences?
There was a problem hiding this comment.
I think this kind of logic actually belongs to the controller, as it's the part that decides what should get created (as opposed to the "how", which is inside the specific object packages).
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
There was a problem hiding this comment.
Reviewed 12 of 24 files at r1, 10 of 12 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @objectiser)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @objectiser)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Closes #88
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de