fix: ingress rules order is preserved

[mlavacca]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Ingress Rules order needs to be preserved when converting into HTTPRoute rules.

Which issue(s) this PR fixes:

Fixes #91

Does this PR introduce a user-facing change?:

NONE

[LiorLieberman]

@mlavacca I think we want to make sure we dont produce different rule orders other than making the tests pass with different order.
Changing the rule orders have implications. Also it can result in unnecessary update to the API(in case the HTTPRoute is already in the cluster)

[mlavacca]

@mlavacca I think we want to make sure we dont produce different rule orders other than making the tests pass with different order. Changing the rule orders have implications. Also it can result in unnecessary update to the API(in case the HTTPRoute is already in the cluster)

yep, I agree that we should preserve the rule orders. I dug a bit into the issue, and the problem is caused by the fact that in the common package we aggregate the ingress rules into a map, and then we iterate that map to create HTTPRoute rules. Since map order is not guaranteed, this can cause flakes. This problem has been surfaced by the new kong test as there were no tests with ingresses having multiple rules previously. We may need to add some logic to fix it. I'll do it in this PR.

[levikobi]

Thanks for tackling this one @mlavacca!

[LiorLieberman]

Throwing an idea.
Is this helping also achieving what we want?

type orderedPathsByMatchGroup struct {
	keys []pathMatchKey
	data map[pathMatchKey][]ingressPath
}

func (rg *ingressRuleGroup) toHTTPRoute() (gatewayv1beta1.HTTPRoute, field.ErrorList) {
	// pathsByMatchGroup := map[pathMatchKey][]ingressPath{}
	pathsByMatchGroup := &orderedPathsByMatchGroup{
		keys: []pathMatchKey{},
		data: make(map[pathMatchKey][]ingressPath),
	}
	var errors field.ErrorList

	for i, ir := range rg.rules {
		for j, path := range ir.rule.HTTP.Paths {
			ip := ingressPath{ruleIdx: i, pathIdx: j, ruleType: "http", path: path}
			pmKey := getPathMatchKey(ip)
			if _, ok := pathsByMatchGroup.data[pmKey]; !ok {
				pathsByMatchGroup.keys = append(pathsByMatchGroup.keys, pmKey)
			}
			pathsByMatchGroup.data[pmKey] = append(pathsByMatchGroup.data[pmKey], ip)

		}
	}

later in the range:

	for _, pmKey := range pathsByMatchGroup.keys {
		paths := pathsByMatchGroup.data[pmKey]
		path := paths[0]

By keeping only the keys in a slice, we can also retain the order in which the elements were added.
I have not ran this but it feels like it could work

[mlavacca]

Throwing an idea. Is this helping also achieving what we want?

type orderedPathsByMatchGroup struct {
	keys []pathMatchKey
	data map[pathMatchKey][]ingressPath
}

func (rg *ingressRuleGroup) toHTTPRoute() (gatewayv1beta1.HTTPRoute, field.ErrorList) {
	// pathsByMatchGroup := map[pathMatchKey][]ingressPath{}
	pathsByMatchGroup := &orderedPathsByMatchGroup{
		keys: []pathMatchKey{},
		data: make(map[pathMatchKey][]ingressPath),
	}
	var errors field.ErrorList

	for i, ir := range rg.rules {
		for j, path := range ir.rule.HTTP.Paths {
			ip := ingressPath{ruleIdx: i, pathIdx: j, ruleType: "http", path: path}
			pmKey := getPathMatchKey(ip)
			if _, ok := pathsByMatchGroup.data[pmKey]; !ok {
				pathsByMatchGroup.keys = append(pathsByMatchGroup.keys, pmKey)
			}
			pathsByMatchGroup.data[pmKey] = append(pathsByMatchGroup.data[pmKey], ip)

		}
	}

later in the range:

	for _, pmKey := range pathsByMatchGroup.keys {
		paths := pathsByMatchGroup.data[pmKey]
		path := paths[0]

By keeping only the keys in a slice, we can also retain the order in which the elements were added. I have not ran this but it feels like it could work

It makes sense, your proposal is more efficient. I've made this change and changed the tests accordingly.

[LiorLieberman]

Thanks Mattia!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiorLieberman, mlavacca

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 [LiorLieberman]

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

[LiorLieberman]

Thanks Mattia!
/lgtm