Determine ServiceInstances from Proxy labels instead of Pod Spec when possible #16483
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
howardjohn
requested review from
costinm,
hzxuzhonghu,
mandarjog and
rshriram
as code owners
istio-testing
added
the
do-not-merge/work-in-progress
googlebot
added
the
cla: yes
istio-testing
added
the
size/M
|
/retest |
|
seems this breaks multicluster |
|
/retest |
|
This looks like As #16476 says, there is still a potential bug for this. |
|
the problem is with split listeners we need to be able to apply inbound rules, etc, immediatelyotherwise there will be a period of time when inbound traffic comes in and rules are not applied the reason this is safe currently is because we have the readiness check to ensure inbound listeners are received, but this won't work now since container port is not required. better ideas are welcome, as I'm not sure this will work |
I am not sure about this? |
|
@hzxuzhonghu see changes like #16528. We will now capture all ports in inbound iptables. If you don't have containport, you will get Passthrough. |
rshriram
reviewed
Aug 26, 2019
| // metadata already. Because of this, we can still get most of the information we need. | ||
| // While this may not be 100% accurate, the pod information should show up within a few second, so the impact should be minimal. | ||
| // With this information we can set up RBAC, so we are not vulnerable during this time. | ||
| if len(out) == 0 && len(proxy.WorkloadLabels) > 0 { |
There was a problem hiding this comment.
I would not do this as fallback but as the main way of looking up the pod labels as it’s a very costly operation to look up by labels.
There was a problem hiding this comment.
Yeah I agree now, working on changing this to detect if we have all info require in labels and use as main lookup
istio-testing
added
size/L
golangcibot
reviewed
Aug 26, 2019
howardjohn
force-pushed
the
pilot/k8s-service-race
branch
from
654ce01
to
0d73c8f
Compare
istio-testing
added
size/M
golangcibot
reviewed
Aug 27, 2019
howardjohn
requested review from
linsun,
morvencao,
ostromart and
sdake
as code owners
istio-testing
added
size/L
|
/retest infra flake |
|
Fixes #16630 |
costinm
reviewed
Aug 28, 2019
| @@ -405,6 +405,10 @@ global: | |||
| # have a shared root CA for this model to work. | |||
| enabled: false | |||
|
|
|||
| # Should be set to the name of the cluster this installation will run in. This is required for sidecar injection | |||
| # to properly label proxies | |||
There was a problem hiding this comment.
The name must match the secret name configured for multicluster ( or whatever we use on the other side ).
costinm
reviewed
Aug 28, 2019
| // metadata already. Because of this, we can still get most of the information we need. | ||
| // If we cannot accurately construct ServiceInstances from just the metadata, this will return an error and we can | ||
| // attempt to read the real pod. | ||
| instances, err := c.getProxyServiceInstancesFromMetadata(proxy) |
There was a problem hiding this comment.
How about doing this only of getPodByIP returns null ?
There was a problem hiding this comment.
That beats the purpose of the effort in some sense. It is a lot of overhead to keep looking up stuff from k8s, especially in large clusters. Even if k8s code returns null, we have "done" the work which actually increases the backlog of connections that Pilot needs to service. OTOH, quickly creating the instance and starting to service the proxy will relieve the backlog.
costinm
reviewed
Aug 28, 2019
|
/lgtm |
|
@costinm @rshriram you two seem to have different opinions on which should
be primary (labels or pod). I am on the fence. Any strong
opinion/justification for one or the other?
Certainly having it be the primary method was needed for testing since in
our CI tests I doubt we run into pod==nil often
…On Wed, Aug 28, 2019 at 4:02 PM Yuchen Dai ***@***.***> wrote:
/lgtm
I think it would address the #16630
<#16630> Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16483?email_source=notifications&email_token=AAEYGXKRFEV37LRUERINTPDQG37XZA5CNFSM4IOYBGK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5MW5TI#issuecomment-525954765>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEYGXMFP4VDDC3DKPMTHQDQG37XZANCNFSM4IOYBGKQ>
.
|
rshriram
approved these changes
Aug 28, 2019
| @@ -405,6 +405,10 @@ global: | |||
| # have a shared root CA for this model to work. | |||
| enabled: false | |||
|
|
|||
| # Should be set to the name of the cluster this installation will run in. This is required for sidecar injection | |||
| # to properly label proxies | |||
| clusterName: "" | |||
There was a problem hiding this comment.
Nit. Call it as clusterID if the meta is called id. I wonder if we can auto initialize this to some uuid
There was a problem hiding this comment.
see my comment above - this must match the name used in the 'registry' config, I don't think it can be a generated UUID ( without making it hard to configure the registries). And there are plans to use the cluster name in telemetry, policy,etc.
| // metadata already. Because of this, we can still get most of the information we need. | ||
| // If we cannot accurately construct ServiceInstances from just the metadata, this will return an error and we can | ||
| // attempt to read the real pod. | ||
| instances, err := c.getProxyServiceInstancesFromMetadata(proxy) |
There was a problem hiding this comment.
That beats the purpose of the effort in some sense. It is a lot of overhead to keep looking up stuff from k8s, especially in large clusters. Even if k8s code returns null, we have "done" the work which actually increases the backlog of connections that Pilot needs to service. OTOH, quickly creating the instance and starting to service the proxy will relieve the backlog.
| dummyPod := &v1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: proxy.ConfigNamespace, | ||
| Labels: proxy.WorkloadLabels[0], |
There was a problem hiding this comment.
I dont think thats the case in Consul either. I have no idea why this workloadLabels was an array of maps in the first place. But make sure that the map is not split into each array entry (with one k-v per array entry)
| } | ||
|
|
||
| // Find the Service associated with the pod. | ||
| svcLister := listerv1.NewServiceLister(c.services.informer.GetIndexer()) |
There was a problem hiding this comment.
I don't know much about the right or most efficient k8s client apis to use. Would be good to get an ack from @hzxuzhonghu on this piece alone as he does work in that area
There was a problem hiding this comment.
This is the most efficient way currently using k8s native method. But compared to the reverse search it is more costly.
| Network: c.endpointNetwork(proxy.IPAddresses[0]), | ||
| Locality: util.LocalityToString(proxy.Locality), | ||
| }, | ||
| Service: svcMap[hostname], |
There was a problem hiding this comment.
hopefully we are guaranteed that this map is not being changed while these instances are being constructed?
There was a problem hiding this comment.
good catch, I think this could happen. I'll fix this
| } | ||
|
|
||
| // findPortFromMetadata resolves the TargetPort of a Service Port, by reading the Pod spec. | ||
| func findPortFromMetadata(svcPort v1.ServicePort, podPorts string) (int, error) { |
There was a problem hiding this comment.
I strongly suggest to not do this metadata lookup in that double forloop above, as this function is doing a bunch of json marshalling which is going to kill perf. Instead, parse the podPorts into some useful structure, may be even our own Port object where name can be "" if the port is of type int. Then in https://github.com/istio/istio/pull/16483/files#diff-288be3f23828adceb9ebb059ce53a23eR619, you can simply do a lookup from the pre-processed array.
There was a problem hiding this comment.
good call, will move it out
howardjohn
force-pushed
the
pilot/k8s-service-race
branch
from
6e39f17
to
b1a801b
Compare
|
/retest |
costinm
approved these changes
Aug 30, 2019
|
In response to a cherrypick label: #16483 failed to apply on top of branch "release-1.3": |
|
@howardjohn Please submit a manual backport PR. |
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Aug 30, 2019
(cherry picked from commit 2e789b9)
|
backport sent
…On Fri, Aug 30, 2019 at 12:54 PM Romain Lenglet ***@***.***> wrote:
@howardjohn <https://github.com/howardjohn> Please submit a manual
backport PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16483?email_source=notifications&email_token=AAEYGXIQYASPD43UAIDO7G3QHF3F3A5CNFSM4IOYBGK2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5STTZA#issuecomment-526727652>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAEYGXK3GISVMMTRGLUIRL3QHF3F3ANCNFSM4IOYBGKQ>
.
|
istio-testing
pushed a commit
that referenced
this pull request
Aug 30, 2019
hzxuzhonghu
reviewed
Sep 23, 2019
| Port: targetPort, | ||
| ServicePort: svcPort, | ||
| Network: c.endpointNetwork(proxy.IPAddresses[0]), | ||
| Locality: util.LocalityToString(proxy.Locality), |
There was a problem hiding this comment.
@howardjohn This is a regression, we can never get the real locality info from the node labels now.
howardjohn
added a commit
to howardjohn/istio
that referenced
this pull request
Jan 12, 2020
* istio#16223 * istio#16272 * istio#16187 * istio#16466 * istio#16634 * istio#16594 * istio#16666 * istio#16483 * istio#16820 * istio#16842 * istio#16852 * istio#16835 * istio#16863 * istio#16892 * istio#16991 * istio#16957 * istio#17013 * istio#17134 * istio#17155 * istio#17235 * istio#17342 * istio#17477 * istio#17615 * istio#17334 * istio#17708 * istio#17737 * Fix injection template * Fix quoting * Fix test values * Add accidentally deleted affinity
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.