Update Bookinfo code by vadimeisenbergibm · Pull Request #6842 · istio/istio

vadimeisenbergibm · 2018-07-04T11:15:15Z

Today, after TLS external services are supported without necessary TLS origination, https://istio.io/blog/2018/egress-https/ must be rewritten to show access with TLS origination and without it. The code of the details must be changed accordingly.

Istio now can handle HTTPS (originated by the application). The decision to use HTTP or HTTPS now is unrelated to the fact that the appliation runs with Istio or without it.

codecov · 2018-07-04T11:43:24Z

Codecov Report

Merging #6842 into master will increase coverage by 1%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #6842   +/-   ##
======================================
+ Coverage      71%     71%   +1%     
======================================
  Files         369     369           
  Lines       31977   32017   +40     
======================================
+ Hits        22461   22532   +71     
+ Misses       8597    8558   -39     
- Partials      919     927    +8

Impacted Files	Coverage Δ
mixer/adapter/cloudwatch/handler.go	`87% <0%> (-3%)`	⬇️
mixer/adapter/rbac/rbacStore.go	`76% <0%> (-3%)`	⬇️
pilot/pkg/model/egress_rules.go	`95% <0%> (-2%)`	⬇️
mixer/adapter/bypass/bypass.go	`62% <0%> (-1%)`	⬇️
mixer/adapter/circonus/circonus.go	`71% <0%> (ø)`	⬇️
mixer/adapter/servicecontrol/checkprocessor.go	`80% <0%> (ø)`	⬇️
mixer/adapter/memquota/keys.go	`100% <0%> (ø)`	⬆️
mixer/adapter/stdio/zap.go	`100% <0%> (ø)`	⬆️
mixer/adapter/noop/noop.go	`100% <0%> (ø)`	⬆️
mixer/adapter/solarwinds/solarwinds.go	`0% <0%> (ø)`	⬆️
... and 9 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 405f47f...6bd480d. Read the comment docs.

rshriram · 2018-07-04T16:55:54Z

samples/bookinfo/kube/bookinfo-details-v2.yaml

        - containerPort: 9080
        env:
-        - name: WITH_ISTIO
+        - name: DO_NOT_USE_TLS_FOR_EXTERNAL_SERVICE_CALLS


Shouldn’t we be setting this to false always to show SNI routing ?
Also this name is too big. I suggest use_https_googleapis which is short n sweet

@rshriram I propose to use this example to demonstrate both HTTP with TLS origination and HTTPS, and to compare between them. The idea is to demonstrate an organization that requires monitoring of the content and requires not to use TLS (HTTP with TLS origination). The environment variable simulates some general policy variable to enforce the policy of the organization. Maybe a simpler name would be DO_NOT_USE_TLS or DO_NOT_ENCRYPT.
Then I will compare this approach with HTTPS - an organization that does not have strict monitoring requirements and is OK to perform monitoring by SNI.

frankbu · 2018-07-06T19:26:49Z

samples/bookinfo/src/details/details.rb

-    # WITH_ISTIO means that the details app runs with an Istio sidecar injected.
+    # DO_NOT_ENCRYPT means that the app must access external services without TLS,
+    # using unencrypted traffic protocols like HTTP. The TLS origination will be
+    # performed if needed, elsewhere, e.g. by a sidecar proxy.


This description seems too general and confusing. If I understand correctly, the flag is simply used to configure the code to use either HTTP or HTTPS when calling the external service, with HTTPS the default. I would suggest simply explaining what the flag does:

# DO_NOT_ENCRYPT is used to configure the details service to use either HTTP (true) or HTTPS (false, default) # when calling the external service to retrieve the book information.

IMO, explaining that a sidecar may perform TLS origination is not helpful here, that belongs in the docs.

frankbu · 2018-07-06T19:26:57Z

samples/bookinfo/src/details/details.rb

-    # Without the Istio sidecar injected, the TLS origination must be performed by the `http` object,
-    # by setting `use_ssl` field to true.
-    unless ENV['WITH_ISTIO'] === 'true' then
+    # If this environment variable is false, the app must use TLS, e.g. HTTPS, to access


Unless this environment variable is set to true, the app will use TLS (HTTPS) to access

niaquinto/gradle has been removed from docker

* Switch back to standard ratings image * Remove unneeded file name * Update Writer flush in productpage.py * Allow services domain to be configurable * This is needed to run on CloudFoundry

googlebot · 2018-07-10T17:09:49Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

frankbu

/lgtm

istio-testing · 2018-07-10T17:15:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frankbu, vadimeisenbergibm

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:

~~samples/OWNERS~~ [frankbu,vadimeisenbergibm]

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

istio-testing · 2018-07-10T18:24:53Z

New changes are detected. LGTM label has been removed.

vadimeisenbergibm · 2018-07-10T20:07:54Z

/test e2e-mixer-no_auth

vadimeisenbergibm · 2018-07-10T20:08:05Z

/test e2e-bookInfo-envoyv2-v1alpha3

vadimeisenbergibm · 2018-07-10T20:13:23Z

/test e2e-mixer-no_auth

istio-testing · 2018-07-10T20:58:25Z

@vadimeisenbergibm: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/e2e-bookInfoTests-v1alpha3.sh	`6bd480d`	link	`/test e2e-bookInfo-envoyv2-v1alpha3`

Details

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. I understand the commands that are listed here.

andraxylia · 2018-07-10T21:20:25Z

@vadimeisenbergibm can we do this on release-1.0 branch?

That will get merged periodically.

vadimeisenbergibm · 2018-07-10T21:21:43Z

@andraxylia yes, let me resubmit the PR to release-1.0 branch.

vadimeisenbergibm · 2018-07-10T21:31:30Z

Resubmitted as #6988 to release-1.0.

WITH_ISTIO -> DO_NOT_USE_TLS_FOR_EXTERNAL_SERVICE_CALLS

7ceca14

Istio now can handle HTTPS (originated by the application). The decision to use HTTP or HTTPS now is unrelated to the fact that the appliation runs with Istio or without it.

googlebot added the cla: yes label Jul 4, 2018

istio-testing requested review from frankbu and rshriram July 4, 2018 11:15

istio-testing added the approved label Jul 4, 2018

vadimeisenbergibm removed the request for review from rshriram July 4, 2018 11:15

vadimeisenbergibm added 3 commits July 4, 2018 15:37

set DO_NOT_USE_TLS_FOR_EXTERNAL_SERVICE_CALLS to true for details v2

f69002c

return a yaml separator

a17c246

remove redundant dash

7a3b7f4

rshriram approved these changes Jul 4, 2018

View reviewed changes

DO_NOT_USE_TLS_FOR_EXTERNAL_SERVICE_CALLS -> DO_NOT_ENCRYPT

c9e7247

frankbu reviewed Jul 6, 2018

View reviewed changes

vadimeisenbergibm and others added 3 commits July 10, 2018 15:03

rephrase comments about DO_NOT_ENCRYPT

45aa880

replace niaquinto/gradle with library/gradle

3faa755

niaquinto/gradle has been removed from docker

Use new bookinfo

4819bbf

* Switch back to standard ratings image * Remove unneeded file name * Update Writer flush in productpage.py * Allow services domain to be configurable * This is needed to run on CloudFoundry

googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Jul 10, 2018

frankbu approved these changes Jul 10, 2018

View reviewed changes

istio-testing assigned frankbu Jul 10, 2018

istio-testing added the lgtm label Jul 10, 2018

zachgersh mentioned this pull request Jul 10, 2018

Bookinfo changes for 0.8 #5483

Closed

update bookinfo images to 1.7.0 version

6bd480d

istio-testing removed the lgtm label Jul 10, 2018

vadimeisenbergibm changed the base branch from master to release-1.0 July 10, 2018 20:10

vadimeisenbergibm changed the base branch from release-1.0 to master July 10, 2018 20:12

rshriram changed the title ~~Update Istio details v2 to optionally use HTTP~~ Update Bookinfo code Jul 10, 2018

andraxylia approved these changes Jul 10, 2018

View reviewed changes

vadimeisenbergibm mentioned this pull request Jul 10, 2018

Update bookinfo code #6988

Merged

vadimeisenbergibm closed this Jul 10, 2018

rshriram mentioned this pull request Jul 11, 2018

Updates to bookinfo services build to keep it alive #6488

Closed

Conversation

vadimeisenbergibm commented Jul 4, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

rshriram Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

vadimeisenbergibm Jul 4, 2018

Choose a reason for hiding this comment

Uh oh!

frankbu Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

frankbu Jul 6, 2018

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 10, 2018

Uh oh!

frankbu left a comment

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Jul 10, 2018

Uh oh!

istio-testing commented Jul 10, 2018

Uh oh!

vadimeisenbergibm commented Jul 10, 2018

Uh oh!

vadimeisenbergibm commented Jul 10, 2018

Uh oh!

vadimeisenbergibm commented Jul 10, 2018

Uh oh!

istio-testing commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andraxylia commented Jul 10, 2018

Uh oh!

vadimeisenbergibm commented Jul 10, 2018

Uh oh!

vadimeisenbergibm commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov bot commented Jul 4, 2018 •

edited

Loading

istio-testing commented Jul 10, 2018 •

edited

Loading