httpcaddyfile: Add optional status code argument to handle_errors directive

[armadi1809]

Resolves #5928

I am pretty sure there is a lot that can be improved here since this is my first PR. I particularly don't like the way I add the expression matcher to the subroutes created inside the parseHandleErrors and I am pretty there is a better way of doing it.

Thank you in advance for reviewing my code!

[CLAassistant]

All committers have signed the CLA.

[mohammed90]

You can add tests for the Caddyfile translation to JSON. See this directory for examples:

https://github.com/caddyserver/caddy/tree/4173e2c77ab883a509ef3be1cbdc868442c5a9b8/caddytest/integration/caddyfile_adapt

You can also test the behavior by adding tests to this file:

https://github.com/caddyserver/caddy/blob/4173e2c77ab883a509ef3be1cbdc868442c5a9b8/caddytest/integration/caddyfile_test.go

[armadi1809]

@mohammed90 For the attached Caddyfile, does the attahced json file looks right? This works but I am not sure if this is the expected result.
caddyJson.txt
Caddyfile.txt

[mohammed90]

@mohammed90 For the attached Caddyfile, does the attahced json file looks right? This works but I am not sure if this is the expected result. caddyJson.txt Caddyfile.txt

Don't make it one giant test. Break it into multiple ones that target a specific transformation behavior. You can have the giant test as an additional one to ensure the smaller parts still function properly even when merged. It also helps you with validating your understanding of the implementation as you manually do the process which you've automated.

At a glance, it seems correct, but break it apart to make the review easier and the validation more concrete and accurate.

[francislavoie]

Might need a test that covers having handle_errors in two different site blocks. I don't remember if we were doing it before, but I think we need to wrap error routes with a host matcher like we do for regular routes. Maybe nobody's tried to use handle_errors in multiple site blocks yet 🙈

[armadi1809]

Sounds good. I will work on adding the tests and see about the multiple site blocks.

However, can someone explain why are all those test jobs failing? The logs within the jobs themselves are not super helpful :'(

[armadi1809]

Oh I think we need to update the test files for the old handle_errors am I right?

[francislavoie]

This test is failing:

=== RUN   TestCaddyfileAdaptToJSON
  {
  	"apps": {
  		"http": {
  			"servers": {
  				"srv0": {
  					"listen": [
  						":443"
  					],
  					"routes": [
  						{
  							"match": [
  								{
  									"host": [
  										"example.com"
  									]
  								}
  							],
  							"handle": [
  								{
  									"handler": "subroute",
  									"routes": [
  										{
  											"handle": [
  												{
  													"handler": "vars",
  													"root": "/srv"
  												}
  											]
  										},
  										{
  											"handle": [
  												{
  													"error": "Unauthorized",
  													"handler": "error",
  													"status_code": 403
  												}
  											],
  											"match": [
  												{
  													"path": [
  														"/private*"
  													]
  												}
  											]
  										},
  										{
  											"handle": [
  												{
  													"error": "Not found",
  													"handler": "error",
  													"status_code": 404
  												}
  											],
  											"match": [
  												{
  													"path": [
  														"/hidden*"
  													]
  												}
  											]
  										},
  										{
  											"handle": [
  												{
  													"handler": "file_server",
  													"hide": [
  														"./Caddyfile"
  													]
  												}
  											]
  										}
  									]
  								}
  							],
  							"terminal": true
  						}
  					],
  					"errors": {
  						"routes": [
  							{
 - 								"match": [
 + 								"group": "group0",
 + 								"handle": [
  									{
 - 										"host": [
 - 											"example.com"
 - 										]
 + 										"handler": "rewrite",
 + 										"uri": "/{http.error.status_code}.html"
  									}
 - 								],
 + 								]
 + 							},
 + 							{
  								"handle": [
  									{
 - 										"handler": "subroute",
 - 										"routes": [
 - 											{
 - 												"group": "group0",
 - 												"handle": [
 - 													{
 - 														"handler": "rewrite",
 - 														"uri": "/{http.error.status_code}.html"
 - 													}
 - 												]
 - 											},
 - 											{
 - 												"handle": [
 - 													{
 - 														"handler": "file_server",
 - 														"hide": [
 - 															"./Caddyfile"
 - 														]
 - 													}
 - 												]
 - 											}
 + 										"handler": "file_server",
 + 										"hide": [
 + 											"./Caddyfile"
  										]
  									}
 - 								],
 - 								"terminal": true
 + 								]
  							}
  						]
  					}
  				}
  			}
  		}
  	}
  }
    caddyfile_adapt_test.go:52: failed to adapt error_example.txt
    caddytest.go:401: warning: heredoc.txt:2: : Caddyfile input is not formatted; run 'caddy fmt --overwrite' to fix inconsistencies
    caddytest.go:401: warning: map_and_vars_with_raw_types.txt:3: : Caddyfile input is not formatted; run 'caddy fmt --overwrite' to fix inconsistencies

And you'll need to run gofumpt -w caddyconfig/httpcaddyfile/builtins.go to make the linter happy I think.

[armadi1809]

@mohammed90 I broke down the test into 4 smaller pieces:
1- A test with just the status code
2- A test with the range using the xx suffix
3- A test to make sure the sorting works when there is a handle_error directive without a status code
4- A test combining ranges and simple status codes together.

I also made sure the test that was failing in the pipelines is now passing.

Please let me know if there is anything else I should do for you to be able to review my code. Thank you!

[francislavoie]

Great! Tests and linter are passing. I think the last thing to handle is multiple site blocks, making sure they stay mutually exclusive. The existing tests will probably need to be adjusted as well to make sure they're wrapped with a subroute with a host matcher.

[armadi1809]

@francislavoie I think I get what you mean. I will try to put something together for that shortly. Thank you for the feedback.

[armadi1809]

@francislavoie By multiple site blocks do you mean something like the attached Caddyfile? If so, that is working properly with the current code. Let me know if I am not understanding your request correctly.
Caddyfile.txt

[francislavoie]

Not quite, I mean 2 sites with the different hosts but the same port. So like:

foo.localhost {
	...
}

bar.localhost {
	...
}

This will use the same server in JSON config with their routes being separated by a host matcher.

[armadi1809]

@francislavoie, Okay I now understand what you mean. But how would you think this should look like in the context of handling errors. Should the routes have 2 matchers? A host matcher and an expression matcher? Is that possible?

[francislavoie]

It should work the same as the non-error routes. Each site produces a subroute with a host matcher, then all the error handlers for that site go within that matcher. Nested subroutes.

[armadi1809]

@francislavoie. I think it's all good now. I added a test caddyfile to account for the multiple site blocks case. I also adjusted other tests to include the host matcher in them.

[armadi1809]

Hi @francislavoie, I just wanted to bring your attention to this PR again. Your feedback on the latest changes would be appreciated if you still see issues with the changes suggested here. Thank you!

[francislavoie]

Yep, holidays and things 😂 thanks for the reminder, doing a round of review now.

[francislavoie]

Looks great! I made a quick commit to remove leftovers from the first commit that isn't needed anymore. This also needs a bit more validation logic to ensure bad inputs are rejected. Almost ready!

[armadi1809]

@francislavoie thank you for your feedback. The requested changes should now be implemented.

[francislavoie]

I don't see any changes since I last reviewed 🤔 did you forget to push?

[armadi1809]

Strange 😅. This is a screenshot of what I am seeing. The force push should contain the changes.

[francislavoie]

I'm not seeing any added validation code 🤔

[francislavoie]

That's better, thank you!