Skip to content

Extract service name from request host and cluster name#2548

Merged
istio-testing merged 15 commits intoistio:masterfrom
bianpengyuan:destination-service-name
Dec 14, 2019
Merged

Extract service name from request host and cluster name#2548
istio-testing merged 15 commits intoistio:masterfrom
bianpengyuan:destination-service-name

Conversation

@bianpengyuan
Copy link
Copy Markdown
Contributor

To avoid introducing throw away code to get node information from root context, this change also moves request_info out stream context, which removes computation of request duration and size and use property abi to fetch them from host directly. These changes should be trivial and safe, we could do manual testing to de-risk cherry-pick of this pr to 1.4.

@bianpengyuan bianpengyuan requested a review from a team November 11, 2019 09:17
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 11, 2019
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 11, 2019
Copy link
Copy Markdown
Contributor

@lambdai lambdai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much more elegant than before!
Leave a mixer expert to final approve

Copy link
Copy Markdown
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..

@kyessenov
Copy link
Copy Markdown
Contributor

It appears upstream SSL info is not filled in onLog. I'll need to look further into this.

@kyessenov
Copy link
Copy Markdown
Contributor

The failure seems to be caused by an uninitialized field service_auth_policy in request info struct.

@bianpengyuan
Copy link
Copy Markdown
Contributor Author

Can I get some review on this PR? It would be nice to get this into 1.4 as well, so that we could gain more parity with mixer.

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A couple of small code nits.

@bianpengyuan bianpengyuan added the cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch label Dec 13, 2019
@kyessenov kyessenov changed the title Support extracing service name from request host Extract service name from request host and cluster name Dec 14, 2019
@istio-testing istio-testing merged commit 5a66e29 into istio:master Dec 14, 2019
@istio-testing
Copy link
Copy Markdown
Collaborator

In response to a cherrypick label: #2548 failed to apply on top of branch "release-1.4":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	extensions/common/context.cc
M	extensions/common/context.h
M	extensions/stackdriver/metric/record.cc
M	extensions/stackdriver/stackdriver.cc
A	testdata/metric/client_request_total.yaml.tmpl
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): testdata/metric/client_request_total.yaml.tmpl deleted in HEAD and modified in support extracing service name from request host. Version support extracing service name from request host of testdata/metric/client_request_total.yaml.tmpl left in tree.
Auto-merging extensions/stackdriver/stackdriver.cc
Auto-merging extensions/stackdriver/metric/record.cc
CONFLICT (content): Merge conflict in extensions/stackdriver/metric/record.cc
Auto-merging extensions/common/context.h
Auto-merging extensions/common/context.cc
Patch failed at 0001 support extracing service name from request host

bianpengyuan added a commit to bianpengyuan/proxy that referenced this pull request Dec 30, 2019
* support extracing service name from request host

* service_name:port

* s/namespace_pos/name_pos

* update

* lint

* add initialization to service auth policy

* format

* use absl time

* update test

* address comment
bianpengyuan added a commit to bianpengyuan/proxy that referenced this pull request Dec 31, 2019
* support extracing service name from request host

* service_name:port

* s/namespace_pos/name_pos

* update

* lint

* add initialization to service auth policy

* format

* use absl time

* update test

* address comment
istio-testing pushed a commit that referenced this pull request Jan 3, 2020
* support extracing service name from request host

* service_name:port

* s/namespace_pos/name_pos

* update

* lint

* add initialization to service auth policy

* format

* use absl time

* update test

* address comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherrypick/release-1.4 Set this label on a PR to auto-merge it to the release-1.4 branch cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants