Add URL to TrafficTarget Status #3526
Conversation
This change adds URL to the output of each named Traffic Target. The URL is prefixed with a scheme. Currently we only support subdomains for named traffic targets and 'http' as the scheme. However, this change enables clients to grab the URL from the API rather than making assumptions on scheme and URL format which paves the way for operator customization. Fixes knative#3450
knative-prow-robot
added
size/L
There was a problem hiding this comment.
@dgerd: 1 warning.
In response to this:
This change adds URL to the output of each named Traffic Target. The URL
is prefixed with a scheme. Currently we only support subdomains for
named traffic targets and 'http' as the scheme. However, this change enables
clients to grab the URL from the API rather than making assumptions on
scheme and URL format which paves the way for operator customization.Fixes #3450
Release Note
URL is now present on Status.Traffic entries that have "name" specified.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.
* Add domain as status update in route reconciler test * Switch back to using K8sServiceFullname instead of Address.Hostname * Unexport scheme
|
Created an issue ( knative/docs#1078 ) to update docs when this is merged. Also created an issue to expand our scheme support from HTTP ( #3527 ). |
| if tt.Name == blue.TrafficTarget { | ||
| // Strip prefix as WaitForEndPointState expects a domain | ||
| // without scheme. | ||
| blueDomain = strings.TrimPrefix(tt.URL, "http://") |
There was a problem hiding this comment.
Perhaps use "net/url" to parse the URL and extract the host?
There was a problem hiding this comment.
I prefer this for now as it is the opposite of what "WaitForEndpoingState" is doing. However, "WaitForEndpointState" needs to be fixed.
I would like to follow up and update pkg to not do url.Parse(fmt.Sprintf("http://%s", theURL)). There is a fallback to asURL, err = url.Parse(theURL); however, due to the parsing behavior the fallback does not actually work.
Once pkg is updated this can be removed entirely.
| greenDomain = strings.TrimPrefix(tt.URL, "http://") | ||
| } | ||
| } | ||
| if blueDomain == "" || greenDomain == "" { |
There was a problem hiding this comment.
Do we want to validate the actual values?
There was a problem hiding this comment.
For conformance I don't want to validate how the sub-route domains get formed (i.e. "green.ksvc.domain.com" or "green-ksvc.domain.com").
I could use url.Parse() and make sure a domain comes out, but I am not in love with the validation, and we do not do anything similar for route.Status.Domain in any of our e2e tests.
If the url is not empty and the domain is invalid the problem should be fairly easy to spot with existing output. It should look something like:
util.go:210: For domain green.blue-green-route-nfluhlbe.serving-tests.example.com: wanted at least 50, got 0 requests.
|
/lgtm |
|
/test pull-knative-serving-integration-tests |
|
Forgot to clear cache before testing/building locally. Should be good now. |
|
/test pull-knative-serving-integration-tests |
|
/test pull-knative-serving-integration-tests |
|
/lgtm |
| return p.String() == "URL" | ||
| }, | ||
| cmp.Ignore(), | ||
| ) |
|
The following is the coverage report on pkg/.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dgerd, mattmoor 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:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
This change adds URL to the output of each named Traffic Target. The URL
is prefixed with a scheme. Currently we only support subdomains for
named traffic targets and 'http' as the scheme. However, this change enables
clients to grab the URL from the API rather than making assumptions on
scheme and URL format which paves the way for operator customization.
Fixes #3450
Release Note