Action:

• Log into Test Account
• Send Payment for $1 million

Expected:

• Payment rejected as account balance is ~$5k

Actual:

• Payment accepted and account balance is negative ~$995k

For now, you can only send payments between internal accounts. We may want to support sending back to deposit accounts. This requires UI and endpoint changes

There is much duplicated code between BalanceReader and TransactionHistory. If we were to merge the two services, we could reduce the code complexity and the resource footprint of the application.

Currently, the two services both:

• Read every transaction in the database
  • calculate a set of values
  • store the results
• Poll for new transactions
  • update the associated values
• Serve REST requests to retrieve said values

The only difference what value is being calculated:

• BalanceReader: a running account balance
• TransactionHistory: a running list of historical transactions

We could easily merge these two services and simplify the application and codebase. As an example:

AccountsReader

• /accounts/<accountid>/balance GET
• /accounts/<accountid>/transactions GET

The ledgerwriter seems to be allowing sending funds > current balance. Check into what's causing this

Expected Behavior

Continuous Integration builds should take around ~10 minutes to complete.

Actual Behavior

Several builds recently have taken 20 minutes or more: Completed Pull Request Workflows

With the bulk of the time happening in the "Deploy from Source" step. Example: https://github.com/GoogleCloudPlatform/bank-of-anthos/runs/575424795?check_suite_focus=true

https://discuss.istio.io/t/k8s-istio-sidecar-injection-with-other-init-containers/845

istio/istio#12038

When Istio sidecar injection is enabled for the app, and strict mutual TLS is also enabled, the app's init-db container (redis ping) will start before Istio's init container.

This is currently causing errors on a GKE cluster --

$ kubectl logs -n fsi ledgerwriter-5884b5d954-s2nmp init-db-ready
Error: Protocol error, got "\x15" as reply type byte

Although this isn't ideal, I'd recommend moving the "wait for DB" retry logic into the app code / client startup. Keeping the readiness and liveness probes is fine, but for mTLS compability, I recommend adding the "RewriteHTTP" probes to our release Deployments -- https://istio.io/docs/ops/configuration/mesh/app-health-check/#use-annotations-on-pod

• Add new e2e step to actions workflow (#92)
• 1: User can log in with default account and see correct balance
• 2: User is blocked from signing in with bad credentials
• 3: User can create account and see zero balance
• 4: User can deposit funds
  • See balance update
  • See transaction in history
  • See new contact show up
• 5: User can transfer funds
  • See balance update
  • See transaction in history
  • See new contact show up
• 6: User is blocked from sending invalid data
  • Amount > balance
  • Amount <= 0
  • Amount with more than 2 decimal digits
  • Badly formatted routing or account numbers
• 7: User is redirected from /home to /login when not authenticated
• 8: User is redirected from /login and /signup to /home when already authenticated

We don't validate account numbers when you send money to a contact account. We should probably validate in the ledgerwriter, and possibly at the frontend

Currently, you may send payments to any account number. Consider allowing payments only to user accounts that actually exist.

• Use gunicorn instead of bare flask
• pin packages to specific versions

If balancereader or transactionhistory are removed, the frontend crashes. It should still render what it can

Istio 1.4.6 - mTLS is not enabled (when enabled, this often causes readiness/liveness probe failure.)

Events:
  Type     Reason     Age                    From                                                        Message
  ----     ------     ----                   ----                                                        -------
  Normal   Scheduled  4m26s                  default-scheduler                                           Successfully assigned default/balancereader-f56f87c4c-szdgx to gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x
  Normal   Pulled     2m11s (x2 over 4m25s)  kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Container image "redis:alpine" already present on machine
  Normal   Created    2m11s (x2 over 4m24s)  kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Created container init-db-ready
  Normal   Started    2m11s (x2 over 4m24s)  kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Started container init-db-ready
  Normal   Pulled     2m9s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Container image "docker.io/istio/proxyv2:1.4.6" already present on machine
  Normal   Created    2m9s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Created container istio-init
  Normal   Started    2m9s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Started container istio-init
  Normal   Created    2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Created container reader
  Normal   Pulled     2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Container image "gcr.io/megandemo/fsi/balancereader:aae97ea-dirty@sha256:bfe2905ca663f0975ad7b4d0a07c832660f335f9f0287173ea9a61286c9e439e" already present on machine
  Normal   Started    2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Started container reader
  Normal   Pulled     2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Container image "docker.io/istio/proxyv2:1.4.6" already present on machine
  Normal   Created    2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Created container istio-proxy
  Normal   Started    2m8s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Started container istio-proxy
  Warning  Unhealthy  2m7s                   kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Readiness probe failed: HTTP probe failed with statuscode: 503
  Warning  Unhealthy  93s (x7 over 2m3s)     kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Readiness probe failed: HTTP probe failed with statuscode: 503
  Warning  Unhealthy  28s (x2 over 33s)      kubelet, gke-fsi-single-cluster-default-pool-0bdf24ed-6d9x  Liveness probe failed: HTTP probe failed with statuscode: 500

Currently the frontend only shows basic error messages

• Deposit failed
• Transaction failed
• etc

The backend services return human-readable error messages on failures. We could update the frontend to show these informative error messages:

• Deposit failed: invalid account number
• Transaction failed: may not add yourself to contacts
• etc, etc, etc

Use more neutral options for user signup defaults

Currently, the new user signup page defaults to user details that pinpoint the Google Seattle, South Lake Union office. Google is a global company, and this sample app is already being planned to be used in multiple places across the globe.

Suggestion: the location of 'The Charging Bull of Wall Street'

Address:  Bowling Green, New York City
State:  NY
Zip:  10004
Timezone:  GMT -5

Google JSON Style Guide

Google Java Style Guide
Java Doc

TransactionHistory and BalanceReader act as caches to the data in ledger-db. If ledger-db is deleted and restarted, the caches will keep serving old data. They should self-destruct

When I run the cleanup script, some scripts appear to be missing :

./env: line 10: /home/user/bank_of_anthos/bank-of-anthos-scripts/install/common/manage-state.sh: No such file or directory
./env: line 11: load_state: command not found
WORK_DIR set to /home/user/bank_of_anthos/bank-of-anthos-scripts/install/workdir
Updated property [core/project].
./cleanup.sh: line 32: ./connect-hub/cleanup-hub.sh: No such file or directory

Currently, the backend allows contacts without a label. We should update the backend to always save contacts with a label.

The frontend shows that contact/external account labels are optional. If no label is provided then the contact is not saved.

Perhaps someone can add in the dependency on kops being deployed within cloudshell into the README, to save on time debugging what the pre-requisites required actually are.

Installation is trivial :

curl -LO https://github.com/kubernetes/kops/releases/download/$(curl -s https://api.github.com/repos/kubernetes/kops/releases/latest | grep tag_name | cut -d '"' -f 4)/kops-linux-amd64
chmod +x kops-linux-amd64
sudo mv kops-linux-amd64 /usr/local/bin/kops

Description

Current CI uses one self hosed runner with github actions. With each build expected to take ~10 minutes, the queue time can grow really long. It would be helpful to have an additional runner to cut time down when several PR's are opened at once.

Most K8s related customers are using golang as their default language and golang is very picky at the structure of the src code.

Maybe good to have at least one service which is written with golang.

Some services need to share variable data, like LOCAL_ROUTING_NUM and DEFAULT_ACCOUNT_ID. We should consider putting them into config maps that can be mounted into multiple pods

The contacts service currently handles 'contacts' and 'external accounts'. Thus, the name 'contacts service' seems to mismatch with its functionality.

• Consider renaming the service to encapsulate both sets of functionality.
• Consider separating the functionality into 2 services

I was able to deploy this on my on-prem cluster with some simple modifications.

1. I created a namespace (kubectl create ns bank-of-anthos) to make it simple to clean up after, as on-prem clusters aren't quite as easy to build and tear down as gke in the cloud. I then modified the create secret and apply commands to include -n bank-of-anthos when running them. This is optional but probably a best practice.

2. I modified ./kubernetes-manifests/frontend.yaml to include a loadBalancerIP field which is required when exposing services on prem. So my service definition looks like follows:

apiVersion: v1
kind: Service
metadata:
  name: frontend
spec:
  type: LoadBalancer
  loadBalancerIP: <enter available VIP here>
  selector:
    app: frontend
  ports:
  - name: http
    port: 80
    targetPort: 8080

If you enter an invalid date, you get a backend failure. It would be better if we could display a warning in the frontend before submission

This part of readme is slightly sad as it requires extra user interaction
to deploy the app, and requires another step of additional user interaction.

openssl genrsa -out jwtRS256.key 4096
openssl rsa -in jwtRS256.key -outform PEM -pubout -out jwtRS256.key.pub
kubectl create secret generic jwt-key --from-file=./jwtRS256.key --from-file=./jwtRS256.key.pub

AFAICT the resulting key is never used on developer machine and only lives in
the cluster (?). If that's the case, consider creating a one-off Job manifest
in kubernetes-manifests.yaml.

Istio's own deployment applies this practice (one-off Jobs).

Similarly, an initContainer for userservice may be also able to handle this,
though I'm not so sure about feasibility of that one as it depends on the
volume directly.

Support for running the bootstrap.sh script from GCP GCE (Ubuntu VM).

Right now, the script is doing this check :
if [[ $OSTYPE == "linux-gnu" && $CLOUD_SHELL == true ]]; then

I'm not sure what pre-requisites need to be in place, but certainly kubectl and helm and kops and gcloud and kubectx need to be available.

https://hackernoon.com/restful-api-design-step-by-step-guide-2f2c9f9fcdbf

I didn't setup rsa private keys but ran skaffold run successfully.

All pods are healthy but the frontend is not responding on its IP.

Logs show an exception, so I'm guessing a health probe that should be able to
catch this class of issues if homepage is not loading.

json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
[2020-04-15 14:42:27 +0000] [9] [ERROR] Exception on /payment [POST]
Traceback (most recent call last):
  File "/env/lib/python3.7/site-packages/flask/app.py", line 2447, in wsgi_app
    response = self.full_dispatch_request()
  File "/env/lib/python3.7/site-packages/flask/app.py", line 1952, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/env/lib/python3.7/site-packages/flask/app.py", line 1821, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/env/lib/python3.7/site-packages/flask/_compat.py", line 39, in reraise
    raise value
  File "/env/lib/python3.7/site-packages/flask/app.py", line 1950, in full_dispatch_request
    rv = self.dispatch_request()
  File "/env/lib/python3.7/site-packages/flask/app.py", line 1936, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/vmagent/app/flask_server.py", line 139, in payment
    _submit_transaction(transaction_data)
  File "/home/vmagent/app/flask_server.py", line 212, in _submit_transaction
    raise UserWarning(resp.json().get('msg', ''))
  File "/env/lib/python3.7/site-packages/requests/models.py", line 898, in json
    return complexjson.loads(self.text, **kwargs)
  File "/opt/python3.7/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/opt/python3.7/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/opt/python3.7/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None

Description

Add tests to userservice

Expected Behavior

• Tests run in CI
• Test cases
  • /version returns expected version with 200 OK
  • /ready returns 200 OK
  • can create user and return 201 and expected user
  • creating existing user returns 409
  • sql error results in 500 and error message
  • invalid user with missing fields returns 400
  • invalid user with mismatched password values results in 400
  • can log in with existing user
  • logging in with existing user and invalid password results in 404

The frontend logs are full of arbitrary INFO logs. The code also throws ERRORs on failed login attempts.
A failed login is not something that needs attention, it is expected behavior.

Update logging levels to be more consistent. Make them spares enough to be useful to debug issues.

docs/development.md doesn't talk about Java, so upon running skaffold build,
I've got a JDK install popup on macOS, and logs saying:

error checking cache, caching may not work as expected: getting hash for
artifact gcr.io/bank-of-anthos/balancereader: getting dependencies for
"gcr.io/bank-of-anthos/balancereader": getting jib-maven dependencies:
initial Jib dependency refresh failed: failed to get Jib dependencies:
running [/Users/ahmetb/workspace/bank-of-anthos/mvnw
jib:_skaffold-fail-if-jib-out-of-date -Djib.requiredVersion=1.4.0 --projects
src/balancereader --also-make jib:_skaffold-files-v2 --quiet --batch-mode]

It seems frontend svc has type:LoadBalancer. If the demo always will use Istio,
why not just integrate to the istio ingressgateway?

Problem:
Duplicate transactions can be created in two ways

1. user spams "submit" button
2. attacker (or buggy router) copies and replays TCP packets, resulting in duplicate valid transactions reaching backend

Plan:
the frontend generates a uuid on each modal load, and passes it and a timestamp with the transaction. The ledgerwriter keeps a cache of recent uuids, and ignores duplicates and old transactions. The client generated uuid/timestamp doesn't need to be persisted to the database or trusted, this is just to weed out accidental duplicates

this isn't quite a best practice

      volumes:
      - name: keys
        secret:
          secretName: jwt-key
          items:
          - key: jwtRS256.key
            path: privatekey
          - key: jwtRS256.key.pub
            path: publickey

since the relative path e.g. /root/ can change based on user set in dockerfile
or at execution time (as a matter of fact, disabling root execution is a very
common GKE/kubernetes best practice).

Please use paths like /etc/secrets/foo etc.

The maximum value for a 32-bit integer. Consider using a 64-bit integer.

Update the list of transactions to show the names of your contacts if that account is in your contacts:

External Checking $1480
Mom $439
Friend -$121
412894324 -$32

Repro:
Deposit $X
Send $X

Expected:
New transaction recorded and account balance == $0

Actual:
Transaction failed

For Java: https://www.loggly.com/ultimate-guide/java-logging-basics/

For Python: https://www.loggly.com/ultimate-guide/python-logging-basics/

Sometimes the smoke test will fail even though the services are all running.

According to @ahmetb:

So the smoke tests fail on grep Aggregated which is actually indicating a test issue (i.e. string not found). Tests need to be better in this case.

Currently using apiVersion: skaffold/v1beta15, putting us a major version behind the latest apiVersion: skaffold/v2alpha4

• update github actions skaffold version
• update skaffold version in config

Description

Cypress UI Tests can generate screenshots for failing tests that are helpful for debugging. These should be pushed to storage and viewable by authors of PR's.

Expected Behavior

• Screenshots are enabled
• Screenshots are pushed to Storage Bucket
• Links to screenshots are surfaced in logs
• Screenshots are deleted on VM or CI has sufficient permission to delete screenshots on next build

Current Behavior

Screenshots are currently disabled because cypress docker container runs as root, resulting in artifacts that only root user can delete. When CI re-runs, it is unable to delete the screenshots directory from the previous build.

##[error]One or more errors occurred. (One or more errors occurred. (Access to the path '/home/user/actions-runner/_work/anthos-finance-demo/anthos-finance-demo/e2e/cypress/screenshots/login_spec.js/Bad Credentials on Form Submission -- fails with alert banner (failed).png' is denied.)) (Access to the path '/home/user/actions-runner/_work/anthos-finance-demo/anthos-finance-demo/e2e/cypress/screenshots/login_spec.js/Bad Credentials on Form Submission -- fails with alert banner (failed).png' is denied.)
##[error]Exit code 1 returned from process: file name '/home/user/actions-runner/bin.2.168.0/Runner.PluginHost', arguments 'action "GitHub.Runner.Plugins.Repository.v1_0.CheckoutTask, Runner.Plugins"'.

Possible solutions

• Run cypress as non-root
• Modify permissions on screenshots/ dir
• Move screenshots out of _work dir (temp fix, would need to be cleaned up manually)

Prevent users from being able to create contacts or external accounts with identical account numbers & routing numbers.

Instead, show an error message: e.g. "contact already exists"

Description

Add unit tests to ledger-writer service

Expected Behavior

• Tests run in CI
• LedgerWriterController
  • versions()
  • readiness()
  • refactors for testability
  • addTransaction
  • validateTransaction
  • submitTransaction

It looks like we can add our app name in the startup logs

Under Continuous Integration section the link to the GitHub actions https://github.com/GoogleCloudPlatform/bank-of-anthos/blob/master/docs/.github/workflows is 404.

Did someone just forget to git add ?