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

Fix malformed caddyfile #260

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

florkbr
Copy link
Contributor

@florkbr florkbr commented Feb 7, 2025

Rolling the latest FEO out to stage broke stage. Several apps returning 200;s but with empty content for "fed-mods.json" and all other files. I originally thought the Caddyfile previously delivered worked locally - however Caddy defaults to returning 200's if there is no route "configured" and simply logs noop in the pod logs.

We discovered frontends using Konflux are using task create-frontend-dockerfile which creates the Caddyfile: https://github.com/RedHatInsights/konflux-consoledot-frontend-build/blob/main/tasks/create-frontend-dockerfile/create-frontend-dockerfile.yaml#L74. Note this references frontend-build-container:3bacc0b

frontend-build-container:3bacc0b Caddyfile can be found here:
https://gitlab.cee.redhat.com/insights-platform/frontend-build-container/-/blob/3bacc0b6ae373b84bdc30adf06c4ac32dd82d024/server_config_gen.sh#L22

I've updated our Caddyfile to match what is in frontend-build-container:3bacc0b as closely as possible.

Copy link
Contributor Author

@florkbr florkbr Feb 7, 2025

Choose a reason for hiding this comment

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

I did a local diff of the Caddyfile in this PR versus what's in frontend-build-container:3bacc0b:

➜  frontend-operator git:(fix-malformed-caddyfile) ✗ diff controllers/templates/Caddyfile_from_3bacc0b controllers/templates/Caddyfile
2c2
<     {\$CADDY_TLS_MODE}
---
>     {$CADDY_TLS_MODE}
12c12
<   
---
> 
14c14,15
<     {\$CADDY_TLS_CERT}
---
>     {$CADDY_TLS_CERT}
>     header -Vary
19c20
<         path ${ROUTE_PATH}*
---
>         path {$ROUTE_PATH}*
22c23
<         uri strip_prefix ${ROUTE_PATH}
---
>         uri strip_prefix {$ROUTE_PATH}
24c25
<             root /opt/app-root/src/${DIST_FOLDER}$([ "$IS_PR" == true ] && echo "" || echo "/stable")
---
>             root /opt/app-root/src/dist/stable
31c32
<         path ${BETA_ROUTE_PATH}*
---
>         path {$BETA_ROUTE_PATH}*
34c35
<         uri strip_prefix ${BETA_ROUTE_PATH}
---
>         uri strip_prefix {$BETA_ROUTE_PATH}
36c37
<             root /opt/app-root/src/${DIST_FOLDER}$([ "$IS_PR" == true ] && echo "" || echo "/preview")
---
>             root /opt/app-root/src/dist/preview
43c44
<         path ${PREVIEW_ROUTE_PATH}*
---
>         path {$PREVIEW_ROUTE_PATH}*
46c47
<         uri strip_prefix ${PREVIEW_ROUTE_PATH}
---
>         uri strip_prefix {$PREVIEW_ROUTE_PATH}
48c49
<             root /opt/app-root/src/${DIST_FOLDER}$([ "$IS_PR" == true ] && echo "" || echo "/preview")
---
>             root /opt/app-root/src/dist/preview

As far as I can tell - only chrome specifies anything other than dist for the DIST_FOLDER (and we currently have an exception for chrome in the reconciler). See https://github.com/search?q=org%3ARedhatInsights+path%3A**%2Fbuild_deploy.sh+DIST_FOLDER&type=code&ref=advsearch

I was unclear on how to map $IS_PR to the FEO environment.

There does appear to be a few frontends that specify a custom Caddyfile. See https://github.com/search?q=org%3ARedhatInsights+path%3A**%2FCaddyfile&type=code&ref=advsearch

I can remove the beta/preview paths - but figured it was best to stay consistent with what we have today and remove later.

@florkbr
Copy link
Contributor Author

florkbr commented Feb 7, 2025

Confirmed locally for apps other than chrome files are now being surfaced. For example landing-page-frontend:
image

@florkbr florkbr requested a review from adamrdrew February 7, 2025 03:56
@florkbr florkbr marked this pull request as ready for review February 7, 2025 19:13
Copy link
Collaborator

@adamrdrew adamrdrew left a comment

Choose a reason for hiding this comment

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

This all looks good to me

@adamrdrew adamrdrew merged commit 530924c into RedHatInsights:main Feb 7, 2025
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants