ingress: Add transition-ingress-from-beta-to-stable enhancement #697
Conversation
| Mitigations for the purpose of these alerts): | ||
|
|
||
| * An alert for Routes that were created from Ingresses that OpenShift is no longer managing. | ||
| * An alert for Ingresses older than 1 day that do not specify `spec.ingressClassName`. |
There was a problem hiding this comment.
Would the IngressClass controller publish a metric detailing how many Ingresses older than 1 day that do not specify spec.ingressClassName exist in the cluster? Im wondering if maybe it would make more sense to report how many Ingresses exist without spec.ingressClassName via a metric, and write the alerting rule to fire after the new metric returns a non-zero value for 24 hours.
There was a problem hiding this comment.
I don't know what the use case for leaving spec.ingressClassName is, but I'm assuming one exists, or else the field would be required for new ingresses. My thinking was that the use case probably involved a developer creating an incomplete ingress and then updating it to make it complete, in which case a high-tenancy cluster with a lot of churn could continuously have some incomplete ingress at any point in time over a 1-day period, so it makes more sense to alert when a specific ingress has been incomplete for >1 day. This thinking could be entirely wrong though, and I don't really know why spec.ingressClassName is optional.
| OpenShift no longer manages. This metric is used in one of the alerting rules | ||
| described below. | ||
|
|
||
| Two alerting rules are added to cluster-monitoring-operator (see Risks and |
There was a problem hiding this comment.
nit: Would the ingress operator add these alert rules to a cluster via it's manifests, as opposed to the alert living in the CMO?
There was a problem hiding this comment.
Oh, I forgot we could add rules in https://github.com/openshift/cluster-ingress-operator/blob/master/manifests/0000_90_ingress-operator_03_prometheusrules.yaml. Yeah, let's do that.
| the administrator has the option of creating a custom IngressClass and | ||
| annotating it with `ingressclass.kubernetes.io/is-default-class=true`. | ||
|
|
||
| Finally, it is possible that a user could have created an Ingress, _did_ specify |
There was a problem hiding this comment.
| Finally, it is possible that a user could have created an Ingress, _did_ specify | |
| Finally, it is possible that a user could have created an Ingress that _did_ specify |
There was a problem hiding this comment.
I think this is what you mean?
There was a problem hiding this comment.
I mean it as "could have created [...], [and] did specify [...], and nevertheless intended [...]". I can try to rewrite it to be clearer.
Miciah
force-pushed
the
ingress-add-transition-ingress-from-beta-to-stable-enhancement
branch
from
0b69b17 to
16b28fc
Compare
| These new features include the `spec.pathType` and `spec.ingressClassType` | ||
| fields as well as the associated IngressClass API. This enhancement does *not* | ||
| extend the Route API to accommodate new features in the Ingress API; these are | ||
| only supported to the extent that they are compatible with the Route API. |
There was a problem hiding this comment.
Does "these" in L48 refer to the "new features in the Ingress API" ? If so, it would be useful to have a table of those that are compatible with the existing Route API and those that are not. This would help describe goal 1 below better.
There was a problem hiding this comment.
It does. I'll add a table. Thanks for the suggestion!
|
|
||
| ### Goals | ||
|
|
||
| 1. Enable users to use new Ingress v1 API features where feasible. |
There was a problem hiding this comment.
Which features are actually expected to be feasible? If we are not able to enumerate those features, it is best to explicitly point out the ones we tested and will support.
There was a problem hiding this comment.
For new features, it's pretty much just spec.ingressClassName. I'll remove this list item because it is vague and doesn't add anything.
| Additionally, the ingress-to-route controller is extended to check the value of | ||
| `spec.rules[*].http.paths[*].pathType` on Ingresses. If an Ingress rule | ||
| specifies the value "Exact" for this field, the ingress-to-route controller | ||
| ignores the rule. |
There was a problem hiding this comment.
To me it seems counterintuitive that "Exact" means to ignore the rule. Is this just one of the infeasible features?
There was a problem hiding this comment.
Right, it is infeasible because the Route API does not have a corresponding way to express "match this exact path" (as opposed to "match this prefix").
| IngressClass already has that annotation. Note that the operator does not add | ||
| the annotation when reconciling already created IngressClass objects as the | ||
| administrator may intentionally have configured the cluster to have no default | ||
| IngressClass. |
There was a problem hiding this comment.
Perhaps there should be no automatic "is-default-class" annotation if we want to allow the cluster to have no default. If the admin may intentionally want no default, it doesn't make sense for them to have to go and remove it after we add it.
There was a problem hiding this comment.
I don't have a clear understanding of why an administrator wouldn't want a default ingressclass; I just didn't want to preclude the possibility, so I added this description of how to configure it. Having no default ingressclass means that creating an ingress without specifying an ingressclass does not expose the ingress, which is a change in behavior from before this enhancement. Having the default ingresscontroller's ingressclass be the default one eliminates a configuration step for administrators who want it to be default, which I expect is the most common and least confusing configuration for administrators and users.
|
|
||
| As follow-up work, we are considering modifying the ingress operator to list all | ||
| Ingresses and Routes in the cluster and publish a metric for Routes that were | ||
| created for Ingresses that OpenShift no longer manages. This metric could used |
There was a problem hiding this comment.
"could used" -> "could be used"
|
|
||
| An alternative to exposing Ingresses that *do* specify `spec.ingressClassName` | ||
| would be not to expose them (i.e., delete existing associated Routes). This | ||
| alternative would pose an unknown but likely moderate risk to high of breaking |
There was a problem hiding this comment.
"moderate risk to high" -> "moderate to high risk" ?
Miciah
force-pushed
the
ingress-add-transition-ingress-from-beta-to-stable-enhancement
branch
from
16b28fc to
8cb2414
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knobunc 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-robot
added
the
approved
|
Looks good. /lgtm |
This enhancement updates the ingress-to-route controller to use the stable
networking.k8s.io/v1version of the Ingress API. Although the API server already supports the v1 API version without this enhancement, the ingress-to-route controller requires changes to transition from using the v1beta1 client to using the v1 client and to support new v1 features. These new features include thespec.pathTypeandspec.ingressClassTypefields as well as the associated IngressClass API. This enhancement does not extend the Route API to accommodate new features in the Ingress API; these are only supported to the extent that they are compatible with the Route API.Implementation PRs:
TODO openshift/cluster-monitoring-operator PR for alerts.