Skip to content

Add connection requested server name attribute to TCP read filter#1843

Merged
hklai merged 18 commits intoistio:release-1.0from
vadimeisenbergibm:add_connection_requested_server_name_attribute
Jul 10, 2018
Merged

Add connection requested server name attribute to TCP read filter#1843
hklai merged 18 commits intoistio:release-1.0from
vadimeisenbergibm:add_connection_requested_server_name_attribute

Conversation

@vadimeisenbergibm
Copy link
Contributor

What this PR does / why we need it:

Adds SNI attribute to the TCP read filter, to be used in telemetry reports and policy checks.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): implements #istio/istio#6810

Release note:

SNI report and check in TCP filter

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 10, 2018
@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 Jul 10, 2018
@istio-testing istio-testing requested review from linsun and lizan July 10, 2018 07:04
builder.AddBool(utils::AttributeName::kConnectionMtls,
check_data->IsMutualTLS());

builder.AddString(utils::AttributeName::kConnectionRequestedServerName,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please check whether the string is empty first? we normally don't send unset values to mixer.

bool GetSourceIpPort(std::string* str_ip, int* port) const override;
bool GetSourceUser(std::string* user) const override;
bool IsMutualTLS() const override;
std::string GetRequestedServerName() const override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to apply the same changes to http filter in src/envoy/http/mixer

@vadimeisenbergibm
Copy link
Contributor Author

@kyessenov I have applied your comments.

@vadimeisenbergibm vadimeisenbergibm changed the title [WIP] Add connection requested server name attribute to TCP read filter Add connection requested server name attribute to TCP read filter Jul 10, 2018
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 10, 2018
Copy link
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 to me, but please get an approval from @qiwzhang and release sign-off from @hklai

@vadimeisenbergibm
Copy link
Contributor Author

@qiwzhang could you please review this PR?

Copy link
Contributor

@hklai hklai left a comment

Choose a reason for hiding this comment

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

/lgtm

@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hklai, vadimeisenbergibm
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmycyj

Assign the PR to them by writing /assign @jimmycyj in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

virtual bool IsMutualTLS() const = 0;

// Get requested server name, SNI in case of TLS
virtual std::string GetRequestedServerName() const = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are cases data is not available, the coding style is to return a bool. It should be:

// return true if data is valid.
bool GetRequestedServerName(std::string* name);

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

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants