#171 - Allow setting up read only replicas #174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- this change is needed to ensure that generic configmap can be used by both writable and readonly replicas. then a schemas configmap for writable replicas and one for readonly can be created without much duplication.
- allows for example to only enable SSL port on service but allow both ports on internal headless service
|
First of all thank you for your great contribution ! can you add a test in |
No worries. Great suggestion, will do. |
|
I tried it and I found out that if you are using If I want to still have the replication working, I think it's best to use a strict access control list such as : Using this I ensure that only admin user is allowed to write , others can only read |
Replication is working fine in my case but I will check if I forgot to commit something. From docs that I read olcReadOnly should not break replication |
jp-gouin
reviewed
Jun 24, 2024
There was a problem hiding this comment.
In the init-schema init container we should remove the first half since the read only will necessarily replicate against the other one
templates/statefulset.yaml
Outdated
| @@ -193,6 +193,8 @@ spec: | |||
| {{- end }} | |||
| - configMapRef: | |||
| name: {{ template "openldap.fullname" . }}-env | |||
| - configMapRef: | |||
There was a problem hiding this comment.
I think it's best to add a env var and drop the configmap-env-schemas.yaml file
- name: LDAP_EXTRA_SCHEMAS
value: {{ print "cosine,inetorgperson,nis," (include "openldap.schemaFiles" .)}}
| {{- if .Values.containerSecurityContext.enabled }} | ||
| securityContext: {{- omit .Values.containerSecurityContext "enabled" | toYaml | nindent 12 }} | ||
| {{- end }} | ||
| env: |
There was a problem hiding this comment.
I think it's best to add a env var and drop the configmap-env-readonly-schemas.yaml file
- name: LDAP_EXTRA_SCHEMAS
value: {{ print "cosine,inetorgperson,nis," (include "openldap.schemaFiles" .) ",readonly"}}
templates/statefulset-readonly.yaml
Outdated
| - sh | ||
| - -c | ||
| - | | ||
| host=$(hostname) |
There was a problem hiding this comment.
can be simplified to
command:
- sh
- -c
- |
cp -p -f /cm-schemas-acls/*.ldif /custom_config/
echo "let the replication takes care of everything :)"
{{- if .Values.global.existingSecret }}
sed -i -e "s/%%CONFIG_PASSWORD%%/${LDAP_CONFIG_ADMIN_PASSWORD}/g" /custom_config/*
sed -i -e "s/%%ADMIN_PASSWORD%%/${LDAP_ADMIN_PASSWORD}/g" /custom_config/*
{{- end }}
|
If I leave the default readonly.ldif: |
dn: olcDatabase={2}mdb,cn=config
olcReadonly: TRUEI can see the following logs : Openldap complain about the If I change the It's correctly added but the replication seems broken : What is your output when you run |
|
Sure, here: This query is going to the I also have no replication errors, have created multiple users (such as testuser in above query). |
i had this issue as well, in |
|
i will try to write a github test to see if it leads to your issue as well, but my values are as follows: |
|
Also, might or might not be relevant, I noticed that there is an issue on ACLs in cluster mode, so i do run: manually, once the main statefulset cluster is healthy. |
- removed env cm for schemas and instead just use env in each SS - removed part of readonly init container that only applies to master cluster
|
If you can give me permissions to run workflows ad-hoc that would be nice! @jp-gouin (not fully sure how that works) |
|
@davidfrickert manual approval is required for first time contributor Anyway I recommend you use act to test your workflow locally before commit . This will save you quite some time |
jp-gouin
reviewed
Jun 25, 2024
| sslLdapPortNodePort: 30636 | ||
| type: NodePort | ||
| serviceReadOnly: | ||
| ldapPortNodePort: 31389 |
There was a problem hiding this comment.
You need to update .bin/kind-conf.yml to include the 2 new ports :
kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
- role: worker
kubeadmConfigPatches:
- |
kind: JoinConfiguration
nodeRegistration:
kubeletExtraArgs:
node-labels: "ingress-ready=true"
extraPortMappings:
- containerPort: 80
hostPort: 8080
protocol: TCP
- containerPort: 443
hostPort: 8444
protocol: TCP
- containerPort: 30636
hostPort: 30636
- containerPort: 30389
hostPort: 30389
- containerPort: 31389
hostPort: 31389
- containerPort: 31636
hostPort: 31636
- role: worker
- role: worker
There was a problem hiding this comment.
ah, thanks for the tip
just checked this and you're right, the pod stays healthy after crashing once but the read only setting is not applied, am checking on alternatives EDIT: I don't think the olcAccess way works as this gets replicated onto the rest of the cluster |
|
okay, RO replica with ACL actually seems to work. |
|
another update, i finally found out how to make it truly read only. |
|
any ideas on how to stop olcMirrorMode/olcMultiProvider to be replicated from master replicas to read only replica @jp-gouin ? but i'd prefer if this attribute is not replicated so i dont have to run manual stuff. |
|
How about changing the |
will try, thanks |
I think this might help a bit in another issue that i was facing that was deleting olcMirrorMode from "read only replica" would also delete it from main cluster. |
|
latest commits should make tests work for readonly (tested locally) edit: well, putting normal nodes in refreshAndPersist and read only in refreshOnly doesn't work as that is also sync'd so eventually replica also goes into refreshAndPersist mode |
|
So I got a pretty decent results with the following changes : The The Finally,
|
|
Hi @davidfrickert |
hey, sorry for the delay, been sick and working on other stuff. I think that your proposal doesn't achieve a fully read-only status as someone with the global admin credentials can still write through that connection which is not ideal. The setup using the removal of the olcMultiProvider from the read only replica makes it truly read only such that writes are always rejected, so it seems better. What do you think? |
|
Hey no worries So how does the replication work if the base is in total read-only ? |
It is in read-only from LDAP operations but syncrepl can still replicate fine (tested) |
|
Alright to be honest I'm a bit lost in all the comments 😅 Can you summarize what is working and how to make it work ? Does the last commit reflect it ? Are schemas replicated from master nodes ? |
|
I skipped a few comments, but I think what you guys are looking for is the concept of a 'consumer'. In openldap a consumer does not have a syncprov overlay and it should not accept writes. I am also not sure read replicas fit the semantics of a stateful set. I'd be curious to find other industry examples of read replica manifests. |
|
Yes @pritchardtw this is what is suggested above , to only have read only nodes performing a base replication with To echo what you said , it shouldn't be a |
|
I'm not that well versed in OpenLDAP so the current implementation might not be ideal. If the consumer option is easier to manage and it provides the same protection (no write for any user, not even the admin user), then that sounds great. |
|
The last commit does work but it seems a bit shaky, as it requires you to manually remove the olcMultiProvider config from both databases of the read-only replica to make it read-only. This could probably be achieved with a Job (?) But as said above if you guys think there is a way to get this read-only functionality without needing to replicate data then i'm all for it! |
|
hi folks, I missed this in the documentation for
By setting our So by combining our two approach @davidfrickert we can actually have a bunch of Setting a node as consumer can be achieve using only You don't need to do any post-install steps. Does that sounds good to you ? Note: I'll look into converting the Full solution below
|
this looks good, can we get it onto a branch to test? |
|
There is a significantly simpler way of achieving the functionality of a read-only proxy that I was not aware, that is using the "meta" backend (and olcReadOnly that was mentioned here already) I will try to submit a PR for review once i have it cleaned up. |
|
@davidfrickert That generally defeats the purpose of a read replica if you want to scale reads if they are all landing on the primaries. If you want a read only client I'd configure a client with proper read only permissions. If you want a read replica I'd set up an openldap server as a |
|
Alright ! after a merge nightmare, you can now try the read-only feature. Let me know what you think |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Pre-submission checklist:
Need to do reduce duplication as of right now there is huge duplication in statefulset, services, etc.