-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor Kubernetes ingress configuration and add HTTP to HTTPS redirection #289
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||||||||
| # k8s/api-ingressroute.yaml | ||||||||||||||
NiveditJain marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Header comment path is incorrect. The file header says “k8s/api-ingressroute.yaml” but the file lives at “k8s/ingress/api-server-ingress-http.yaml”. Apply this diff: -# k8s/api-ingressroute.yaml
+# k8s/ingress/api-server-ingress-http.yaml📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| apiVersion: traefik.io/v1alpha1 | ||||||||||||||
| kind: IngressRoute | ||||||||||||||
| metadata: | ||||||||||||||
| name: exosphere-api-server-http | ||||||||||||||
|
Comment on lines
+4
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Be explicit about namespace to avoid surprises in CI contexts. Unless your workflow sets the default namespace, add kind: IngressRoute
metadata:
name: exosphere-api-server-http
+ namespace: default📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| spec: | ||||||||||||||
| entryPoints: | ||||||||||||||
| - web | ||||||||||||||
| routes: | ||||||||||||||
| - match: Host(`api.exosphere.host`) | ||||||||||||||
| kind: Rule | ||||||||||||||
|
Comment on lines
+7
to
+11
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Inconsistent IngressRoute: HTTP entryPoint with TLS enabled. Choose one of two valid patterns. You’re binding to entryPoint “web” (HTTP) while enabling TLS. TLS will not terminate on a non-TLS entrypoint. Pick one:
Option A (HTTP-only, attach middleware): spec:
entryPoints:
- - web
+ - web
routes:
- match: Host(`api.exosphere.host`)
kind: Rule
+ middlewares:
+ - name: http-to-https-redirect
services:
- name: exosphere-api-server
namespace: default
port: 80
- tls:
- certResolver: letsencryptOption B (HTTPS-only on websecure): spec:
- entryPoints:
- - web
+ entryPoints:
+ - websecure
routes:
- match: Host(`api.exosphere.host`)
kind: Rule
services:
- name: exosphere-api-server
namespace: default
port: 80
tls:
certResolver: letsencryptAlso ensure you actually have a separate HTTP IngressRoute only if you’re not using global redirection. Also applies to: 16-17 🤖 Prompt for AI Agents |
||||||||||||||
| services: | ||||||||||||||
| - name: exosphere-api-server | ||||||||||||||
| namespace: default | ||||||||||||||
| port: 80 | ||||||||||||||
| tls: | ||||||||||||||
| certResolver: letsencrypt | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add missing newline at EOF to satisfy linters. Apply this diff: - certResolver: letsencrypt
\ No newline at end of file
+ certResolver: letsencrypt
+📝 Committable suggestion
Suggested change
🧰 Tools🪛 YAMLlint (1.37.1)[error] 17-17: no new line character at the end of file (new-line-at-end-of-file) 🤖 Prompt for AI Agents |
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,9 @@ | ||||||||||||||||||||||
| apiVersion: traefik.io/v1alpha1 | ||||||||||||||||||||||
| kind: Middleware | ||||||||||||||||||||||
| metadata: | ||||||||||||||||||||||
| name: http-to-https-redirect | ||||||||||||||||||||||
|
Comment on lines
+3
to
+4
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Make middleware namespace explicit. If your IngressRoutes are in “default”, keeping the middleware in the same namespace is fine, but making it explicit avoids surprises when contexts change. Apply this diff: kind: Middleware
metadata:
- name: http-to-https-redirect
+ name: http-to-https-redirect
+ namespace: default📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| spec: | ||||||||||||||||||||||
| redirectScheme: | ||||||||||||||||||||||
| scheme: https | ||||||||||||||||||||||
| port: "443" | ||||||||||||||||||||||
| permanent: true | ||||||||||||||||||||||
NiveditJain marked this conversation as resolved.
Show resolved
Hide resolved
NiveditJain marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+6
to
+9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Trailing space and missing newline; also consider wiring the middleware.
Apply this diff: spec:
redirectScheme:
scheme: https
port: "443"
- permanent: true
+ permanent: true
+📝 Committable suggestion
Suggested change
🧰 Tools🪛 YAMLlint (1.37.1)[error] 9-9: no new line character at the end of file (new-line-at-end-of-file) [error] 9-9: trailing spaces (trailing-spaces) 🤖 Prompt for AI Agents |
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||||||||||
| # k8s/api-ingressroute.yaml | ||||||||||||||
NiveditJain marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Header comment path is incorrect. The file header says “k8s/api-ingressroute.yaml” but the file is “k8s/ingress/landing-page-ingress-http.yaml”. Apply this diff: -# k8s/api-ingressroute.yaml
+# k8s/ingress/landing-page-ingress-http.yaml📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| apiVersion: traefik.io/v1alpha1 | ||||||||||||||
| kind: IngressRoute | ||||||||||||||
| metadata: | ||||||||||||||
| name: exosphere-landing-page-http | ||||||||||||||
|
Comment on lines
+4
to
+5
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Be explicit about namespace to avoid default-namespace assumptions. kind: IngressRoute
metadata:
name: exosphere-landing-page-http
+ namespace: default📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| spec: | ||||||||||||||
| entryPoints: | ||||||||||||||
| - web | ||||||||||||||
| routes: | ||||||||||||||
| - match: Host(`exosphere.host`) | ||||||||||||||
| kind: Rule | ||||||||||||||
|
Comment on lines
+7
to
+11
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Same inconsistency: HTTP entryPoint with TLS enabled. Align with one pattern. Mirror the API ingress fix. Choose one:
Option A: spec:
entryPoints:
- web
routes:
- match: Host(`exosphere.host`)
kind: Rule
+ middlewares:
+ - name: http-to-https-redirect
services:
- name: exosphere-landing-page
namespace: default
port: 80
- tls:
- certResolver: letsencryptOption B: spec:
- entryPoints:
- - web
+ entryPoints:
+ - websecure
routes:
- match: Host(`exosphere.host`)
kind: Rule
services:
- name: exosphere-landing-page
namespace: default
port: 80
tls:
certResolver: letsencryptAlso applies to: 16-17 🤖 Prompt for AI Agents |
||||||||||||||
| services: | ||||||||||||||
|
Comment on lines
+7
to
+12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Security hardening: consider HSTS on HTTPS routes. If you proceed with HTTPS-only, add a Headers middleware with Example (in a separate middleware manifest): apiVersion: traefik.io/v1alpha1
kind: Middleware
metadata:
name: security-headers
namespace: default
spec:
headers:
stsSeconds: 31536000
stsIncludeSubdomains: true
stsPreload: trueThen attach: routes:
- match: Host(`exosphere.host`)
kind: Rule
- services:
+ middlewares:
+ - name: security-headers
+ services:
- name: exosphere-landing-page🤖 Prompt for AI Agents |
||||||||||||||
| - name: exosphere-landing-page | ||||||||||||||
| namespace: default | ||||||||||||||
| port: 80 | ||||||||||||||
| tls: | ||||||||||||||
| certResolver: letsencrypt | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Add missing newline at EOF to satisfy linters. Apply this diff: - certResolver: letsencrypt
\ No newline at end of file
+ certResolver: letsencrypt
+🧰 Tools🪛 YAMLlint (1.37.1)[error] 17-17: no new line character at the end of file (new-line-at-end-of-file) 🤖 Prompt for AI Agents |
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,9 @@ deployment: | |
|
|
||
| additionalArguments: | ||
| - "--entrypoints.web.address=:8081" | ||
| - "--entrypoints.web.http.redirections.entrypoint.to=:443" | ||
NiveditJain marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - "--entrypoints.web.http.redirections.entrypoint.scheme=https" | ||
| - "--entrypoints.web.http.redirections.entrypoint.permanent=true" | ||
| - "--entrypoints.websecure.address=:8443" | ||
|
|
||
|
Comment on lines
6
to
12
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Avoid duplication: entrypoint-level redirect makes the middleware redundant unless explicitly used. You added both an entrypoint-level redirect and a Middleware. If you keep the entrypoint-level redirect, you generally don't need an HTTP IngressRoute or the redirect middleware. Conversely, if you prefer per-route control using the middleware, remove the additionalArguments and attach the middleware to the HTTP IngressRoutes. Would you like me to generate a minimal set of manifests for either approach (global vs per-route)? 🤖 Prompt for AI Agents |
||
| ports: | ||
|
|
@@ -19,12 +22,12 @@ ports: | |
|
|
||
| ingressRoute: | ||
| dashboard: | ||
| enabled: false # turn on later if you want the UI | ||
| enabled: false | ||
|
|
||
| certificatesResolvers: | ||
| letsencrypt: | ||
| acme: | ||
| email: "nivedit@exosphere.host" | ||
| storage: /data/acme.json | ||
| httpChallenge: | ||
| entryPoint: web # solve the challenge on :80 | ||
| entryPoint: web | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Inconsistent wait message vs actual sleep; also simplify directory apply path.
Apply this diff:
If the intention was 2 minutes, change to:
📝 Committable suggestion
🤖 Prompt for AI Agents