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

kong: FIX Kill wait-for-postgres if host or port not set #28

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

yasn77
Copy link
Contributor

@yasn77 yasn77 commented Jan 30, 2020

Fixes #27

The script runs forever and simply prints 'waiting for db' if KONG_PG_HOST or
KONG_PG_PORT are not set. Which isn't very helpful.

This commit hard fails if either of those environment variables are not set,
making it easier and quicker to diagnose problems.

As a note, the reason why they may not be set is when a user provides there own
database but doesn't set pg_port or pg_host.

Additional changes:

  • Message is slightly more informative, so if connecting takes awhile,
    the use can see host and port being attempted.
  • Set the default postgres port

@@ -437,6 +437,8 @@ the template that it itself is using form the above sections.
{{- $_ := set $autoEnv "KONG_PG_PORT" .Values.postgresql.service.port -}}
{{- $pgPassword := include "secretkeyref" (dict "name" (include "kong.postgresql.fullname" .) "key" "postgresql-password") -}}
{{- $_ := set $autoEnv "KONG_PG_PASSWORD" $pgPassword -}}
{{- else if eq .Values.env.database "postgres" }}
{{- $_ := set $autoEnv "KONG_PG_PORT" "5432" }}
Copy link
Member

Choose a reason for hiding this comment

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

What about KONG_PG_HOST in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Omitting KONG_PG_HOST should be fine, since we can expect that it's always defined in K8S deployments. KONG_PG_PORT relies on the default from kong.conf, which netcat has no way of knowing. The kong.conf default port is typically correct for most Postgres installs, but the default host is localhost and won't be valid unless Postgres is running within the same Pod as Kong.

That's not impossible to set up, but you'd have to rework a bunch of the chart to do it, so I'm comfortable saying that it shouldn't be necessary to explicitly add the default to autoEnv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh, my thoughts were that if you use an external DB then you will add pg_host and expect it to default to Postgres port.

Besides, the nc check will hard fail if either is omitted.

@hbagdi
Copy link
Member

hbagdi commented Jan 30, 2020

@yasn77 Thank you for all the PRs. We will be working on them and merging them one by one but it will be a little slow as we are swamped at the moment. Just letting you know, and thanks again for being patient with us!

@yasn77
Copy link
Contributor Author

yasn77 commented Jan 31, 2020

@yasn77 Thank you for all the PRs. We will be working on them and merging them one by one but it will be a little slow as we are swamped at the moment. Just letting you know, and thanks again for being patient with us!

No problems 👍 and thanks to the Kong team for producing a cool bit of software :)

I have the changes in our fork, so we can continue, but I am pretty strong believer in putting stuff back, hence the PRs :)

@hbagdi hbagdi changed the base branch from master to next February 3, 2020 18:14
@hbagdi
Copy link
Member

hbagdi commented Feb 3, 2020

@yasn77 Can you rebase this on top of next and then it should be good to merge.

The script runs forever and simply prints 'waiting for db' if KONG_PG_HOST or
KONG_PG_PORT are not set. Which isn't very helpful.

This commit hard fails if either of those environment variables are not set,
making it easier and quicker to diagnose problems.

As a note, the reason why they may not be set is when a user provides there own
database but doesn't set pg_port or pg_host.

Additional changes:
 - Message is slightly more informative, so if connecting takes awhile,
the use can see host and port being attempted.
 - Set the default postgres port
@yasn77 yasn77 force-pushed the fix_hanging_wait-for-postgres branch from de20d97 to 11fff86 Compare February 4, 2020 10:26
@yasn77
Copy link
Contributor Author

yasn77 commented Feb 4, 2020

@yasn77 Can you rebase this on top of next and then it should be good to merge.

Done 👍

@hbagdi hbagdi merged commit b8bade9 into Kong:next Feb 4, 2020
rainest pushed a commit that referenced this pull request Feb 11, 2020
The script runs forever and simply prints 'waiting for db' if KONG_PG_HOST or
KONG_PG_PORT are not set. Which isn't very helpful.

This commit hard fails if either of those environment variables are not set,
making it easier and quicker to diagnose problems.

As a note, the reason why they may not be set is when a user provides there own
database but doesn't set pg_port or pg_host.

Additional changes:
 - Message is slightly more informative, so if connecting takes awhile,
the use can see host and port being attempted.
 - Set the default postgres port

From #28
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.

Migration init job hangs when using your own DB
3 participants