OTA-423: Validate route host before creating app#135
Conversation
openshift-ci
Bot
added
the
approved
jottofar
force-pushed
the
ota-423
branch
4 times, most recently
from
aadbab9 to
d80f839
Compare
|
/test operator-e2e |
jottofar
force-pushed
the
ota-423
branch
4 times, most recently
from
a0fb3a9 to
33ec382
Compare
|
|
||
| func validateRouteName(instance *cv1.UpdateService, name string, namespace string) error { | ||
| var errReasons []string | ||
| routeName := namePolicyEngineRoute(instance) + "-" + namespace |
There was a problem hiding this comment.
Some kind of // hack: copying the logic from https://github.com/openshift/openshift-apiserver/blame/d7b5d2432130ae600c4ab6e047d8a7ce29698a56/pkg/route/apiserver/simplerouteallocation/plugin.go#L50-L54 or some such, so if/when this goes stale, folks know where to look to see how it should be updated.
It's going to be a pain to match our operator-side validation with a potentially evolving default route plugin. If we are going to grow opinions like this, perhaps we should set spec.subdomain to decouple ourselves somewhat from the underlying route controller's logic?
There was a problem hiding this comment.
I don't feel strongly that we need to have this precheck so if you and/or Lala are okay with dropping it so am I. With the shortened route name change in here I wouldn't think we'll encounter the >63 issue very often if at all.
So it's the cost of maintenance vs. catching the error before the operand is created and thereby eliminating the need for the user to have to delete the operand after route creation failure.
There was a problem hiding this comment.
So it's the cost of maintenance vs. catching the error before the operand is created and thereby eliminating the need for the user to have to delete the operand after route creation failure.
Hmm... Do they even need to delete the operand? I'd expect them to have to delete the UpdateService and create a replacement with a shorter name (unless you're allowed to patch resource names?). And when the old UpdateService goes away, I'd expect our operator to notice and remove the operand and other associated resources we'd been using to fulfill the outgoing UpdateService's request.
There was a problem hiding this comment.
If this check fails nothing is created so if they could try again with a shorter operand name.
There was a problem hiding this comment.
@jottofar AFAIU this check will consider the name given to the route and then validate if the name falls within the 63 character length. To do that it needs to take the namespace length in to account because that's how the route name length is calculated. Is that right?
There was a problem hiding this comment.
Correct. Namespace is part of the route name and is out of our control and something that could change. For example, if we created the operand named test in namespace openshift-update-service the route name, before this change, would be test-policy-engine-route-openshift-update-service and after change would be test-route-openshift-update-service.
There was a problem hiding this comment.
Confirming via a 4.9.0 OpenShift Update Service operator in a 4.10.0-0.ci-2021-11-04-133306 cluster, nobody can change the name on the UpdateService (or other resources?):
So the repair for "the name you picked is too long" will be "notice, delete the UpdateService, and create a replacement with a shorter name". I don't think it's worth jumping through hoops to guess at Route validity, because the upside would be shaving a minute or so off that notice cycle. And the downside is that we need this inlined copy making assumptions about how the cluster will be generating domain names for Routes.
There was a problem hiding this comment.
I'm fine with dropping the validation piece. It's not how I originally envisioned it anyway. I'll just change this to only include the name shortening change which should reduce/eliminate issue occurrence.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jottofar, LalatenduMohanty The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/operator-e2e Test is failing on all recent (~few weeks) cincinnati-operator PRs due to OSUS operand not becoming Ready. Not related to this change and I've tested locally. |
|
@jottofar: Overrode contexts on behalf of jottofar: ci/prow/operator-e2e DetailsIn response to this:
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. |
Catching up with openshift/cincinnati-operator@8cea4f3383 (OTA-423: Validate route host before creating app, 2021-10-18, openshift/cincinnati-operator#135), which dropped the '-policy-engine' scoping.
4 participants
No description provided.