Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Postgres sandbox #13651

Merged
merged 6 commits into from
Oct 22, 2020
Merged

Add a Postgres sandbox #13651

merged 6 commits into from
Oct 22, 2020

Conversation

fabriziomello
Copy link
Contributor

@fabriziomello fabriziomello commented Oct 20, 2020

Add an example of how to use Envoy's Postgres network filter. ref #13597

Signed-off-by: Fabrízio de Royes Mello <fabrizio@ongres.com>
Signed-off-by: Fabrízio de Royes Mello <fabrizio@ongres.com>
@phlax
Copy link
Member

phlax commented Oct 20, 2020

@fabriziomello ill check through more once docs are rendered, but at first pass it looks great

Signed-off-by: Fabrízio de Royes Mello <fabrizio@ongres.com>
@phlax
Copy link
Member

phlax commented Oct 20, 2020

@fabriziomello you have a couple of format issues (which will block other CI)

there are some shellcheck issues

https://dev.azure.com/cncf/envoy/_build/results?buildId=54807&view=logs&j=55bc2ab9-36c1-5ab8-58a6-8c258728eeff&t=33b57bf9-ef13-58c8-bc2b-f804aeb16d39&l=139

if you install shellcheck locally you can run it with shellcheck -x examples/postgres/verify.sh

the other issue is an annoying one ill comment inline...

Name Command State Ports
------------------------------------------------------------------------------------------------------------------------
postgres_postgres_1 docker-entrypoint.sh postgres Up 0.0.0.0:5432->5432/tcp
postgres_proxy_1 /docker-entrypoint.sh /usr ... Up 10000/tcp, 0.0.0.0:1999->1999/tcp, 0.0.0.0:8001->8001/tcp
Copy link
Member

Choose a reason for hiding this comment

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

the formatter needs a single space after ... see here 25d87ca

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

Choose a reason for hiding this comment

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

Where exactly?

here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I fixed it already! Thanks!

Signed-off-by: Fabrízio de Royes Mello <fabrizio@ongres.com>
@fabriziomello
Copy link
Contributor Author

@fabriziomello you have a couple of format issues (which will block other CI)

there are some shellcheck issues

https://dev.azure.com/cncf/envoy/_build/results?buildId=54807&view=logs&j=55bc2ab9-36c1-5ab8-58a6-8c258728eeff&t=33b57bf9-ef13-58c8-bc2b-f804aeb16d39&l=139

if you install shellcheck locally you can run it with shellcheck -x examples/postgres/verify.sh

Thanks. Fixed!

the other issue is an annoying one ill comment inline...

Where exactly?

@fabriziomello
Copy link
Contributor Author

Seems the CI didn't like my verify.sh. Will check it tmr.

proxy:
image: envoyproxy/envoy-dev:latest
volumes:
- ./envoy.yaml:/etc/envoy.yaml
Copy link
Member

@phlax phlax Oct 20, 2020

Choose a reason for hiding this comment

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

you will need to mount this file into a docker image with a Dockerfile - like in the mysql example, and ensure its readable (the envoy proc runs as non-root in the container, CI checks to ensure this works where umask = 027)

this is why verify.sh is failing only on CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh now I understand the reason why the mysql example uses a separated Dockerfile. Fixed!

networks:
envoymesh:
aliases:
- envoy
Copy link
Member

Choose a reason for hiding this comment

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

i dont think the alias is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

networks:
envoymesh:
aliases:
- postgres
Copy link
Member

Choose a reason for hiding this comment

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

this alias definitely isnt necessary as its the same as the service name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

# See PostgreSQL documentation about "trust":
# https://www.postgresql.org/docs/current/auth-trust.html
POSTGRES_HOST_AUTH_METHOD: trust
expose:
Copy link
Member

@phlax phlax Oct 20, 2020

Choose a reason for hiding this comment

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

i dont think either expose or ports are necessary as the containers communicate internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agree. Fixed!

envoymesh:
aliases:
- envoy
expose:
Copy link
Member

Choose a reason for hiding this comment

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

this expose isnt necessary as ports below will expose the ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -34,6 +34,8 @@ def IsMissing(value):


def GetExtensionMetadata(target):
if not BUILDOZER_PATH:
raise ExtensionDbError('Buildozer not found!')
Copy link
Member

Choose a reason for hiding this comment

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

im curious was this a fix for an issue you hit building the docs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.. I hit some errors because the buildozer not found but the message was not clear about it and to discover it I needed to debug the python script. So I added this small exception do make our live easier.

Step 4: Issue commands using psql
*********************************

Use ``psql`` to issue some commands and verify they are routed via Envoy. Note
Copy link
Member

Choose a reason for hiding this comment

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

s/Use psql/This example uses a psql client inside a container/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}

run_log "Create a postgres database"
_psql -c 'CREATE DATABASE test;'
Copy link
Member

Choose a reason for hiding this comment

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

could we rename the db to maybe testdb - i was a bit confused at first by the name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make total sense... I also renamed the table name from test to tbl.

@phlax
Copy link
Member

phlax commented Oct 20, 2020

@fabriziomello i ran it/went through it locally and it runs nicely

docs are rendered here and look good - i left some nits above

https://storage.googleapis.com/envoy-pr/13651/docs/start/sandboxes/postgres.html


Check TCP stats were updated.

Terminal 1
Copy link
Member

Choose a reason for hiding this comment

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

i think just remove this "Terminal 1"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@fabriziomello
Copy link
Contributor Author

Seems now the CI is happy!! Finally!!

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

Thanks @fabriziomello lgtm

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Awesome!

@mattklein123 mattklein123 merged commit 7000fad into envoyproxy:master Oct 22, 2020
@phlax phlax mentioned this pull request Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants