Ability to Customize HAProxy 2.x Error Page #680
Conversation
miheer
force-pushed
the
customize-pages
branch
6 times, most recently
from
b70c0e1
to
9105125
Compare
|
@Miciah PTAL |
| Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711). | ||
| does not work with OCP 4. It's also important that the right page as per user requirements is served at the right moment. | ||
| Example - 503 error page shall be returned if no pod is available. | ||
| But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements. |
There was a problem hiding this comment.
But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements.
How does this enhancement accomplish this? Can multiple error pages (ie one HTML page for 503, and one HTML page for 404) be appended to the same error pages config map (and thus fed into HAProxy)? I would suggest elaborating on how a user specifics multiple error pages somewhere in this enhancement (it's not entirely clear to me).
There was a problem hiding this comment.
configmap can have multiple pages
| ## Summary | ||
| There is no supported and reasonable method to customize the HAProxy 2.x error page as deployed in OpenShift. | ||
| Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711). | ||
| does not work with OCP 4. It's also important that the right page as per user requirements is served at the right moment. |
There was a problem hiding this comment.
Customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711). does not work with OCP 4
This solution was never intended to work on OCP 4, right? Also, I would suggest removing the spurious period after the parenthesis.
There was a problem hiding this comment.
I will reword it. Yes it is not meant for OCP 4. Customers say 503 page is served even when the page is not found when it shall serve 404. The reason I think that happens is because we provide only 503 in our haproxy template
| But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements. | ||
| ## Motivation | ||
| In OCP 3.11 customizing error pages is possible via a [documented procedure](https://access.redhat.com/solutions/3425711) | ||
| Customers resist customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711) |
There was a problem hiding this comment.
Customers resist customizing the HAProxy error page per our recommended procedure (documented here: https://access.redhat.com/solutions/3425711)
Are you saying that customers in OCP 3.11 resist following the recommended procedure? If so, why? Or are you saying customers on OCP 4.x resist following this procedure for updating the HAProxy error pages (simply because it does not work)?
There was a problem hiding this comment.
Actually that's a different thing. Customers say 503 page is served even when the page is not found when it shall serve 404. The reason I think that happens is because we provide only 503 in our haproxy template
There was a problem hiding this comment.
I will make it clear
| 1. The IngressController API is extended by adding an optional | ||
| `HttpErrorCodePage` field with type string | ||
| ```go | ||
| //This used to sync the configmap having customizable error code page created by cluster admin in openshift-config namespace. |
There was a problem hiding this comment.
In general I would suggest using a single space after each comment indicator, ie // <text> instead of //<text>.
| ``` | ||
| 2. `errorfile` stanzas need to be added as per https://www.haproxy.com/blog/serve-dynamic-custom-error-pages-with-haproxy/ | ||
| 3. default error pages will be added in openshift/router for the haproxy. | ||
| 4. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config` |
There was a problem hiding this comment.
Is syncing the config map from the openshift-config namespace to the openshift-ingress namespace necessary? Is there a reason we can't just use the config map in the original namespace directly?
There was a problem hiding this comment.
This is the standard pattern for operators: User-created configmaps and secrets go in openshift-config, operator-managed shared configmaps and secrets go in openshift-config-managed, and operators synch configmaps and secrets from those namespaces to their own namespaces as needed. This eases auditing and provides more control over access, validation, and updates. Unfortunately we didn't follow this pattern in the ingress operator from the start with some resources, but it is the pattern we're trying to follow in all operators.
There was a problem hiding this comment.
Ah I see, thanks for the historical explanation. Thanks!
| 3. default error pages will be added in openshift/router for the haproxy. | ||
| 4. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config` | ||
| to `openshift-ingress`. | ||
| 5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go`. |
There was a problem hiding this comment.
| 5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go`. | |
| 5. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go` |
|
|
||
| ### Validation | ||
| Omitting `spec.httpErrorCodePage` specifies the default behavior i.e serves default error code pages. | ||
| If user specifies an invalid value, then the IngressController ignores the annotation. |
There was a problem hiding this comment.
If user specifies an invalid value, then the IngressController ignores the annotation.
What constitutes an invalid value? Which annotation are you referring to here?
There was a problem hiding this comment.
In particular, what keys does the configmap need to have? What is the format of the data values? For example, does an error page need to include the HTTP header? (Presumably it should.) Does the page need to have an HTML document? (I expect text/plain would work.) If HTML, is it validated? (I don't think we should validate HTML, but we should call out the design decision.) Is there a max length? Does encoding matter?
There was a problem hiding this comment.
Not yet decided on the max length of the page. Need to think on encoding.
| router pods. | ||
|
|
||
| ### Risks and Mitigations | ||
| No risk identified. This is a cosmetic change. |
There was a problem hiding this comment.
I think there are a couple risks worth mentioning.
- What if a user supplies an incomplete or confusing error page(s)? Or what if a user provides an absolutely massive HTML file for a trivial 503? Could that effect performance somehow?
- Are there any applications that use the default ingress controller that expect a 503 in non-503 cases? Does the administrator need to exercise caution when creating custom error pages for the default ingress controller? (I would hope not, but it might be worth double checking)
There was a problem hiding this comment.
If the user does not provide proper file name in configmap then the custom error page will not be served. The default shall be served in that case
Yeah admin will need to take some precaution. I will list them. May be we can do some validations at our end. Currently if page is not found then also 503 is served. This I think is because we have only 503 page defined in errorfile section of haproxy template of the router.
| Unit test cases will be added for the new controller which sync the configmap from openshift-config | ||
| to openshift-ingress. | ||
| Unit test cases will be added for the ingress controller where the router deployment get a volume mount | ||
| of the custom error code page |
There was a problem hiding this comment.
I would suggestion adding some details about a potential e2e test for this enhancement.
| namespace: openshift-ingress-operator | ||
| spec: | ||
| httpErrorCodePage: "http503page" // http503page is the config map name with custom | ||
| //503 error page created in openshift-config by cluster-admin. |
There was a problem hiding this comment.
Yaml comments start with #. But instead of comments in the yaml, please provide an example definition for the "http503page" configmap.
miheer
force-pushed
the
customize-pages
branch
2 times, most recently
from
8e69340
to
99fbb67
Compare
|
/lgtm |
|
/lgtm cancel |
miheer
force-pushed
the
customize-pages
branch
3 times, most recently
from
1010de8
to
e18070c
Compare
| There is no supported and reasonable method to customize the HAProxy 2.x error page in OCP 4.x | ||
| It's also important that the right page as per user requirements is served at the right moment. | ||
| Example - 503 error page shall be returned if no pod is available. | ||
| But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements. |
There was a problem hiding this comment.
| There is no supported and reasonable method to customize the HAProxy 2.x error page in OCP 4.x | |
| It's also important that the right page as per user requirements is served at the right moment. | |
| Example - 503 error page shall be returned if no pod is available. | |
| But when the URI is wrong/does not exist, we should return 404 or a customized 404 page as per user requirements. | |
| There is no supported method to customize an IngressController's error pages in OCP 4. | |
| Users may want to customize an IngressController's error pages for branding or other reasons. | |
| For example, users may want a custom HTTP 503 error page to be returned if no pod is available. | |
| When the requested URI does not exist, users may want an IngressController to return a custom 404 page. |
| Using this feature, user can provide custom http pages for 503 and 404 for the haproxy router by adding these pages | ||
| in a configmap using --file attribute. |
There was a problem hiding this comment.
What is the "--file attribute"? I'm guessing the phrase can be deleted.
| Using this feature, user can provide custom http pages for 503 and 404 for the haproxy router by adding these pages | |
| in a configmap using --file attribute. | |
| Using this feature, users can provide custom HTTP pages for HTTP 503 and 404 responses using a configmap. |
| Users say 503 page is served even when the page is not found when it shall serve 404. | ||
| The reason that happens is because we provide only 503 in our haproxy template. |
There was a problem hiding this comment.
I think this a secondary motivation for this enhancement. The primary motivation is that users want to customize the error pages that are returned, for example for branding purposes. Please mention both motivations.
| The reason that happens is because we provide only 503 in our haproxy template. | ||
|
|
||
| ### Goals | ||
| 1. Enable the cluster administrator to specify custom error code pages for haproxy router to serve them at the right moment. |
There was a problem hiding this comment.
From the motivation and summary, it seems you can also add this goal: "Return a distinct HTTP 404 error page when the requested URI does not exist, instead of returning the HTTP 503 error page."
I think you can delete the phrase "at the right moment". If the phrase refers to returning 404s, then that should be a separate goal. If the phrase means something else, please clarify.
| 1. Enable the cluster administrator to specify custom error code pages for haproxy router to serve them at the right moment. | ||
|
|
||
| ### Non-Goal | ||
| TBD |
There was a problem hiding this comment.
- Custom error pages for HTTP responses other than 503 and 404.
- Enabling users to configure custom error pages for individual routes.
| 1. If a user provides incorrect filename than specified format i.e error-page-<error code>.http in | ||
| the keys of the configmap then it will be ignored and default page will be served. |
There was a problem hiding this comment.
| 1. If a user provides incorrect filename than specified format i.e error-page-<error code>.http in | |
| the keys of the configmap then it will be ignored and default page will be served. | |
| 1. If a user provides a key that is not of the format `error-page-<error code>.http` in | |
| the configmap, then the configmap will be ignored, and default error pages will be served. |
| Unit test cases will be added for the new controller which sync the configmap from openshift-config | ||
| to openshift-ingress. |
There was a problem hiding this comment.
| Unit test cases will be added for the new controller which sync the configmap from openshift-config | |
| to openshift-ingress. | |
| Unit test cases will be added for the new controller which sync the configmap from `openshift-config` | |
| to `openshift-ingress`. |
| 1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created | ||
| in openshift-config namespace which is then synced to openshift-ingress namespace. |
There was a problem hiding this comment.
| 1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created | |
| in openshift-config namespace which is then synced to openshift-ingress namespace. | |
| 1. https://github.com/openshift/api/pull/843 - Adds a field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created | |
| in the `openshift-config` namespace which is then synced to `openshift-ingress` namespace. |
| 1. https://github.com/openshift/api/pull/843 - Adds an field called `httpErrorCodePages` to the ingress controller API to get the custom error page configmap created | ||
| in openshift-config namespace which is then synced to openshift-ingress namespace. | ||
| 2. https://github.com/openshift/cluster-ingress-operator/pull/571 | ||
| a. Controller to sync configmaps from openshift config namespace to openshift-ingress namespace NE-535 |
There was a problem hiding this comment.
| a. Controller to sync configmaps from openshift config namespace to openshift-ingress namespace NE-535 | |
| a. Controller to sync configmaps from the `openshift-config` namespace to the `openshift-ingress` namespace ([NE-535](https://issues.redhat.com/browse/NE-535)). |
Is NE-535 specifically for this part of the feature?
| // Currently only error pages for 503 and 404 responses can be customized. | ||
| // Each value in the configmap should be the full response, including HTTP headers. | ||
| // If this field is empty, the ingress controller uses the default error pages. | ||
| HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"` |
There was a problem hiding this comment.
Shouldn't this be the configmap reference type?
| HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"` | |
| HttpErrorCodePages configv1.ConfigMapNameReference `json:"httpErrorCodePages,omitempty"` |
miheer
force-pushed
the
customize-pages
branch
7 times, most recently
from
26a17ee
to
d794df2
Compare
|
@Miciah PTAL |
There was a problem hiding this comment.
There are still some unaddressed issues. However, we need to unblock the openshift/api PR. Can you do https://github.com/openshift/enhancements/pull/680/files#r601968021 for now? We can get the rest in follow-ups.
| The reason that happens is because we provide only 503 in our haproxy template. | ||
|
|
||
| ### Goals | ||
| Return a distinct HTTP 404 error page when the requested URI does not exist, instead of returning the HTTP 503 error page. |
There was a problem hiding this comment.
Looks like you accidentally deleted the old goal: "Enable the cluster administrator to specify custom error code pages for haproxy router".
| Custom error pages for HTTP responses other than 503 and 404. | ||
| Enabling users to configure custom error pages for individual routes. |
| 2. The configmap keys shall have the format error-page-`<error code>`.http. Right now only error-page-503.http and | ||
| error-page-404.http will be available. |
There was a problem hiding this comment.
| 2. The configmap keys shall have the format error-page-`<error code>`.http. Right now only error-page-503.http and | |
| error-page-404.http will be available. | |
| 2. The configmap keys shall have the format `error-page-<error code>.http`. Right now only `error-page-503.http` and | |
| `error-page-404.http` will be recognized. |
| $ oc -n openshift-config create configmap my-custom-error-code-pages\ | ||
| --from-file=error-page-503.http\ | ||
| --from-file=error-page-404.http |
There was a problem hiding this comment.
| $ oc -n openshift-config create configmap my-custom-error-code-pages\ | |
| --from-file=error-page-503.http\ | |
| --from-file=error-page-404.http | |
| $ oc -n openshift-config create configmap my-custom-error-code-pages \ | |
| --from-file=error-page-503.http \ | |
| --from-file=error-page-404.http |
Or unwrap it:
| $ oc -n openshift-config create configmap my-custom-error-code-pages\ | |
| --from-file=error-page-503.http\ | |
| --from-file=error-page-404.http | |
| $ oc -n openshift-config create configmap my-custom-error-code-pages --from-file=error-page-503.http --from-file=error-page-404.http |
| error-page-503.http: |- | ||
| <your custom http response page contents> | ||
| error-page-404.http: | | ||
| <your custom http response page contents> |
There was a problem hiding this comment.
This still needs to be done.
| // If this field is empty, the ingress controller uses the default error pages. | ||
| HttpErrorCodePages string `json:"httpErrorCodePages,omitempty"` | ||
| ``` | ||
| Using the default, the router uses the default error pages used by haproxy. |
There was a problem hiding this comment.
This still needs to be fixed.
|
|
||
| ``` | ||
| 3. `errorfile` stanzas need to be added as per https://www.haproxy.com/blog/serve-dynamic-custom-error-pages-with-haproxy/ | ||
| 4. default error pages will be added in openshift/router for the haproxy. |
There was a problem hiding this comment.
This still needs to be addressed. I think you mean to say the following:
| 4. default error pages will be added in openshift/router for the haproxy. | |
| 4. A default error page for HTTP 404 responses will be added in openshift/router. |
| 4. default error pages will be added in openshift/router for the haproxy. | ||
| 5. A new controller to sync the cluster admin created configmap having custom error pages from `openshift-config` | ||
| to `openshift-ingress`. | ||
| 6. changes to ingress controller's function `desiredRouterDeployment` present in `deployment.go` to mount the configmap to the router. |
There was a problem hiding this comment.
This still should be fixed.
| ### User Stories | ||
|
|
||
| #### As a cluster administrator, I expect to have custom error code pages getting served | ||
| 1. The administrator creates a configmap named http503page in the openshift-config namespace. |
There was a problem hiding this comment.
This still isn't addressed.
| 3. The ingress operator copies the http503page configmap from the openshift-config namespace to the | ||
| openshift-ingress namespace. |
There was a problem hiding this comment.
This still isn't addressed.
miheer
force-pushed
the
customize-pages
branch
2 times, most recently
from
e77e05d
to
90b9473
Compare
|
Thanks! The enhancement looks well defined in its current form, modulo some markup issues and such. We can clean up the remaining issues later, but let's get this PR merged to unblock openshift/api#843. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah 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
|
Thanks! Maybe the linter will tolerate the simplified example... |
Ability to Customize HAProxy 2.x Error Page
https://issues.redhat.com/browse/NE-379