-
-
Notifications
You must be signed in to change notification settings - Fork 326
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: http to https #264
fix: http to https #264
Conversation
WalkthroughThis update enhances router configuration testing for both HTTP and HTTPS, ensuring proper middleware application based on domain settings. It also improves domain management functionality, allowing for conditional secure router creation and a more streamlined approach to handling entry points. Overall, these changes strengthen the application's routing logic and maintainability. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/traefik/traefik.test.ts (1 hunks)
- server/utils/traefik/domain.ts (4 hunks)
Additional comments not posted (13)
server/utils/traefik/domain.ts (5)
16-16
: LGTM!The introduction of
routerNameSecure
for handling secure traffic is clear and straightforward.
22-26
: LGTM!The
createRouterConfig
function is called with the "web" entry point, aligning with the updated function signature for more granular control.
28-37
: LGTM!The conditional creation and deletion of the secure router based on the
domain.https
flag enhances flexibility in managing HTTP and HTTPS traffic.
47-55
: LGTM!The deletion of the secure router if it exists ensures proper cleanup of configurations.
Line range hint
71-110
:
LGTM!The updated function signature and refined logic for handling entry points and middlewares improve the maintainability and clarity of the code.
__test__/traefik/traefik.test.ts (8)
83-91
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is not added for an HTTP domain.
93-106
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is not added and the redirect middleware is added for an HTTP domain with a redirect.
108-125
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is not added and multiple redirect middlewares are added for an HTTP domain with multiple redirects.
127-135
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is added for an HTTPS domain.
137-150
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is added and the redirect middleware is not added for an HTTPS domain with a redirect.
152-160
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is not added for a "websecure" entry point on an HTTPS domain.
162-175
: LGTM!The test correctly verifies that the "redirect-to-https" middleware is not added and the redirect middleware is added for a "websecure" entry point on an HTTPS domain with a redirect.
179-187
: LGTM!The test correctly verifies that the correct certificate resolver is set for a "websecure" entry point with a "letsencrypt" certificate type.
Thank you for this work @lorenzomigliorero and the test are really appreciated. everything seems right, just a few note, when adding a domain you need to first enable |
@Siumauricio Should we condition the certificate in another PR? This is about solving the |
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- test/traefik/traefik.test.ts (1 hunks)
- server/utils/traefik/domain.ts (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- test/traefik/traefik.test.ts
- server/utils/traefik/domain.ts
Fix: #253
When a domain is HTTPS, we need to generate two routers, one for the web and one for the webscure entrypoint.
The redirect-to-https middleware needs to stay on the web router, otherwise it is not catched.
There's a bit of conditioning to manage, I summarise here all the cases:
I wrote a unit test file to test these conditions. More tests can be added to cover other features, like domain deletion and domain update, but it's a stable starting point.
Summary by CodeRabbit
New Features
Bug Fixes