Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SS-1141 Added info on how to setup the custom backend for the errors on k8s level #62

Merged
merged 2 commits into from
Nov 27, 2024

Conversation

churnikov
Copy link
Contributor

@churnikov churnikov commented Nov 19, 2024

For now you can try out 404 page like this:

https://rstarst.serve-dev.scilifelab.se/

…evel

Changed ingress configuration for the apps, so that they would redirect to our 503 page in case app not working
Up apps charts versions
@churnikov churnikov requested review from hamzaimran08 and a team November 19, 2024 13:27
@churnikov churnikov self-assigned this Nov 19, 2024
@lindhe
Copy link

lindhe commented Nov 19, 2024

Thanks for the ping, I'll have a look soon.

Copy link

@lindhe lindhe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotations looks in order, as far as I can tell. But I would suggest you review the documented way-of-working for deploying this. As you see in my comments, I think much of this deserves to be files in the repo which are applied via kubectl. That will make it much easier for you to version control any future code changes and also makes the deployment process less error prone.

Do you have Argo CD for deploying manifests? If so, I would probably suggest configuring that instead of applying this manually.

README.md Outdated
<details>
<summary>Click to see the content of the custom-error-backend.yaml file</summary>

```yaml
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the indentation is incorrect for this code fencing. Does not look right when looking at the rendered README.

README.md Outdated

Configuration for the default backend: service and handling of a 404 error on a subdomain level (wild card domain).

Create `custom-error-backent.yaml` file with the following content.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Typo: -backent
  2. Totally up to you, but personally I would prefer to have custom-error-backend.yaml as a file, not just in the README. It's so long (and important), it deserves to be a file I think.

README.md Outdated
$ kubectl apply -f custom-error-backend.yaml
```

In the rancher dashboard change the values for configmaps `custom-error-pages-404` and `custom-error-pages-503` to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advice against using Rancher's dashboard features to edit ConfigMaps. Not that it would not work, just that it's very easy to make mistakes. Instead, I suggest you create the ConfigMaps using kubectl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying my best to set it up via config map, but it kept giving me errors like this for:: invalid JSON document or unicode errors.

Rancher's forms were able to handle this, so I went with them

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be possible to do like this:

kubectl create configmap custom-error-pages --from-file 404=./html/404.html --from-file 503=./html/503.html

The result should look similar to this: https://github.com/kubernetes/ingress-nginx/blob/6ceccbd67b140b7626670ad17f926f121a9e5563/docs/examples/customization/custom-errors/custom-default-backend-error_pages.configMap.yaml

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were apparently a misconfiguration in Rancher causing this to break. It's fixed now.

README.md Outdated
Comment on lines 167 to 169
Take `404.html` file from [here](error-page-404.html)

Take `503.html` file from [here](error-page-503.html)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Take `404.html` file from [here](error-page-404.html)
Take `503.html` file from [here](error-page-503.html)
You find the error pages in [error-page-404.html](error-page-404.html) and [error-page-503.html](error-page-503.html).

@@ -10,6 +10,8 @@ metadata:
nginx.ingress.kubernetes.io/auth-url: "{{ .Values.global.protocol }}://{{ .Values.global.auth_domain }}:8080/auth/?release={{ .Values.release }}"
nginx.ingress.kubernetes.io/auth-signin: "https://{{ .Values.global.domain }}/accounts/login/?next=$scheme%3A%2F%2F$host"
{{- end }}
nginx.ingress.kubernetes.io/custom-http-errors: "503"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the README, you give instructions for configuring both 404 and 503, yet here you only configure 503. Is that intentional?

Removed info on custom default backend from the README.md file
Added 503 handling for the studio pod
@churnikov churnikov requested a review from lindhe November 26, 2024 13:51
Copy link
Collaborator

@hamzaimran08 hamzaimran08 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@churnikov
Copy link
Contributor Author

Follow up pr ScilifelabDataCentre/serve#260

@churnikov churnikov merged commit 20eacca into develop Nov 27, 2024
1 check passed
@churnikov churnikov deleted the SS-1141-custom-error-pages branch November 27, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants