Handle the case when ingress name has dot #532
Conversation
knative-prow-robot
added
the
size/M
Codecov Report
@@ Coverage Diff @@
## main #532 +/- ##
==========================================
+ Coverage 81.43% 81.54% +0.10%
==========================================
Files 15 15
Lines 1018 1024 +6
==========================================
+ Hits 829 835 +6
Misses 110 110
Partials 79 79
Continue to review full report at Codecov.
|
| // We directly copy this function instead of importing it into vendor and using it because | ||
| // if this function is changed in the upstream (for example, Istio allows the dot in the future), we don't want to | ||
| // import the change without awareness because it could break the compatibility of Gateway name generation. | ||
| func isDNS1123Label(value string) bool { |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think technically we can either use istio one or k8s one.
But I am inclined to copy the function into this repo because if there is any change to this function in the upstream (e.g. allowing dot in the future), then the gateway name could be changed implicitly and we may not be aware of this.
If the version of the function we use in the our code is not compatible with the requirement of Istio or k8s in the future, then our E2E tests should be failed and we will get notice about it. WDYT?
There was a problem hiding this comment.
I get your point, but this is not some random function, it follows RFC 1123. It is not going to change.
Up to you.
| @@ -304,6 +323,14 @@ func MakeTLSServers(ing *v1alpha1.Ingress, ingressTLS []v1alpha1.IngressTLS, gat | |||
| return SortServers(servers), nil | |||
| } | |||
|
|
|||
| func portNamePrefix(prefix, ingressName string) string { | |||
There was a problem hiding this comment.
| func portNamePrefix(prefix, ingressName string) string { | |
| func portNamePrefix(ingressNamespace, ingressName string) string { |
or
| func portNamePrefix(prefix, ingressName string) string { | |
| func portNamePrefix(prefix, suffix string) string { |
| const GatewayHTTPPort = 80 | ||
| const ( | ||
| GatewayHTTPPort = 80 | ||
| DNS1123LabelMaxLength = 63 // Public for testing only. |
There was a problem hiding this comment.
| DNS1123LabelMaxLength = 63 // Public for testing only. | |
| dns1123LabelMaxLength = 63 |
I think this is not used for testing 🤔
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nak3 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
Fixes #
#514
Release Note