Add constructor / equality checks for dns, email and uri types #4156

ozevren · 2018-03-09T22:49:31Z

No description provided.

geeknoid · 2018-03-09T23:17:05Z

mixer/pkg/il/runtime/externs.go

+	if in == "" {
+		return "", errors.New("error converting string to url: empty string")
+	}
+	_, err := url.Parse(in)


Combine with next line.

geeknoid · 2018-03-09T23:17:14Z

mixer/pkg/il/runtime/externs.go

+	if err != nil {
+		return "", fmt.Errorf("error converting string to url '%s': '%v'", in, err)
+	}
+	return in, err


return in, nil

geeknoid · 2018-03-09T23:18:08Z

mixer/pkg/il/runtime/externs.go

+	}
+
+	// Try to apply as much normalization logic as possible.
+	if url1.Scheme != url2.Scheme {


SHould this be a case-insensitive check?

Great catch. Thanks. Added support.

geeknoid · 2018-03-09T23:18:24Z

mixer/pkg/il/runtime/externs.go

+		return false, nil
+	}
+
+	if url1.Scheme == "http" || url1.Scheme == "https" {


HTTP and HTTPS?

geeknoid · 2018-03-09T23:18:57Z

mixer/pkg/il/runtime/externs.go

+		url1.Host = url2.Host
+	}
+
+	return url1.String() == url2.String(), nil


Case insensitive?

The case sensitive parts are (for http/https) are normalized within the if block. Other parts are case sensitive.

codecov · 2018-03-09T23:32:18Z

Codecov Report

Merging #4156 into master will increase coverage by 1%.
The diff coverage is 96%.

@@          Coverage Diff           @@
##           master   #4156   +/-   ##
======================================
+ Coverage      73%     73%   +1%     
======================================
  Files         313     313           
  Lines       28829   28879   +50     
======================================
+ Hits        20936   20997   +61     
+ Misses       6570    6559   -11     
  Partials     1323    1323

Impacted Files	Coverage Δ
mixer/pkg/il/compiler/compiler.go	`90% <100%> (+3%)`	⬆️
mixer/pkg/il/runtime/externs.go	`97% <95%> (ø)`	⬇️
mixer/adapter/solarwinds/log_handler.go	`50% <0%> (-7%)`	⬇️
pilot/pkg/config/monitor/monitor.go	`69% <0%> (-5%)`	⬇️
mixer/adapter/circonus/circonus.go	`67% <0%> (-4%)`	⬇️
mixer/adapter/kubernetesenv/cache.go	`83% <0%> (-1%)`	⬇️
mixer/adapter/prometheus/server.go	`96% <0%> (ø)`	⬇️
mixer/adapter/denier/denier.go	`93% <0%> (ø)`	⬇️
mixer/adapter/stdio/zap.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/memquota.go	`100% <0%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d7752f...d4d88ed. Read the comment docs.

geeknoid · 2018-03-10T00:26:04Z

mixer/pkg/il/runtime/externs.go

+	_, domain := getEmailParts(a.Address)
+
+	_, err = externDNSName(domain)
+	if err != nil {


Combine with previous line

+ dnsName() method converts a string to a dns name. The conversion is mostly about validation and does not change the actual format of the data. + dnsNameEqual() compares dns names. It first normalizes the names to a lookup format and canonicalizes by optionally eliding the trailing '.'

+ email() method converts a string to an e-mail. The conversion is mostly about validation and does not change the actual format of the data. + emailEqual() compares emails. It first normalizes the names using Go's mail libraries, then compares the local part in a case-sensitive manner, and compares the domain part using dnsNameEqual().

+ uri() method converts a string to a uri. The conversion is pretty liberal, is only about validation and does not change the actual format of the data. + uriEqual() compares uris. It first normalizes the strings using Go's uri libraries, then compares the hostname using dnsEqual() if the protocol is http or https. Otherwise, full case-sensitive comparison is used.

mandarjog · 2018-03-10T21:06:21Z

/test e2e-simple

The evaluator can use both with interface{} and string types for e-mail, dns name and uri. However, updating these to use string internally allows these types to make use of any future performance benefits when dealing with strings.

geeknoid · 2018-03-11T22:47:47Z

/lgtm

istio-merge-robot · 2018-03-11T22:47:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geeknoid

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

Needs approval from an approver in each of these OWNERS Files:

~~mixer/OWNERS~~ [geeknoid]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

istio-merge-robot · 2018-03-11T22:49:08Z

/test all [submit-queue is verifying that this PR is safe to merge]

ozevren · 2018-03-12T03:09:18Z

/test istio-unit-tests

istio-merge-robot · 2018-03-12T03:17:12Z

/test all [submit-queue is verifying that this PR is safe to merge]

istio-merge-robot · 2018-03-12T03:38:17Z

Automatic merge from submit-queue.

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (istio#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (istio#1938) a32a587 Add a check cache test for string map sub keys (istio#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (istio#4190) 36809d80a fuzz: coverage profile generation instructions. (istio#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (istio#4075) 05c0d52d3 build: use clang-6.0. (istio#4168) 01f403ec4 thrift_proxy: introduce header transport (istio#4082) 564d256fb tcp: allow connection pool callers to store protocol state (istio#4131) 3e1d643b9 configs: match latest API changes (istio#4185) bc6a10c2f Fix wrong mock function name. (istio#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (istio#4182) 3d1325e89 Converting envoy configs to V2 (istio#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (istio#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (istio#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (istio#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (istio#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (istio#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (istio#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (istio#4157) 71152b710 ratelimit: Add ratelimit custom response headers (istio#4015) 306287418 ssl: make Ssl::Connection const everywhere (istio#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (istio#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (istio#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (istio#4130) 8c189a552 Make over provisioning factor configurable (istio#4003) 6c08fb43c Making test less flaky (istio#4149) 592775b7b fuzz: bare bones HCM fuzzer. (istio#4118) For istio#7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

…). (#8161) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: 2656f34 Update Envoy SHA to latest with MetricImpl optimizations (release-1.0). (#1939) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/istio/proxy: c9d2230 Update Envoy SHA to latest with MetricImpl optimizations. (#1938) a32a587 Add a check cache test for string map sub keys (#1931) Pulling the following changes from github.com/envoyproxy/envoy: c1cc68dda stats: refactoring MetricImpl without strings (#4190) 36809d80a fuzz: coverage profile generation instructions. (#4193) ba40cc933 upstream: rebuild cluster when health check config is changed (#4075) 05c0d52d3 build: use clang-6.0. (#4168) 01f403ec4 thrift_proxy: introduce header transport (#4082) 564d256fb tcp: allow connection pool callers to store protocol state (#4131) 3e1d643b9 configs: match latest API changes (#4185) bc6a10c2f Fix wrong mock function name. (#4187) e994c1c0b Bump yaml-cpp so it builds with Visual Studio 15.8 (#4182) 3d1325e89 Converting envoy configs to V2 (#2957) 8d0680feb Add timestamp to HealthCheckEvent definition (#4119) 497efb95b server: handle non-EnvoyExceptions safely if thrown in constructor. (#4173) 6d6fafdb3 config: strengthen validation for gRPC config sources. (#4171) 132302caf fuzz: reduce log level when running under fuzz engine. (#4161) 7c04ac255 test: fix V6EmptyOptions in coverage with IPv6 enable (#4155) 1b2219bd7 ci: remove deprecated bazel --batch option (#4166) 2db6a4ce1 ci: update clang to version 6.0 in the Ubuntu build image. (#4157) 71152b710 ratelimit: Add ratelimit custom response headers (#4015) 306287418 ssl: make Ssl::Connection const everywhere (#4179) 706e26238 Handle validation of non expiring tokens in jwt_authn filter (#4007) f06e9588a fuzz: tag trivial fuzzers with no_fuzz. (#4156) 27fb1d353 thrift_proxy: add service name matching to router implementation (#4130) 8c189a552 Make over provisioning factor configurable (#4003) 6c08fb43c Making test less flaky (#4149) 592775b7b fuzz: bare bones HCM fuzzer. (#4118) For #7912. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ozevren requested review from geeknoid and a team March 9, 2018 22:49

googlebot added the cla: yes label Mar 9, 2018

geeknoid reviewed Mar 9, 2018

View reviewed changes

geeknoid approved these changes Mar 10, 2018

View reviewed changes

ozben added 5 commits March 9, 2018 16:54

Accommodate CR feedback and make linter happy.

9be7fbb

More linter fixes.

27c3d45

istio-testing assigned geeknoid Mar 11, 2018

istio-testing added the lgtm label Mar 11, 2018

istio-merge-robot added the approved label Mar 11, 2018

istio-merge-robot merged commit 84c25f8 into istio:master Mar 12, 2018

ozevren deleted the fixit1 branch March 14, 2018 22:47

This was referenced Aug 23, 2018

Update Proxy SHA to latest with MetricImpl optimizations. #8160

Merged

Update Proxy SHA to latest with MetricImpl optimizations (release-1.0). #8161

Merged

Add constructor / equality checks for dns, email and uri types #4156

Add constructor / equality checks for dns, email and uri types #4156

Uh oh!

Conversation

ozevren commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog commented Mar 10, 2018

Uh oh!

geeknoid commented Mar 11, 2018

Uh oh!

istio-merge-robot commented Mar 11, 2018

Uh oh!

istio-merge-robot commented Mar 11, 2018

Uh oh!

ozevren commented Mar 12, 2018

Uh oh!

istio-merge-robot commented Mar 12, 2018

Uh oh!

istio-merge-robot commented Mar 12, 2018

Uh oh!

Uh oh!

codecov bot commented Mar 9, 2018 •

edited

Loading