Make route domain error specific

[skonto]

Fixes #12149

Proposed Changes

• Makes route error propagation more specific.
  Instead of:

                    {
                        "lastTransitionTime": "2024-04-03T07:58:25Z",
                        "message": "invalid domain name \"wathola-receiver-test-continuous-events-propagation-with-prober-zxmkp.example.com\": url: Invalid value: \"wathola-receiver-test-continuous-events-propagation-with-prober-zxmkp\": must be no more than 63 characters",
                        "reason": "Unknown",
                        "status": "Unknown",
                        "type": "Ready"
                    }

we get:

                 {
                        "lastTransitionTime": "2024-04-03T10:07:57Z",
                        "message": "invalid domain name \"wathola-receiver-test-continuous-events-propagation-with-prober-zxmkp.example.com\": url: Invalid value: \"wathola-receiver-test-continuous-events-propagation-with-prober-zxmkp\": must be no more than 63 characters",
                        "reason": "ErrorConfig",
                        "status": "False",
                        "type": "Ready"
                    }

• In the future we could make this way permanent errors more specific in other code paths too.

Release Note

Reason and status for domain errors is no more specific to indicate a permanent issue.

[codecov]

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 84.89%. Comparing base (c2d0af1) to head (8dd8686).
Report is 32 commits behind head on main.

Files	Patch %	Lines
pkg/apis/serving/v1/route_lifecycle.go	0.00%	2 Missing ⚠️
pkg/reconciler/route/route.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15082      +/-   ##
==========================================
+ Coverage   84.11%   84.89%   +0.77%     
==========================================
  Files         213      213              
  Lines       16783    13107    -3676     
==========================================
- Hits        14117    11127    -2990     
+ Misses       2315     1623     -692     
- Partials      351      357       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ReToCode]

Do we still need to use fmt.Errorf if we call .Error() on it anyway?

[ReToCode]

Do we have constants for ErrorConfig somewhere? Or why do we even need the reason as there is only one value here?

[skonto]

@dprotaso @izabelacg pls review.

[dprotaso]

I think errors.Is is more apt since you're not using special fields in DomainNameError

[skonto]

error.Is will not work in this case (custom error) as it does not check the type. It returns false. In its code it has:

		if isComparable && err == target {
			return true
		}

This will never be equal, unless we have a ref of some global error with a specific msg eg. created with errors.New().

errors.As does this check:

		if reflectlite.TypeOf(err).AssignableTo(targetType) {
			val.Elem().Set(reflectlite.ValueOf(err))
			return true
		}

that matches the error.
See also
https://stackoverflow.com/questions/39121172/how-to-compare-go-errors.
https://medium.com/@reetas/exploring-the-appropriate-use-cases-for-errors-is-and-errors-as-in-comparing-errors-in-golang-3584c6bc5417

Anyway I will use some predefined error to mark the tree, I can avoid the custom error if that is the goal (since it has no extra fields).

[skonto]

@dprotaso I changed the error stuff. Btw current error msg is:

                    {
                        "lastTransitionTime": "2024-04-09T13:10:13Z",
                        "message": "domain name error: invalid domain name \"helloworld-go-test-continuous-events-propagation-with-prober-zxmkp.example.com\": url: Invalid value: \"helloworld-go-test-continuous-events-propagation-with-prober-zxmkp\": must be no more than 63 characters",
                        "reason": "ErrorConfig",
                        "status": "False",
                        "type": "RoutesReady"
                    }

Is this now ok to go in?

[dprotaso]

I think what you had before was fine (having the struct) you just need to add your own Is method

see: https://go.dev/play/p/d23KugysN38

[skonto]

I think what you had before was fine (having the struct) you just need to add your own Is method

That is just a type check at the end of the day, I was more in favor of reusing existing code but wfm.

[skonto]

@dprotaso your suggestion also makes things a bit restrictive for future use as error typecheck (as it is already known), does not support wrapped errors:

Could we avoid that and keep current implementaion?

[dprotaso]

Could we avoid that and keep current implementaion?

Sure

[dprotaso]

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dprotaso,skonto]
• pkg/apis/OWNERS [dprotaso]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dprotaso]

Can you update the PR body with a release note?