Skip to content
This repository has been archived by the owner on Jan 16, 2022. It is now read-only.

destination.ip not sent as []byte #112

Closed
douglas-reid opened this issue Sep 20, 2017 · 14 comments
Closed

destination.ip not sent as []byte #112

douglas-reid opened this issue Sep 20, 2017 · 14 comments
Assignees
Milestone

Comments

@douglas-reid
Copy link

Expected to receive attribute destination.ip as a []byte. Instead, received string.

Example from attribute bag dump in latest Mixer:

destination.ip                : 10.56.2.185
source.ip                     : [10 150 0 5]

Note the key difference between the way in which source.ip and destination.ip are received.

@douglas-reid douglas-reid added this to the Istio 0.2 milestone Sep 20, 2017
@qiwzhang
Copy link
Contributor

It could not be.

They are switched to use Bytes in the same PR.
Here is the code: https://github.com/istio/proxy/blob/master/src/envoy/mixer/mixer_control.cc

Here is the PR: istio/proxy@0561c1d#diff-01d13090008b70b5a4e706e9babc542a

I need more evident.

@douglas-reid
Copy link
Author

@qiwzhang Running from istio/istio HEAD.

PILOT_TAG="e5681371e971d20b89823e55e394b2f773e7f07a"
PROXY_TAG="c7785bd97bd3d1610bafdda9ceaa3a061a641bad"

I then just dumped logs. Maybe destination.ip isn't being sent by the proxy, but generated elsewhere?

@kyessenov
Copy link
Contributor

@kyessenov
Copy link
Contributor

@qiwzhang which IP does your filter use? Service IP (external) or pod IP (internal)?

@qiwzhang
Copy link
Contributor

@kyessenov

Proxy is using

upstreamHost->address()->ip();

I guess it is the service host ip in the cluster manager. Pilot set it.

@qiwzhang
Copy link
Contributor

Definitely caused by https://github.com/istio/pilot/blob/master/proxy/envoy/mixer.go#L128

In the Tcp Check, Proxy extracts it from Envoy config, which is string.
In the Tcp Report, Proxy over write it. which is BYTES.

I guess, Proxy should not just blindly copy the string. It should check ".ip" suffix, and convert it to bytes when copying from Mixer config

@qiwzhang
Copy link
Contributor

Ultimate solution is to allow Pilot to write attribute type for the mesh attributes. Now it only supports string type. Leave this as 0.3 feature.

@douglas-reid
Copy link
Author

Are we sure? I haven't debugged fully. It might be related to issues with:

https://github.com/istio/mixer/blob/master/testdata/configroot/scopes/global/subjects/global/rules.yml#L22

@qiwzhang
Copy link
Contributor

Filed https://github.com/istio/proxy/issues/483 to fix this.

@qiwzhang qiwzhang reopened this Sep 21, 2017
@qiwzhang
Copy link
Contributor

@douglas-reid assigned it to you so you can debug it first. If it is caused by https://github.com/istio/pilot/blob/master/proxy/envoy/mixer.go#L128. I have another issue to cover it. so you can close it.

@qiwzhang qiwzhang assigned douglas-reid and unassigned qiwzhang and geeknoid Sep 21, 2017
@douglas-reid
Copy link
Author

It looks like it isn't coming from the mixer attribute derivation stuff:

I0923 01:07:39.430774       1 grpcServer.go:152] Attribute Bag:
context.protocol              : http
destination.ip                : 10.56.2.185
destination.service           : productpage.default.svc.cluster.local
destination.uid               : kubernetes://productpage-v1-2488187180-wzkkm.default
quota.amount                  : 1
quota.name                    : RequestCount
request.headers               : map[x-b3-traceid:0000d392e006806f :authority:75.22.199.35.bc.googleusercontent.com x-forwarded-proto:http x-ot-span-context:0000d392e006806f;0000d392e006806f;0000000000000000 x-b3-sampled:1 x-envoy-expected-rq-timeout-ms:15000 x-request-id:c9fbec6f-6202-98e6-838b-944fc6661857 x-forwarded-for:10.150.0.5 pragma:no-cache accept-charset:iso-8859-1,utf-8;q=0.9,*;q=0.1 x-b3-spanid:0000d392e006806f x-envoy-internal:true :path:/login accept-language:en :method:GET content-length:0 accept:image/gif, image/x-xbitmap, image/jpeg, image/pjpeg, image/png, */* user-agent:Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GoogleSecurityScanner; go/SecopsSecurityScan 1.337)]
request.host                  : 75.22.199.35.bc.googleusercontent.com
request.method                : GET
request.path                  : /login
request.scheme                : http
request.time                  : 2017-09-23 01:07:39.429065888 +0000 UTC
request.useragent             : Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; GoogleSecurityScanner; go/SecopsSecurityScan 1.337)
source.ip                     : [10 56 3 154]
source.port                   : 53742
source.uid                    : kubernetes://istio-ingress-3820240895-vsxdq.istio-system
---
destination.labels            : map[app:productpage pod-template-hash:2488187180 version:v1]
destination.namespace         : default
destination.service           : productpage.default.svc.cluster.local
destination.serviceAccount    : default
source.labels                 : map[istio:ingress pod-template-hash:3820240895]
source.namespace              : istio-system
source.service                : ingress.istio-system.svc.cluster.local
source.serviceAccount         : istio-ingress-service-account

Everything above the --- is from the original proto bag. So, hopefully, the fix you guys have identified is ready to go soon.

@qiwzhang
Copy link
Contributor

The issue
Allow Pilot to pass any type of mesh attributes to Mixer #483
is not easy to fix. It requires both Pilot and Proxy code change. It will be 0.2 after.

@geeknoid
Copy link

geeknoid commented Sep 26, 2017 via email

@qiwzhang
Copy link
Contributor

It is covered by https://github.com/istio/proxy/issues/483

istio-merge-robot pushed a commit to istio/old_mixer_repo that referenced this issue Sep 29, 2017
… istio ingress (#1349)

Automatic merge from submit-queue

Add flag to control source/origin lookups when destination.service is istio ingress

As noted in istio/istio#879, the source information that the Mixer receives (`source.ip` values) are problematic for attempting to lookup (in-cluster). This PR addresses provides a workaround for compounding that issue within Mixer by preventing lookup of source/origin information in the kubernetes adapter when `destination.service == ingress.istio-system.svc.cluster.local`.

This behavior is configurable, through options in the config params passed into the adapter builder.

This PR also cleans up a related bit of the kubernetes adapter behavior related to handling and generation of IP_ADDRESS values. As noted in istio/old_mixerclient_repo#112 (and also in istio/proxy#483), handling of IP_ADDRESS values is completely inconsistent throughout Istio. The changes here remove any inconsistencies on the part of the kubernetes adapter.

It also fixes the incorrect IP-related config in `testdata/configroot`.

**Release note**:

```release-note-none
NONE
```
rvkubiak pushed a commit to rvkubiak/mixer that referenced this issue Oct 10, 2017
… istio ingress (istio#1349)

Automatic merge from submit-queue

Add flag to control source/origin lookups when destination.service is istio ingress

As noted in istio/istio#879, the source information that the Mixer receives (`source.ip` values) are problematic for attempting to lookup (in-cluster). This PR addresses provides a workaround for compounding that issue within Mixer by preventing lookup of source/origin information in the kubernetes adapter when `destination.service == ingress.istio-system.svc.cluster.local`.

This behavior is configurable, through options in the config params passed into the adapter builder.

This PR also cleans up a related bit of the kubernetes adapter behavior related to handling and generation of IP_ADDRESS values. As noted in istio/old_mixerclient_repo#112 (and also in istio/proxy#483), handling of IP_ADDRESS values is completely inconsistent throughout Istio. The changes here remove any inconsistencies on the part of the kubernetes adapter.

It also fixes the incorrect IP-related config in `testdata/configroot`.

**Release note**:

```release-note-none
NONE
```
mandarjog pushed a commit to mandarjog/core3 that referenced this issue Oct 27, 2017
… istio ingress (#1349)

Automatic merge from submit-queue

Add flag to control source/origin lookups when destination.service is istio ingress

As noted in istio/istio#879, the source information that the Mixer receives (`source.ip` values) are problematic for attempting to lookup (in-cluster). This PR addresses provides a workaround for compounding that issue within Mixer by preventing lookup of source/origin information in the kubernetes adapter when `destination.service == ingress.istio-system.svc.cluster.local`.

This behavior is configurable, through options in the config params passed into the adapter builder.

This PR also cleans up a related bit of the kubernetes adapter behavior related to handling and generation of IP_ADDRESS values. As noted in istio/old_mixerclient_repo#112 (and also in istio/proxy#483), handling of IP_ADDRESS values is completely inconsistent throughout Istio. The changes here remove any inconsistencies on the part of the kubernetes adapter.

It also fixes the incorrect IP-related config in `testdata/configroot`.

**Release note**:

```release-note-none
NONE
```

Former-commit-id: 20fbea5bee6d00810d8c501d8de16d6a7d80e593
mandarjog pushed a commit to mandarjog/istio that referenced this issue Oct 30, 2017
… istio ingress (istio#1349)

Automatic merge from submit-queue

Add flag to control source/origin lookups when destination.service is istio ingress

As noted in istio#879, the source information that the Mixer receives (`source.ip` values) are problematic for attempting to lookup (in-cluster). This PR addresses provides a workaround for compounding that issue within Mixer by preventing lookup of source/origin information in the kubernetes adapter when `destination.service == ingress.istio-system.svc.cluster.local`.

This behavior is configurable, through options in the config params passed into the adapter builder.

This PR also cleans up a related bit of the kubernetes adapter behavior related to handling and generation of IP_ADDRESS values. As noted in istio/old_mixerclient_repo#112 (and also in istio/proxy#483), handling of IP_ADDRESS values is completely inconsistent throughout Istio. The changes here remove any inconsistencies on the part of the kubernetes adapter.

It also fixes the incorrect IP-related config in `testdata/configroot`.

**Release note**:

```release-note-none
NONE
```

Former-commit-id: 5c82322e5377038b89897482e303bde0edf72765
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants