-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
@@ -1,6 +1,9 @@ | |||
Listen {{ .Values.network.ip_address }}:{{ .Values.network.port.public }} | |||
Listen {{ .Values.network.ip_address }}:{{ .Values.network.port.admin }} | |||
|
|||
LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\"" combined | |||
LogFormat "%{X-Forwarded-For}i %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\"" proxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By setting up logging like this we can log requests appropriately from both internal and external clients when behind an ingress controller.
ansible localhost -vvv \ | ||
-m mysql_user -a "login_host='{{ include "keystone_db_host" . }}' \ | ||
login_port='{{ .Values.database.port }}' \ | ||
login_user='{{ .Values.database.root_user }}' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future PR I'd really like to review this logic, as writing the both the cloud-admin, and MariaDB root password to a configmap does not seem the best idea.
The simplest approach would simply be to use a secret for this entire file instead, though loading the admin credentials via environment variables (sourced from separate K8s Secrets) initially seems the most elegant and appropriate methodology.
This PS loads all the required keystone configuration files into a container for an apache based deployment. It allows OpenStack-Helm to be image agnosic, meaning operators can use any Apache based Keystone image they want.
@@ -61,26 +61,62 @@ spec: | |||
ports: | |||
- containerPort: {{ .Values.network.port.public }} | |||
- containerPort: {{ .Values.network.port.admin }} | |||
lifecycle: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, I think we should include a terminationGracePeriodSeconds setting along with this and include a values.yaml setting setting this to 30 seconds (what will happen even if we dont set it) for clarity and the potential to override. Not necessary for this PR though but I would like to see it when we make a pass for graceful termination pattern across all charts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me Pete. One small comment for consideration.
@intlabs This became invalidated during a few merges, can you rebase? |
…into yaodu/keystone
This PR updates the Keystone Chart:
This change is