fix(state): use upstreamNameTarget index instead of target #60
Conversation
| } | ||
| target.Upstream = u.DeepCopy() | ||
|
|
||
| s.Targets.Add(*target) |
There was a problem hiding this comment.
Could you explain the reason for doing these changes?
Other entities which have a foreign relation (like routes and services), don't actually search for a service and associate it with the route before adding/updating.
These shouldn't be required from what I can see. I might be missing something here.
There was a problem hiding this comment.
If we didn't set the upstream field here, the Upstream field of target here will have only a ID field. As we require target.Upstream.Name in TargetsCollection , this s.Targets.Add(*target) will return an error here.
And as the error is ignored here, this may cause problem for later usage of s.Targets.
| @@ -112,6 +120,26 @@ func (k *TargetsCollection) Get(ID string) (*Target, error) { | |||
| return &Target{Target: *t.DeepCopy()}, nil | |||
| } | |||
|
|
|||
| // GetByUpstreamNameAndTarget get a target by upstreamName and target | |||
| func (k *TargetsCollection) GetByUpstreamNameAndTarget(upstreamName, target string) (*Target, error) { | |||
There was a problem hiding this comment.
I'd recommend that we keep the name of this method as Get, the doc line should make it clear that the upstreamName and target are required.
Also, we need to make this method a little smarter:
It should accept an upstream name or ID and a target name or ID as input and then return the correct entity.
For this, you will need to use do a multi-index lookup by target and ID first, and then do a linear search based on upstream name or ID.
Please use the following code as reference: 390adbc#diff-a9f2da3b8eedc2c30a573af4b8802f5eR109-R139
If we follow this approach, the index on Target field can remain as is, only, change Unique to false.
|
We will include this in Kong Ingress Controller 0.6.0. |
A Target with the same IP and port can be associated with multiple upstreams. The state table previously had Target set to unique, resulting in unique key violation. This change indexed uniquely based on a combination of upstream and target. This change affect Kong Ingress Controller as well, which is where this problem was first noted. See #60
A Target with the same IP and port can be associated with multiple upstreams. The state table previously had Target set to unique, resulting in unique key violation. This change indexed uniquely based on a combination of upstream and target. This change affect Kong Ingress Controller as well, which is where this problem was first noted. See #60 Co-authored-by: Harry Bagdi <harrybagdi@gmail.com>
A Target with the same IP and port can be associated with multiple upstreams. The state table previously had Target set to unique, resulting in unique key violation. This change indexed uniquely based on a combination of upstream and target. This change affect Kong Ingress Controller as well, which is where this problem was first noted. See #60 Co-authored-by: Harry Bagdi <harrybagdi@gmail.com>
A Target with the same IP and port can be associated with multiple upstreams. The state table previously had Target set to unique, resulting in unique key violation. This change indexed uniquely based on a combination of upstream and target. This change affect Kong Ingress Controller as well, which is where this problem was first noted. See #60 Co-authored-by: Harry Bagdi <harrybagdi@gmail.com>
Related issue #57
Remove the unique target index in TargetsCollection, replace it with a union index of (upstreamName, target). Allow different upstreams to share same targets.
I know this PR may break calls of TargetsCollection outside this repo, and requiring upstream name in target is kind of tricky. But I checked the kubernetes-ingress-controller repo and there's no direct usage of TargetsCollection. So I think it might be ok.
As a fan of Kong, we're considering using the office kubernetes-ingress-controller to replace our version. So truly appreciated if you can take a look at this PR. Thanks.