OCPBUGS-33787: add console and download URLs to console operator config #1887
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@alebedev87: This pull request references NE-1319 which is a valid jira issue. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @alebedev87! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/M
alebedev87
force-pushed
the
console-base-address
branch
2 times, most recently
from
4e55540 to
94ed45c
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
alebedev87
changed the title
|
@alebedev87: This pull request references NE-1319 which is a valid jira issue. In 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 openshift-eng/jira-lifecycle-plugin repository. |
alebedev87
force-pushed
the
console-base-address
branch
2 times, most recently
from
f6dda12 to
b48b8c8
Compare
alebedev87
force-pushed
the
console-base-address
branch
2 times, most recently
from
7873d5a to
cf9aa50
Compare
|
@alebedev87: This pull request references NE-1319 which is a valid jira issue. In 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 openshift-eng/jira-lifecycle-plugin repository. |
alebedev87
force-pushed
the
console-base-address
branch
from
cf9aa50 to
1f30016
Compare
|
All the latest commits were to fix |
operator/v1/types_console.go
Outdated
| @@ -146,6 +146,14 @@ type ConsoleCustomization struct { | |||
| // +listMapKey=id | |||
| // +optional | |||
| Perspectives []Perspective `json:"perspectives"` | |||
| // consoleURL is an URL to be used as the base console address. | |||
| // If not specified, the console route hostname will be used. | |||
| // This is required when using console on clusters where the ingress capability is disabled. | |||
There was a problem hiding this comment.
What happens when you try to use this value when ingress is not disabled?
There was a problem hiding this comment.
That part should be programatically conditioned by the console-operator
There was a problem hiding this comment.
OK, but what does the operator do? Is the value ignored?
There was a problem hiding this comment.
The only use case for the moment is when the ingress is disabled. The comment needs to be updated to highlight this. I think the implementation may be expanded in the future as use cases come up.
There was a problem hiding this comment.
Field comment updated.
alebedev87
force-pushed
the
console-base-address
branch
3 times, most recently
from
ecb5e19 to
c06c91d
Compare
|
Field for the download ingress is added. |
|
/retitle NE-1319: add console and download URLs to console operator config |
alebedev87
force-pushed
the
console-base-address
branch
from
9b54a66 to
6b44a2b
Compare
operator/v1/types_console.go
Outdated
| // +kubebuilder:validation:XValidation:rule="isURL(self)",message="console url must be a valid absolute URL" | ||
| // +kubebuilder:validation:XValidation:rule="url(self).getScheme() == 'https'",message="console url scheme must be https" | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| ConsoleURL string `json:"consoleURL,omitempty"` |
There was a problem hiding this comment.
Since these two URLs (and the object itself) are part of a config API, we tend to prefer to get rid of the omitempty.
If you were to do that, you would have to prefix the CEL to avoid running the validation when the string has length 0.
Can you give this a go?
The benefit here is that there's improved visibility and end users will see the fields in the rendered config as empty values, so they can see that the option exists and maybe then want to populate it
There was a problem hiding this comment.
The benefit here is that there's improved visibility and end users will see the fields in the rendered config as empty values, so they can see that the option exists and maybe then want to populate it
What's wrong with the good old oc explain?
There was a problem hiding this comment.
Sometimes a user may have to go through several levels of oc explain to understand what options are available. Printing the whole struct out in configuration APIs makes it clear that there are levels below what they might see that they had no idea they might even want to oc explain
There was a problem hiding this comment.
Doesn't this look a little misleading? When the CEL validation wants the value to be an URL but the user sees an empty string because they omitted the field.
There was a problem hiding this comment.
I would argue that users seeing an empty URL as allowed would indicate that this is an empty value that means nothing is going to happen no? It's only required to be a URL when it's length is non-zero I think is a reasonable validation no?
There was a problem hiding this comment.
Done, including spec.ingress.
operator/v1/types_console.go
Outdated
| // This field is required for clusters without ingress capability, | ||
| // where access to routes is not possible. | ||
| // Make sure that appropriate ingress is set up at this URL before assigning the value, | ||
| // otherwise the console operator may go degraded. |
There was a problem hiding this comment.
Feels like there's tension between being required for clusters without Ingress (with no timing notes) and needing a functional target. I understand that the console operator wants a functional consoleURL. And that when Ingress is enabled, it can provide that default console URL itself. But if you're in a cluster without Ingress, you probably want to set consoleURL to point at where you expect the console to be (as soon as you know where that is), without worrying about delaying until you know it's working, right? That way the console operator can check whether it's working for you, and let you know if you have more work to do. Can we reword this (and the similar clientDownloadsURL wording) to something like:
.... This field is required for clusters without Ingress capability, where access to Routes is not possible. The console operator will monitor the target and may set its ClusterOperator Degraded=True if the target is unreachable for sufficiently long to represent a lower quality of service.
which lifts some "lower quality of service" wording from the Degraded Godocs. Alternatively, just leave it implicit that the console operator is on top of this, and stop after "where access to routes is not possible."?
There was a problem hiding this comment.
I see some discussion of this wording around here (sorry, I hadn't gone through Outdated but still unresolved threads before posting 😅), so feel free to ignore if I'm just missing context that's already been hashed out.
There was a problem hiding this comment.
But if you're in a cluster without Ingress, you probably want to set consoleURL to point at where you expect the console to be (as soon as you know where that is), without worrying about delaying until you know it's working, right? That way the console operator can check whether it's working for you, and let you know if you have more work to do
Good point. Indeed, the admin cannot be sure the URL is working until it gives the custom URL to the console operator. Since the operator has to update the internal and public configs before the full flow can work. Let me think of how I can reflect this in the description.
There was a problem hiding this comment.
Rephrased to:
// consoleURL is a URL to be used as the base console address.
// If not specified, the console route hostname will be used.
// This field is required for clusters without ingress capability,
// where access to routes is not possible.
// Make sure that appropriate ingress is set up at this URL.
// The console operator will monitor the URL and may go degraded
// if it's unreachable for an extended period.
// Must use the HTTPS scheme.
There was a problem hiding this comment.
I don't have the ability to mark this thread resolved myself, but the c0622aa Godocs look good to me 🙇
alebedev87
force-pushed
the
console-base-address
branch
from
6b44a2b to
5be9bdd
Compare
|
/test verify-client-go |
alebedev87
force-pushed
the
console-base-address
branch
from
5be9bdd to
570187d
Compare
|
Rebased from |
|
@alebedev87 whats the golang version on your machine ? |
|
Working on client-go issue separately, it's not a PR issue |
alebedev87
force-pushed
the
console-base-address
branch
from
570187d to
c0622aa
Compare
|
|
|
/test verify-client-go |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, jhadvig, JoelSpeed 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
|
@alebedev87: 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-sigs/prow repository. I understand the commands that are listed here. |
|
@alebedev87: Jira Issue OCPBUGS-33787: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged:
These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-33787 has not been moved to the MODIFIED state. In 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[ART PR BUILD NOTIFIER] This PR has been included in build ose-cluster-config-api-container-v4.17.0-202405242112.p0.gd899f88.assembly.stream.el9 for distgit ose-cluster-config-api. |
|
/cherry-pick release-4.16 |
|
@alebedev87: new pull request created: #1915 In 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-sigs/prow repository. |
|
Fix included in accepted release 4.16.0-0.nightly-2024-07-04-015518 |
This PR adds new fields to the console operator configuration:
consoleURL,clientDownloadsURL. They allow the user to set an alternative console and client download base addresses (routes being the default). This API aims at helping to tolerate the absence of ingress capability on HyperShift managed clusters.