Skip to content
This repository has been archived by the owner on Jul 24, 2019. It is now read-only.

Keystone: Configmap Updates #114

Merged
merged 2 commits into from
Jan 19, 2017
Merged

Keystone: Configmap Updates #114

merged 2 commits into from
Jan 19, 2017

Conversation

intlabs
Copy link
Contributor

@intlabs intlabs commented Jan 13, 2017

This PR updates the Keystone Chart:

  • Loads all requires configmaps into container for (apache based) image agnostic deployment
  • Gracefully stops apache when terminating pods, allowing transactions to complete.
  • Removes Kolla specific logic for Keystone Endpoint bootstrapping

This change is Reviewable

@@ -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
Copy link
Contributor Author

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 }}' \
Copy link
Contributor Author

@intlabs intlabs Jan 13, 2017

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:
Copy link
Contributor

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.

Copy link
Contributor

@alanmeadows alanmeadows left a 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.

@alanmeadows
Copy link
Contributor

@intlabs This became invalidated during a few merges, can you rebase?

@alanmeadows alanmeadows merged commit d1e1736 into att-comdev:master Jan 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants