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

Openldap config: fix #40 #47

Merged
merged 7 commits into from
Sep 6, 2023
Merged

Openldap config: fix #40 #47

merged 7 commits into from
Sep 6, 2023

Conversation

jeanpommier
Copy link
Member

  • read the configuration from chart's values file(s)
  • support passing the password (and possibly other env var alongside) using a secret

Example of valid secret could be

apiVersion: v1
kind: Secret
metadata:
  name: myldapsecret
type: Opaque
data:
  SLAPD_PASSWORD: "bXlzZWNyZXRsZGFwYWRtaW5wYXNzd29yZA=="

where the value is the actual password base64 encoded (echo -n 'mysecretldapadminpassword' | base64)

@edevosc2c
Copy link
Member

Thank you for the PR, one remark is that I much prefer to do like it's already possible for database credentials.

Create a Kubernetes Secret anyway if the user doesn't pass one: https://github.com/georchestra/helm-georchestra/blob/main/templates/database/database-geodata-secret.yaml

Instead of having the ability to pass a Kubernetes Secret, but still allow the ability to pass the password through an environment variable.

By the way, this time the CI is throwing a real error 😄: https://github.com/georchestra/helm-georchestra/actions/runs/6000210528/job/16271828142?pr=47#step:7:34
(I did fix the CI so now it should throw real errors if they are detected)

@jeanpommier
Copy link
Member Author

I much prefer to do like it's already possible for database credentials

Okay, I can update it accordingly. Are you fine with using the

envFrom:
         - secretRef:

syntax anyway ?

@jeanpommier
Copy link
Member Author

Is this better ?

values.yaml Outdated
@@ -194,6 +194,14 @@ ldap:
adminDn: "cn=admin,dc=georchestra,dc=org"
rolesRdn: "ou=roles"
orgsRdn: "ou=orgs"
# You can override in this secret some sensitive env. vars, like the ldap admin password
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a note that it can NOT be changed just by changing the value of the secret !!!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you're right. I'm reformulating.

@edevosc2c
Copy link
Member

Good for me but I think one day we should document the structure of each secret :).

@@ -111,6 +111,10 @@ Insert LDAP environment variables
*/}}
{{- define "georchestra.ldap-envs" -}}
{{- $ldap := .Values.ldap -}}
{{- $ldap_secret_georchestra_name := printf "%s-ldap-passwords-secret" (include "georchestra.fullname" .) -}}
{{- if $ldap.existingSecret }}
{{- $ldap_secret_georchestra_name = $ldap.existingSecret -}}
Copy link
Member

Choose a reason for hiding this comment

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

can't this be refactored with a "default" function from helm?
similar to

{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite possibly. I must admit I merely copied the logic from https://github.com/georchestra/helm-georchestra/blob/main/templates/_helpers.tpl#L71

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, it should be better now

@jeanpommier
Copy link
Member Author

Good for me but I think one day we should document the structure of each secret :).

You mean you'd rather have it more explictly loading just the one environment variable ? It's fine by me too

@edevosc2c
Copy link
Member

Good for me but I think one day we should document the structure of each secret :).

You mean you'd rather have it more explictly loading just the one environment variable ? It's fine by me too

No, I mean there are a lot of other cases where we allow an external secret but it's undocumented.

Copy link
Member

@edevosc2c edevosc2c left a comment

Choose a reason for hiding this comment

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

good for me, waiting on the CI to pass

@jeanpommier
Copy link
Member Author

CI failure doesn't seem related to this PR, right @edevosc2c ?

@edevosc2c
Copy link
Member

mmh that seems like a CI breaking again, that's odd I did my best to get it working properly. I have re-run it in case it would pass again during the night.

I'll check again tomorrow.

@edevosc2c edevosc2c merged commit 381bf33 into main Sep 6, 2023
1 check passed
@edevosc2c edevosc2c deleted the openldap-config branch September 6, 2023 08:34
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