Handle the case when ingress name has dot #532

ZhiminXiang · 2021-03-02T19:11:51Z

Fixes #
#514

Release Note

Fix the incorrect Gateway name format for DomainMapping auto TLS feature for Istio implmenetation

ZhiminXiang · 2021-03-02T19:12:14Z

/assign @nak3 @tcnghia

codecov · 2021-03-02T19:17:08Z

Codecov Report

Merging #532 (8b4bf3e) into main (713aacd) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            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

Impacted Files	Coverage Δ
pkg/reconciler/ingress/resources/gateway.go	`85.63% <100.00%> (+0.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713aacd...8b4bf3e. Read the comment docs.

JRBANCEL · 2021-03-02T19:28:48Z

pkg/reconciler/ingress/resources/gateway.go

+// 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 {


Can we use the existing https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go#L187 ?

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?

I get your point, but this is not some random function, it follows RFC 1123. It is not going to change.
Up to you.

nak3

Left a few comments but looks good to me overall.

nak3 · 2021-03-03T03:36:41Z

pkg/reconciler/ingress/resources/gateway.go

@@ -304,6 +323,14 @@ func MakeTLSServers(ing *v1alpha1.Ingress, ingressTLS []v1alpha1.IngressTLS, gat
 	return SortServers(servers), nil
 }

+func portNamePrefix(prefix, ingressName string) string {


Suggested change

func portNamePrefix(prefix, ingressName string) string {

func portNamePrefix(ingressNamespace, ingressName string) string {

or

Suggested change

func portNamePrefix(prefix, ingressName string) string {

func portNamePrefix(prefix, suffix string) string {

nak3 · 2021-03-03T03:43:46Z

pkg/reconciler/ingress/resources/gateway.go

-const GatewayHTTPPort = 80
+const (
+	GatewayHTTPPort       = 80
+	DNS1123LabelMaxLength = 63 // Public for testing only.


Suggested change

DNS1123LabelMaxLength = 63 // Public for testing only.

dns1123LabelMaxLength = 63

I think this is not used for testing 🤔

knative-metrics-robot · 2021-03-05T18:07:05Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-net-istio-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/ingress/resources/gateway.go	85.6%	86.2%	0.6

knative-prow-robot · 2021-03-06T01:15:08Z

[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:

~~OWNERS~~ [nak3]

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

handle the case when ingress name has dot

01fd11f

ZhiminXiang requested a review from a team as a code owner March 2, 2021 19:11

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 2, 2021

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 2, 2021

knative-prow-robot assigned nak3 and tcnghia Mar 2, 2021

JRBANCEL reviewed Mar 2, 2021

View reviewed changes

nak3 reviewed Mar 3, 2021

View reviewed changes

Base automatically changed from master to main March 4, 2021 16:24

markusthoemmes requested a review from a team as a code owner March 4, 2021 16:24

markusthoemmes requested a review from a team March 4, 2021 16:24

markusthoemmes requested a review from a team as a code owner March 4, 2021 16:24

markusthoemmes requested review from fallback-notification-blocker and removed request for a team March 4, 2021 16:24

Zhimin Xiang added 2 commits March 5, 2021 17:50

address comment

b31943f

fix typo

8b4bf3e

nak3 approved these changes Mar 6, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2021

knative-prow-robot merged commit eebb4e6 into knative-extensions:main Mar 6, 2021

tcnghia mentioned this pull request Mar 9, 2021

Fail to create TLSGateway from DomainMapping's Kingress due to invalid name #514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle the case when ingress name has dot #532

Handle the case when ingress name has dot #532

ZhiminXiang commented Mar 2, 2021

ZhiminXiang commented Mar 2, 2021

codecov bot commented Mar 2, 2021 •

edited

JRBANCEL Mar 2, 2021

ZhiminXiang Mar 2, 2021

JRBANCEL Mar 2, 2021

nak3 left a comment

nak3 Mar 3, 2021

ZhiminXiang Mar 5, 2021

nak3 Mar 3, 2021

ZhiminXiang Mar 5, 2021

knative-metrics-robot commented Mar 5, 2021

knative-prow-robot commented Mar 6, 2021

	func portNamePrefix(prefix, ingressName string) string {
	func portNamePrefix(ingressNamespace, ingressName string) string {

	func portNamePrefix(prefix, ingressName string) string {
	func portNamePrefix(prefix, suffix string) string {

	DNS1123LabelMaxLength = 63 // Public for testing only.
	dns1123LabelMaxLength = 63

Handle the case when ingress name has dot #532

Handle the case when ingress name has dot #532

Conversation

ZhiminXiang commented Mar 2, 2021

ZhiminXiang commented Mar 2, 2021

codecov bot commented Mar 2, 2021 • edited

Codecov Report

JRBANCEL Mar 2, 2021

Choose a reason for hiding this comment

ZhiminXiang Mar 2, 2021

Choose a reason for hiding this comment

JRBANCEL Mar 2, 2021

Choose a reason for hiding this comment

nak3 left a comment

Choose a reason for hiding this comment

nak3 Mar 3, 2021

Choose a reason for hiding this comment

ZhiminXiang Mar 5, 2021

Choose a reason for hiding this comment

nak3 Mar 3, 2021

Choose a reason for hiding this comment

ZhiminXiang Mar 5, 2021

Choose a reason for hiding this comment

knative-metrics-robot commented Mar 5, 2021

knative-prow-robot commented Mar 6, 2021

codecov bot commented Mar 2, 2021 •

edited