NE-881: ingress: Add route-subdomain enhancement #1023
Conversation
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
from
b52549f to
c67bb3b
Compare
| routerCanonicalHostname: router-shard1.shard1.apps.mycluster.com | ||
| routerName: shard1 | ||
| # ... | ||
| ``` |
There was a problem hiding this comment.
Technically console wanted this, we didn't deliver it, and as a result the default console DNS name in openshift ootb sucks (console-openshift-console). While we can't necessarily change that, the use case of cluster agnostic (i.e. unaware of the subname) naming is valid (i want to create a name within the route shard's scope) as an infrastructure user.
| modified to configure router deployments to use the IngressController's | ||
| `status.domain` as the router deployment's domain. |
There was a problem hiding this comment.
Didn't quite get how route's spec.subdomain is linked with router's deployment. And what is router deployment's domain? canonical hostname?
There was a problem hiding this comment.
I've added some examples to help clarify in the next push.
| `spec.host` if `spec.subdomain` has a nonempty value. This change enables each | ||
| router deployment to set a host, which is reported in the Route's | ||
| `status.ingress[*].host` field. |
There was a problem hiding this comment.
Not quite clear which host the router will set. Or you mean the status.ingress.host?
There was a problem hiding this comment.
A router named "foo" will set the route's status.ingress["foo"].host field. If it will help, I can add the previous sentence to the enhancement document. Is the [*] syntax confusing, is the wording confusing, or something else?
There was a problem hiding this comment.
It's clear now, I believe that before you didn't mention that the router updates the status so it could have been interpreted as if the router sets .spec.host.
|
As discussed in Slack. |
|
Latest push (1) adds some examples of how routes' hosts would be reflected in status, (2) adds open questions regarding |
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
2 times, most recently
from
5a1dfbb to
43c7ee9
Compare
|
Added a sentence in the summary to clarify that this enhancement does not change the behavior if |
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
from
43c7ee9 to
89db383
Compare
|
/lgtm |
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
from
89db383 to
aea1c73
Compare
Miciah
changed the title
|
|
||
| ### Open Questions | ||
|
|
||
| Do `oc get` or `oc describe` need changes to print Routes' host names when |
There was a problem hiding this comment.
My thought: I don't think so, it already prints status.ingress[*].host, that should just work based on your description here.
| - For a Route that omits both `spec.host` and `spec.subdomain`, the API should set a default value for `spec.host`, and the router should expose the Route using that host name. | ||
| - For a Route that specifies `spec.host`, the router should expose the Route using that host name. | ||
| - For a Route that omits `spec.host` and specifies `spec.subdomain`, the router should leave `spec.host` empty, and the router should expose the Route using a host name composed from the Route's subdomain and the router's domain. | ||
|
|
There was a problem hiding this comment.
I think you missed a case here: "For a route that specifies both spec.host and spec.subdomain..."
There was a problem hiding this comment.
The second case covers that. I'll make it more explicit though.
There was a problem hiding this comment.
Oh I see, my brain went "4 tests and 3 results....hmmm"
|
|
||
| However, per the Route API's existing semantics, a Route object that has already | ||
| had `spec.host` defaulted on an older OpenShift cluster will continue to be | ||
| served using the same host name, and `spec.subdomain` will be ignored; the |
There was a problem hiding this comment.
So we are basically saying setting spec.host and spec.subdomain means spec.subdomain is a no-op. Now I understand the reasoning behind this if we have both set as we want to avoid an unexpected route domain changes, say if you upgrade with an existing spec.subdomain (and the fact that spec.subdomain is the new object being added here).
However, if I'm blind to the woes of openshift upgrade process and I read the structure of the Route CR as a brand new openshift user, my gut would tell me that spec.subdomain should prepend on spec.host. Maybe that's just me, but I guess I'm putting that out there for food for thought. I wouldn't expect spec.subdomain to just be ignored.
In a perfect world with no openshift upgrades, would we still do it like this? Understood documentation is the answer to these problems, but still asking the question if this feels "nature".
There was a problem hiding this comment.
I understand the API is a little confusing. Unfortunately that arises from the necessity to maintain backwards compatibility. Moreover the spec.subdomain field and these semantics have actually been in openshift/api for many releases—we just haven't implemented the documented semantics—so I'm reluctant to try to change the semantics now.
It might be worth exploring the possibility of printing a warning when both spec.host and spec.subdomain are set. I can look into what would be required to do that. Maybe (although I'm more dubious of this possibility) we could even add an admission check that rejects new routes (whilst allowing updates to old routes, for compatibility) that specify both spec.host and spec.subdomain.
There was a problem hiding this comment.
I added a warning in openshift-apiserver; see openshift/openshift-apiserver#254 (comment).
|
|
||
| ### Default `spec.subdomain` instead of defaulting `spec.host` | ||
|
|
||
| A possible modification to this enhancement would be to change the OpenShift API |
There was a problem hiding this comment.
So my hot take is: I think this alternative is the most correct, as it paves a way to a single domain-specifier solution. Here's my arguments:
- As a user, the relationship between
spec.hostandspec.subdomain(without reading the docs), is not natural to me like I mentioned in an comment above. - I just generally think the existing
spec.hostis a little redundant that you have to copy/paste the full domain into the route to match your ingress controller (hope you don't have copy/paste error). I'd think justspec.subdomainis a easier way to configure your domains.
So, I'm still new to enhancements and I'm not out-right disagreeing with this one, but I am curious if there is agreement that there is some discontinuity between the two, and if you really actually need both spec.host and spec.subdomain.
The questions I have:
- In a perfect world, do we agree that we could really get by with just
spec.subdomain? - If we agree above, What's the transition plan to consolidate the two specifiers, if any?
There was a problem hiding this comment.
So my hot take is: I think this alternative is the most correct, as it paves a way to a single domain-specifier solution. Here's my arguments:
1. As a user, the relationship between `spec.host` and `spec.subdomain` (without reading the docs), is not natural to me like I mentioned in an comment above.
Agreed.
2. I just generally think the existing `spec.host` is a little redundant that you have to copy/paste the full domain into the route to match your ingress controller (hope you don't have copy/paste error). I'd think just `spec.subdomain` is a easier way to configure your domains.
Disagree. See below.
So, I'm still new to enhancements and I'm not out-right disagreeing with this one, but I am curious if there is agreement that there is some discontinuity between the two, and if you really actually need both
spec.hostandspec.subdomain.* In a perfect world, do we agree that we could really get by with just `spec.subdomain`?
No, the user may need to specify a host name outside of the ingress controller's domain. It would be up to the user to create a DNS record pointing that custom host name to the ingress controller (typically, the user would use a CNAME record pointing to the routerCanonicalHostname from the route's status), but notwithstanding that, use cases do exist for specifying spec.host instead of spec.subdomain.
The idea with this alternative is really that the user would still be able to specify spec.host or spec.subdomain, but if the user specified neither, the API would default spec.subdomain, rather than defaulting spec.host. The next paragraph describes the tradeoff, which is, in short, that defaulting spec.subdomain instead of defaulting spec.host would be surprising and could break users' long-established workflows.
There was a problem hiding this comment.
Okay I see your point, I was unaware of specific use cases with spec.host, but that makes sense now. I generally agree with this position.
I still think the domain-related API is getting a little confusing, which I think you agree with. I'm curious, if we had a chance to redo it, what would be a more natural solution. That can be a future discussion.
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
from
aea1c73 to
86b80b2
Compare
* enhancements/ingress/route-subdomain.md: New file.
Miciah
force-pushed
the
add-route-subdomain-enhancement
branch
from
86b80b2 to
f3f168f
Compare
|
Latest push adds a tracking link and adds myself to api-approvers to appease the linter (the enhancement implements an existing API, so there's no API approval needed) as well as updating the reviewers list. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frobware The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@Miciah: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This enhancement implements the
spec.subdomainfield of the OpenShift Route API. When this field is specified, the host name of the Route depends on the domain of the IngressController that exposes the Route. If multiple IngressControllers expose a Route that specifiesspec.subdomain, then the Route has a distinct host name for each IngressController that exposes it. This is particularly useful with sharding where the Route is exposed on multiple shards. It is also useful generally in situations where the user wants to specify the subdomain part of the Route's host name but allow the IngressController to set the domain part.enhancements/ingress/route-subdomain.md: New file.WIP implementation:
ROUTER_DOMAINto enable route subdomain field cluster-ingress-operator#674https://gist.github.com/Miciah/5fd6c88b45803dd3ebb55f63f28ca83e shows an example of how this new feature can be used to expose a route with two different routers and domains.