Merge rules per ingress by the same host, pathType and backend service #2986
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @oliviassss. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: oliviassss The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
oliviassss
changed the title
Codecov ReportBase: 54.09% // Head: 53.91% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
==========================================
- Coverage 54.09% 53.91% -0.18%
==========================================
Files 144 145 +1
Lines 8291 8422 +131
==========================================
+ Hits 4485 4541 +56
- Misses 3479 3546 +67
- Partials 327 335 +8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -3,40 +3,116 @@ package ingress | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "sigs.k8s.io/aws-load-balancer-controller/pkg/k8s" | |||
| "sigs.k8s.io/aws-load-balancer-controller/pkg/model/core" | |||
There was a problem hiding this comment.
Please use goimports sorting style
| //func (t *defaultModelBuildTask) buildListenerRules(ctx context.Context, lsARN core.StringToken, port int64, protocol elbv2model.Protocol, ingList []ClassifiedIngress) error { | ||
| // if t.sslRedirectConfig != nil && protocol == elbv2model.ProtocolHTTP { | ||
| // return nil | ||
| // } |
There was a problem hiding this comment.
Please edit this out of the commit.
| // e.g. {(hostA, pathTypeA, serviceNameA, PortNumberA): [path1, path2,...], | ||
| // (hostB, pathTypeB, serviceNameB, PortNumberB): [path3, path4,...], | ||
| // ...} | ||
| mergeRefMap := make(map[[4]string][]networking.HTTPIngressPath) |
There was a problem hiding this comment.
Use a struct instead of a [4]string as the map key
| // ...} | ||
| mergeRefMap := make(map[[4]string][]networking.HTTPIngressPath) | ||
| // pathToRuleMap stores {path: ruleIdx} relationship | ||
| pathToRuleMap := make(map[networking.HTTPIngressPath]int) |
| } | ||
| host := rule.Host | ||
| for _, path := range rule.HTTP.Paths { | ||
| //if path.PathType != nil && (*path.PathType != networking.PathTypePrefix || |
There was a problem hiding this comment.
Please edit out of the commit. It's hard to review with commented-out code.
| @@ -119,20 +243,83 @@ func (t *defaultModelBuildTask) classifyIngressPathsByType(paths []networking.HT | |||
| return exactPaths, prefixPaths, implementationSpecificPaths, nil | |||
| } | |||
|
|
|||
| func (t *defaultModelBuildTask) buildRuleConditions(ctx context.Context, rule networking.IngressRule, | |||
| path networking.HTTPIngressPath, backend EnhancedBackend) ([]elbv2model.RuleCondition, error) { | |||
| // func (t *defaultModelBuildTask) buildRuleConditions(ctx context.Context, rule networking.IngressRule, | |||
There was a problem hiding this comment.
Edit this commented-out block from the commit
| minIdx := pathToRuleMap[mergedPaths[0]] | ||
| for _, path := range mergedPaths { | ||
| currIdx := pathToRuleMap[path] | ||
| minIdx = algorithm.GetMin(minIdx, currIdx) |
There was a problem hiding this comment.
The Go way is to just inline this.
| // use a map to help merge rules by host, pathType and service | ||
| // getRulesToMerge classifies rules into two categories - rules with unique host, rules with replicate hosts | ||
| // only rules with replicate hosts need merge | ||
| func (t *defaultModelBuildTask) getRulesToMerge(rules []networking.IngressRule) ([]networking.IngressRule, []networking.IngressRule, error) { |
There was a problem hiding this comment.
Perhaps name the return values?
| _, exists := hostToRulesMap[host] | ||
| if exists { | ||
| hostToRulesMap[host] = append(hostToRulesMap[host], rule) | ||
| } else { | ||
| hostToRulesMap[host] = []networking.IngressRule{rule} | ||
| } |
There was a problem hiding this comment.
hostToRulesMap[host] = append(hostToRulesMap[host], rule)
| // getRulesToMerge classifies rules into two categories - rules with unique host, rules with replicate hosts | ||
| // only rules with replicate hosts need merge | ||
| func (t *defaultModelBuildTask) getRulesToMerge(rules []networking.IngressRule) ([]networking.IngressRule, []networking.IngressRule, error) { | ||
| var rulesWithReplicateHosts []networking.IngressRule |
There was a problem hiding this comment.
rulesWithReplicatedHosts
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
Issue
#2203
#2678
Description
Merged the rules per ingress if:
Tests
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯