Add Route for consoles custom hostname #594

jhadvig · 2020-03-11T16:40:12Z

Story: https://issues.redhat.com/browse/CONSOLE-2036

Will run the generator script by the end of the review.

jhadvig · 2020-03-11T16:40:57Z

config/v1/types_console.go

+	Host string `json:host:omitempty`
+	// servingCert references to ConfigMap in the openshift-config namespace that
+	// contains custom certificate.
+	ServingCert ConfigMapNameReference `json:servingCert:omitempty`


Wasn't really sure about this name, but didn't want to duplicate the custom* prefix in every field...

given it also needs to contain the private key (you can't serve a cert without a private key) shouldn't this reference a secret?

Just update the PR with the SecretReference. Thanks !

spadgett · 2020-03-12T15:13:08Z

config/v1/types_console.go

+	// customization is used to optionally provide a small set of
+	// customization options to the web console.
+	// +optional
+	Customization ConsoleCustomization `json:"customization,omitempty"`


I don't see this as customization really. Maybe route? Something like

spec: route: hostname: my-hostname servingCert: my-secret

We spoke with @benjaminapetersen that it might be good to follow the patter that we have in our operator config

I don't mean to bikeshed, but this isn't customization imo. It's basic serving info. I wouldn't think to look there to set these values.

(Technically, any config is a customization I guess, but then it starts to lose meaning.)

To be honest I like both approaches. Since there already is a Authentication field on the ConsoleSpec I kinda incline to go with:

... Route ConsoleRoute `json:"route,omitempty"` ...

@benjaminapetersen thoughts ?

I can roll with this. We have Customization on the operator config which contains a URL, but its an override to a default. Agree, this is a different kind of thing.

I'm good with Route { URL, ServingCert } type of struct.

https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L58 for an example of documentation verbiage.

URL is a valid URI with scheme(http/https)....

We want just hostname here, not URL. That matches what Route API, and we don't want the user to set a scheme, port, or path.

hostname is good, I've been meaning to look up what would be consistent, was thinkging URL isn't quite right.

spadgett · 2020-03-12T15:18:08Z

config/v1/types_console.go

+	CustomURL `json:customURL,omitempty`
+}
+
+// CustomURL replaces the default console URL under which console will be available.


We need to make sure we're documenting the default behavior when these values aren't specified.

spadgett · 2020-03-12T15:20:40Z

config/v1/types_console.go

+	Host string `json:host:omitempty`
+	// servingCert references to Secret in the openshift-config namespace that
+	// contains custom certificate and key.
+	ServingCert corev1.SecretReference `json:servingCert:omitempty`


This probably doesn't need to be required. If you pick a hostname that uses the default routing suffix, you shouldn't need to specify a certificate and key.

Makes sense 👍

Should we document that that's a reasonable thing to do?

spadgett · 2020-03-12T15:21:54Z

config/v1/types_console.go

+	// host is the desired custom URL under which console will be available.
+	Host string `json:host:omitempty`
+	// servingCert references to Secret in the openshift-config namespace that
+	// contains custom certificate and key.


We need to document the fields in the referenced secret that contain the certificate and key.

@spadgett you mean the fields on the Secret itself - tls.crtand tls.key
Or the Name and the Namespace and where they should point to ?

I mean the keys in the secret, tls.crt and tls.key. That needs to be documented somewhere, otherwise the user has no way to know.

Yeah as the Operator will have to pluck them out by this. Unless we let the user configure the keys, but that seems unnecessary.

spadgett · 2020-03-12T15:22:18Z

/cc @bparees

jhadvig · 2020-03-13T13:25:00Z

@spadgett @benjaminapetersen I've update the PR based on your comments

jhadvig · 2020-03-13T13:25:46Z

config/v1/types_console.go

+// CustomRoute replaces the default console hostname under which console will be available.
+type CustomRoute struct {
+	// hostname is the desired custom address under which console will be available.
+	Hostname string `json:hostname`


Here I wasn't sure if we dont wanna follow the Route naming, which would be Host

I think we should probably create ConsoleConfigRoute and do the same as https://github.com/openshift/api/blob/master/imageregistry/v1/types.go#L302. Three fields, include Name. Consistency is good.

Provided we don't go the central config route that is currently being discussed.

// ConsoleConfigRoute holds information on external route access to console type ConsoleConfigRoute struct { // name of the route to be created. Name string `json:"name" protobuf:"bytes,1,opt,name=name"` // hostname for the route. // +optional Hostname string `json:"hostname,omitempty" protobuf:"bytes,2,opt,name=hostname"` // secretName points to secret containing the certificates to be used // by the route. // +optional SecretName string `json:"secretName,omitempty" protobuf:"bytes,3,opt,name=secretName"` }

jhadvig · 2020-03-13T13:28:36Z

config/v1/types_console.go

 	Authentication ConsoleAuthentication `json:"authentication"`
+	// route contains hostname and serving certificate for replacing the default hostname,
+	// under which is console available.
+	// If not specified, default route will be created by the console-operator.


@spadgett @benjaminapetersen would be good to align on the default behaviour, that means if we wanna keep the default route or not, when the custom hostname will be specified.
Not sure if it makes sense to have two urls for console, if the custom one is defined.

I think we do want to keep it, for the squatting reason stated before.
It probably doesn't affect this PR, aside from documentation.

Could update to something like:

If a custom route is specified, a new route will be created with the provided URL. The default route will be maintained but will no longer access the console.

deads2k · 2020-03-16T14:13:53Z

/hold

This approach doesn't seem to address the larger issue of needing to provide custom certificates for all of our components exposed via routes. How about placing this in the ingress config and using an API similar to

api/config/v1/types_apiserver.go

Line 63 in d3d78e0

type APIServerNamedServingCert struct {

to describe them. The individual operators would then be able to select the name for their route.

We could even rename the struct type for inclusion. The kube-apiserver configuration remains separate because it is externally managed and not necessarily exposed via route.

@ironcladlou I was hoping that network-edge would produce something that could be re-used across all exposed routes, like authentication, console, metrics, etc. Does the approach I sketched sound reasonable?

jhadvig · 2020-03-16T14:54:09Z

@deads2k wasnt aware the other components are trying to solve this issue.

Which component (besides the apiserver) is dealing with this issue ?

We could even rename the struct type for inclusion

To solve it uniformly would make sense, but not sure how that would fit into our story where besides the ServingCertificate a specific hostname, under which the console will be available, needs to be specified. Not sure if it would make sense to add the hostname to the mantioned APIServerNamedServingCert struct after inclusion.

@benjaminapetersen @spadgett thoughts ?

deads2k · 2020-03-16T15:31:45Z

@deads2k wasnt aware the other components are trying to solve this issue.

Everything we expose today with a route has this issue and knows what name its looking for.
This includes authentication, console, downloads (currently owned by console), alertmanager, grafana, prometheus, and thanos.

A solution should resolve certs for these route based endpoints in a unified way. Removing wildcard certs requires resolving all of these.

bparees · 2020-03-16T16:06:00Z

@deads2k are you suggesting a central location for all external certs, mapped to hostnames, which ingress would automatically configure matching routes to use?

deads2k · 2020-03-16T17:20:41Z

@deads2k are you suggesting a central location for all external certs, mapped to hostnames, which ingress would automatically configure matching routes to use?

I think it would go in the other direction. Ingress cannot rewrite the operands, but providing hostname/certificate tuples in a central spot in ingresses.config.openshift.io|spec makes sense since it's a general ingress problem. From there, each individual operand has sufficient information to configure the routes.

bparees · 2020-03-16T17:30:41Z

followed up with @deads2k on slack. what he's proposing is a central config api for "map these hostnames to these certs".

individual operators would be responsible for looking at that config and deciding what cert, if any, they needed to configure their operand's routes to use.

I think there's a usability/obviousness/risk-of-breakage tradeoff between central config for this vs having an operator fully own the config in its own resource, but i guess fundamentally i'm ok either way. But i'm also worried about the console team getting blocked on this if the central store api isn't finalized+introduced rapidly.

benjaminapetersen · 2020-03-16T17:56:10Z

It seems like the central list approach is an advantage if components need to share, having them all in one place ensures a uniform API, helps an admin do the right thing.

Would this config have to be named keys, or can it be in a list form? IE, for authentication, console, downloads, alertmanager, grafana, prometheus, and thanos... and foo,bar,baz, can additional items be added w/o PRs to add new keys?

bparees · 2020-03-16T18:00:44Z

Would this config have to be named keys, or can it be in a list form? IE, for authentication, console, downloads, alertmanager, grafana, prometheus, and thanos... and foo,bar,baz, can additional items be added w/o PRs to add new keys?

i would have guessed it would be a hostname/cert pairing and anyone who wants to create a route for a given hostname could look at the config to see if a cert existed for that hostname, and use it.

Though that seems to present another problem w/ the central config (if i understand the idea correctly) which is that all components are going to have to be able to read all the certs for everything. Just getting the cert isn't sufficient to MITM something, but it's a big step.

jhadvig · 2020-03-16T18:56:54Z

Though that seems to present another problem w/ the central config (if i understand the idea correctly) which is that all components are going to have to be able to read all the certs for everything. Just getting the cert isn't sufficient to MITM something, but it's a big step.

In that case, should the openshift-config namespace be used for the Secrets that will be used for storing the certs ?

Also other question is, who will be responsible for the API changes ? Since this change would have bigger impact, than the one the story Im working on is about.

deads2k · 2020-03-16T19:48:23Z

In that case, should the openshift-config namespace be used for the Secrets that will be used for storing the certs ?

Yes.

Also other question is, who will be responsible for the API changes ? Since this change would have bigger impact, than the one the story Im working on is about.

Good APIs can originate from anyone. You probably want to talk with @openshift/sig-network-edge and the other impacted teams

jhadvig · 2020-03-16T19:58:53Z

Good APIs can originate from anyone. You probably want to talk with @openshift/sig-network-edge and the other impacted teams

makes sense

Are there any guesses where the hostname and servingCert will be located ? since I doubt that we will read those from console config (as the current PR suggests)

ironcladlou · 2020-03-17T19:34:10Z

I must be misunderstanding the proposal because I'm not clear how it impacts ingress operator or the router. It sounds like you're talking about centralizing cert data and making changes to (non-ingress) operators to drive how they create routes and inject cert config into the routes they create.

What specific impact would that have to the ingress operator or ingress controller (router)?

bparees · 2020-03-17T19:38:14Z

What specific impact would that have to the ingress operator or ingress controller (router)?

i think none, other than you'd own the CRD that contained the central config that @deads2k is proposing. So your operator would probably include the resource yaml for that CRD.

but i'm still interested in the answer to this, also:

Though that seems to present another problem w/ the central config (if i understand the idea correctly) which is that all components are going to have to be able to read all the certs for everything. Just getting the cert isn't sufficient to MITM something, but it's a big step.

deads2k · 2020-03-17T20:45:50Z

Though that seems to present another problem w/ the central config (if i understand the idea correctly) which is that all components are going to have to be able to read all the certs for everything. Just getting the cert isn't sufficient to MITM something, but it's a big step.

It doesn't have to be all certs, a cluster-admin could choose to limit the access and operators could watch a named item (server supports this), but at least a couple of these components an already read secret content from openshift-config since it is the canonical source.

My one reservation would be a future move to OLM, but I think that migration would be straightforward

jhadvig · 2020-03-19T07:43:38Z

/test verify

benjaminapetersen · 2020-03-19T16:18:22Z

config/v1/types_console.go

 type ConsoleSpec struct {
 	// +optional
 	Authentication ConsoleAuthentication `json:"authentication"`
+	// route contains hostname and serving certificate for replacing the default hostname,


Minor tweaks, I'll just inline in your original text:

// route contains hostname and serving certificate for replacing the default route for the console. // If a custom route is specified, a new route will be created with the provided hostname. // The default console route will be maintained to reserve the default hostname for console if the custom route is removed.

benjaminapetersen · 2020-03-19T20:56:06Z

config/v1/types_console.go

+	// If hostname that uses default routing suffix of the given cluster, the
+	// servignCert specification will not be needed.
+	// +optional
+	SecretName string `json:"secretName,omitempty"`


Guessing this should really be a SecretNameReference rather than a string.
Not a lot to it:

// SecretNameReference references a secret in a specific namespace. // The namespace must be specified at the point of use. type SecretNameReference struct { // name is the metadata.name of the referenced secret // +kubebuilder:validation:Required // +required Name string `json:"name"` }

deads2k · 2020-03-20T14:13:47Z

config/v1/types_console.go

+	// The default console route will be maintained to reserve the default hostname for console if the custom route is removed.
+	// If not specified, default route will be 	used.
+	// +optional
+	Route ConsoleConfigRoute `json:"route,omitempty"`


not in this group. Put it on your operator resource.

is there a reason why it can live on the console config ?

it's more that it shouldn't. this is config that's only consumed by your operator, it's not global config that's consumed by others. (the console operator publishes the public route hostname back to this resource under status, which is global information that's consumed by others, or can be)

Our logic has been a custom route is a normal thing a cluster-admin may configure, thus it should exist on the top-level config.

That said, we can move it.

How "normal" it is is not the right deciding factor for where it should go, the things l listed above are

deads2k · 2020-03-20T14:13:59Z

config/v1/types_console.go

+// ConsoleConfigRoute holds information on external route access to console.
+type ConsoleConfigRoute struct {
+	// name of the route to be created.
+	Name string `json:"name"`


why is this configurable

we have been pointed to https://github.com/openshift/api/blob/master/imageregistry/v1/types.go#L304-L314 so wanted to align with what we already use

feel free to improve on what was done in imageregistry. I don't think we had a good reason to make the route name configurable.

We can drop the name.
@jhadvig since we likely want to keep the default route (squat on it) in case the custom route is removed, we can just hard code console-custom as name for the new route.

deads2k · 2020-03-20T14:15:53Z

config/v1/types_console.go

+	Name string `json:"name"`
+	// hostname is the desired custom address under which console will be available.
+	// +optional
+	Hostname string `json:"hostname,omitempty"`


given a different hostname, couldn't they simply point to your service today and configure an unmanaged route as they wish. This could make your API contract on the service name, but nothing else. No new API and no conflict with resources you manage. What is the value of you having them program this API to program a route instead of simply creating the route?

We need to update our OAuth client with the console callback URL. Also the OAuth server whitelists console URL for kubeadmin logout.

We also use cookies and localStorage. Multiple routes for console could be problematic since the cookies won't be shared.

Console also needs to know its own public URL for various reasons (for instance, to build the OAuth callback URL). Maybe we can read the host from the HTTP request, but I'm not sure that's always reliable.

jhadvig · 2020-03-23T16:55:11Z

@spadgett @benjaminapetersen PR updated. PTAL

benjaminapetersen · 2020-03-23T18:48:37Z

operator/v1/types_console.go

+	// Referenced Secret is required to contain following key value pairs:
+	// - "tls.crt" - to specifies custom certificate
+	// - "tls.key" - to specifies private key of the custom certificate
+	// If hostname that uses default routing suffix of the given cluster, the servignCert specification will not be needed.


Minor tweak:
If the custom hostname uses the default routing suffix of the cluster, the Secret specification for a serving cert will not be needed.

ironcladlou · 2020-03-24T13:36:12Z

operator/v1/types_console.go

+// ConsoleConfigRoute holds information on external route access to console.
+type ConsoleConfigRoute struct {
+	// hostname is the desired custom address under which console will be available.
+	Hostname string `json:"hostname,omitempty"`


If this hostname is not a subdomain of the default ingress domain, seems like the admin would have to manually configure DNS to point at the ingress controller. Maybe I'm misunderstanding something.

Just update the PR adding notification regarding that case

deads2k · 2020-03-25T17:44:11Z

operator/v1/types_console.go

+	// for console if the custom route is removed.
+	// If not specified, default route will be used.
+	// +optional
+	Route *ConsoleConfigRoute `json:"route,omitempty"`


if empty strings in this struct are the same as not present (I would expect them to be), then this doesn't need to be a pointer

jhadvig · 2020-03-26T16:55:30Z

@deads2k update the PR. Removed the omitempty form the added field since we are not serializing them.
PTAL

bparees · 2020-03-31T14:58:04Z

/lgtm

bparees · 2020-03-31T14:58:22Z

/hold cancel

openshift-ci-robot · 2020-03-31T15:00:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, jhadvig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot assigned benjaminapetersen and spadgett Mar 11, 2020

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 11, 2020

jhadvig commented Mar 11, 2020

View reviewed changes

openshift-ci-robot requested review from abhinavdahiya and adambkaplan March 11, 2020 16:41

jhadvig force-pushed the customURL branch from eafe422 to a19efb5 Compare March 12, 2020 14:52

spadgett requested changes Mar 12, 2020

View reviewed changes

openshift-ci-robot requested a review from bparees March 12, 2020 15:22

jhadvig force-pushed the customURL branch from a19efb5 to 6cf36c4 Compare March 13, 2020 13:24

jhadvig commented Mar 13, 2020

View reviewed changes

jhadvig force-pushed the customURL branch from 6cf36c4 to 1bd56e6 Compare March 13, 2020 19:22

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 16, 2020

jhadvig changed the title ~~Add CustomURL fields for console~~ Add Route for consoles custom hostname Mar 18, 2020

jhadvig force-pushed the customURL branch from 217deac to 3149add Compare March 19, 2020 14:12

benjaminapetersen reviewed Mar 19, 2020

View reviewed changes

jhadvig force-pushed the customURL branch from 3149add to 0441b2a Compare March 19, 2020 18:33

benjaminapetersen reviewed Mar 19, 2020

View reviewed changes

deads2k reviewed Mar 20, 2020

View reviewed changes

jhadvig force-pushed the customURL branch from 0441b2a to 45c6022 Compare March 23, 2020 16:54

benjaminapetersen reviewed Mar 23, 2020

View reviewed changes

jhadvig force-pushed the customURL branch 2 times, most recently from e573d14 to fb1e57d Compare March 24, 2020 11:41

ironcladlou reviewed Mar 24, 2020

View reviewed changes

jhadvig force-pushed the customURL branch from fb1e57d to b0b8315 Compare March 24, 2020 15:08

deads2k reviewed Mar 25, 2020

View reviewed changes

Add Route for consoles custom hostname

27f2726

jhadvig force-pushed the customURL branch from b0b8315 to 27f2726 Compare March 26, 2020 16:44

jhadvig mentioned this pull request Mar 26, 2020

Custom console route openshift/console-operator#402

Merged

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 31, 2020

openshift-ci-robot assigned bparees Mar 31, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2020

openshift-merge-robot merged commit 585af27 into openshift:master Mar 31, 2020

spadgett mentioned this pull request Jun 24, 2020

How to change/add webconsole URL after installation of OKD4? openshift/origin-web-console#3172

Closed

Add Route for consoles custom hostname #594

Add Route for consoles custom hostname #594

Uh oh!

Conversation

jhadvig commented Mar 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Mar 12, 2020

Uh oh!

jhadvig commented Mar 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhadvig Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 16, 2020

Uh oh!

jhadvig commented Mar 16, 2020

Uh oh!

deads2k commented Mar 16, 2020

Uh oh!

bparees commented Mar 16, 2020

Uh oh!

deads2k commented Mar 16, 2020

Uh oh!

bparees commented Mar 16, 2020

Uh oh!

benjaminapetersen commented Mar 16, 2020

Uh oh!

bparees commented Mar 16, 2020

Uh oh!

jhadvig commented Mar 16, 2020

Uh oh!

jhadvig Mar 13, 2020 •

edited

Loading