Use standard_types in templates (allow ipaddress, dns, uri, email in template). by guptasu · Pull Request #1993 · istio/istio

guptasu · 2017-12-06T10:39:59Z

What this PR does / why we need it:

This PR contains following changes.

use standard_types inside templates
Reviewer (@geeknoid ) attention needed : define type alias inside pkg/adapter/standardTypes.go. Adapters will get the aliased type instead of string for IP_ADDRESS, EMAIL, URI and DNS_NAME
Reviewer (@ozevren PTAL) attention needed : Support the above 4 types (IP_ADDRESS, URI and DNS_NAME) inside the il/compiler/compiler.go
Reviewer attention needed: Due to type aliasing there is type assertion and conversion inside the generated code. PTAL https://github.com/istio/istio/compare/master...guptasu:useStdTypes2?expand=1#diff-aecbd97d3bcb64dd8d2a900a893b3e39

Other Changes

Remove the original hack that was using []byte for IP_ADDRESS
stop supporting google.protobuf.Timestamp and google.protobuf.Duration in all existing Templates.

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

Special notes for your reviewer:

Release note:

none

istio-testing · 2017-12-06T10:40:00Z

@guptasu: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2017-12-06T11:36:36Z

Codecov Report

Merging #1993 into master will decrease coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1993      +/-   ##
==========================================
- Coverage   79.78%   79.51%   -0.27%     
==========================================
  Files          71       66       -5     
  Lines        6758     6303     -455     
==========================================
- Hits         5392     5012     -380     
+ Misses       1089     1045      -44     
+ Partials      277      246      -31

Flag	Coverage Δ
#broker	`46.09% <ø> (-1.18%)`	⬇️
#pilot	`80.57% <ø> (-0.2%)`	⬇️
#security	`92.03% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
broker/pkg/version/version.go
pilot/platform/kube/inject/inject.go
pilot/platform/kube/inject/configmap.go
pilot/platform/kube/inject/http.go
pilot/platform/kube/inject/initializer.go
pilot/platform/kube/admit/admit.go	`55.3% <0%> (+0.05%)`	⬆️

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 af9cf39...dd24e15. Read the comment docs.

geeknoid

We probably should update the template dev guide along with this PR.

geeknoid · 2017-12-06T12:31:14Z

mixer/pkg/adapter/standardTypes.go

Can we use url.URL?

As discussed offline, we should do this but it needs work on the evaluator. For now I have marked the corresponding types inside mixer/v1/template/standard_types.proto as DO NOT USE. I will leave the rest of the code as is so that we have the tests and everything ready to be used once the evaluator code is ready.

geeknoid · 2017-12-06T12:31:30Z

mixer/pkg/adapter/standardTypes.go

Can we use mail.Address?

As discussed offline, we should do this but it needs work on the evaluator. For now I have marked the corresponding types inside mixer/v1/template/standard_types.proto as DO NOT USE. I will leave the rest of the code as is so that we have the tests and everything ready to be used once the evaluator code is ready.

geeknoid · 2017-12-06T12:34:46Z

mixer/template/sample/template.gen_test.go

Should make this a valid address so validations will succeed here (if we use actual type mail.Address

Will leave this as is since we are not using the mail.Address. And as soon as we use mail.Address, this test should fail.

geeknoid · 2017-12-06T12:34:59Z

mixer/template/sample/template.gen_test.go

Similarly, this should be a valid URL.

Will leave this as is since we are not using the url.URL. And as soon as we use url.URL, this test should fail.

geeknoid

/lgtm
/approve

istio-merge-robot · 2017-12-06T20:36:05Z

/lgtm cancel //PR changed after LGTM, removing LGTM. @geeknoid @guptasu

guptasu

@geeknoid PTAL at the changes to the template.md.

guptasu · 2017-12-06T20:05:31Z

mixer/pkg/adapter/standardTypes.go

As discussed offline, we should do this but it needs work on the evaluator. For now I have marked the corresponding types inside mixer/v1/template/standard_types.proto as DO NOT USE. I will leave the rest of the code as is so that we have the tests and everything ready to be used once the evaluator code is ready.

guptasu · 2017-12-06T20:05:33Z

mixer/pkg/adapter/standardTypes.go

As discussed offline, we should do this but it needs work on the evaluator. For now I have marked the corresponding types inside mixer/v1/template/standard_types.proto as DO NOT USE. I will leave the rest of the code as is so that we have the tests and everything ready to be used once the evaluator code is ready.

guptasu · 2017-12-06T20:06:43Z

mixer/template/sample/template.gen_test.go

Will leave this as is since we are not using the mail.Address. And as soon as we use mail.Address, this test should fail.

guptasu · 2017-12-06T20:06:55Z

mixer/template/sample/template.gen_test.go

Will leave this as is since we are not using the url.URL. And as soon as we use url.URL, this test should fail.

geeknoid · 2017-12-06T22:07:09Z

/lgtm
/approve

istio-merge-robot · 2017-12-06T22:07: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.

Details

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 · 2017-12-06T22:08:17Z

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

istio-merge-robot · 2017-12-06T23:18:00Z

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

istio-testing added the do-not-merge/release-note-label-needed label Dec 6, 2017

googlebot added the cla: yes label Dec 6, 2017

istio-merge-robot added the PostSubmit Failed/Contact Oncall label Dec 6, 2017

guptasu requested review from douglas-reid, geeknoid, mandarjog and ozevren December 6, 2017 10:49

guptasu changed the title ~~Use standard types in templates.~~ Use template/standard_types in templates (Allow IpAddress, DNS, Uri, Email in template). Dec 6, 2017

guptasu changed the title ~~Use template/standard_types in templates (Allow IpAddress, DNS, Uri, Email in template).~~ Use standard_types in templates (Allow IpAddress, DNS, Uri, Email in template). Dec 6, 2017

guptasu changed the title ~~Use standard_types in templates (Allow IpAddress, DNS, Uri, Email in template).~~ Use standard_types in templates (Allow ipaddress, dns, uri, email in template). Dec 6, 2017

guptasu changed the title ~~Use standard_types in templates (Allow ipaddress, dns, uri, email in template).~~ Use standard_types in templates (allow ipaddress, dns, uri, email in template). Dec 6, 2017

istio-testing added release-note-none and removed do-not-merge/release-note-label-needed labels Dec 6, 2017

geeknoid reviewed Dec 6, 2017

View reviewed changes

istio-merge-robot added PostSubmit Failed/Contact Oncall and removed PostSubmit Failed/Contact Oncall labels Dec 6, 2017

geeknoid reviewed Dec 6, 2017

View reviewed changes

istio-testing assigned geeknoid Dec 6, 2017

istio-testing added the lgtm label Dec 6, 2017

istio-merge-robot added the approved label Dec 6, 2017

guptasu added 3 commits December 6, 2017 12:35

Use standard types in templates.

a8f2366

Make linter happy

0583f59

Fix template.md documentation.

dd24e15

guptasu force-pushed the useStdTypes2 branch from c9a103e to dd24e15 Compare December 6, 2017 20:35

istio-merge-robot removed the lgtm label Dec 6, 2017

guptasu commented Dec 6, 2017

View reviewed changes

istio-merge-robot removed the PostSubmit Failed/Contact Oncall label Dec 6, 2017

istio-testing added the lgtm label Dec 6, 2017

istio-merge-robot added the PostSubmit Failed/Contact Oncall label Dec 6, 2017

guptasu removed the PostSubmit Failed/Contact Oncall label Dec 6, 2017

istio-merge-robot added the PostSubmit Failed/Contact Oncall label Dec 6, 2017

geeknoid merged commit 537f40b into istio:master Dec 6, 2017

yangminzhu mentioned this pull request Oct 18, 2018

rbac: update envoy sha to support connection.sni as service role constraint #9344

Merged

Conversation

guptasu commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Dec 6, 2017

Uh oh!

codecov bot commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

geeknoid left a comment

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!

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!

geeknoid left a comment

Choose a reason for hiding this comment

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

guptasu left a comment

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!

geeknoid commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

istio-merge-robot commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

guptasu commented Dec 6, 2017 •

edited

Loading

codecov bot commented Dec 6, 2017 •

edited

Loading