crowdsecurity / helm-charts

CrowdSec community kubernetes helm charts
MIT License
27 stars 33 forks source link

Support for LAPI High availability #112

Closed erwanval closed 7 months ago

erwanval commented 1 year ago
michalkutrzeba-odrabiamy commented 1 year ago

This is great and needed part of configuration! I was looking for it.

I've verified it and it works.

Although I would encourage you to be more consistent, instead inventing new file names, you could use the same that are used by crowdsec and the same that are already documented.

So imho would be better to stick with config.yaml.local instead config-override.yaml.

It would be easier to understand what it really is.

michalkutrzeba-odrabiamy commented 1 year ago

Also, there is a bug in loading value in config so it wont work in version v.1.5.2, we need to update to v1.5.2. docker_start.sh loads values directly from config.yaml skipping config.yaml.local in v1.5.3 it is fixed.

erwanval commented 1 year ago

Although I would encourage you to be more consistent, instead inventing new file names, you could use the same that are used by crowdsec and the same that are already documented.

So imho would be better to stick with config.yaml.local instead config-override.yaml.

It would be easier to understand what it really is.

Thanks for your input. You're right, I just found it cleared to have it named like that at first, but it's better to stay consistent with the documentation. I changed it.

Also, there is a bug in loading value in config so it wont work in version v.1.5.2, we need to update to v1.5.2. docker_start.sh loads values directly from config.yaml skipping config.yaml.local in v1.5.3 it is fixed.

I assume you tested with persistent volume in ReadWriteMany mode then. The cloud provider I'm using doesn't support that, so I wasn't able to test it. However, from my understanding, persisting configuration isn't really required unless you're doing manual changes inside the pod, and not relying on configmap or secrets. Anyway, it's good to know this is solved in 1.5.3, but I'm not including it in this PR, as there is another PR to update to 1.5.4 already open.

he2ss commented 1 year ago

Hi @erwanval,

For information, here is the draft PR (https://github.com/crowdsecurity/crowdsec/pull/2506) that improves the HA handling in crowdsec. I propose to merge this when the next stable will be available.

he2ss commented 9 months ago

Hi @erwanval,

the PR on crowdsec is merged. Let's now wait for the release to bump here the docker image and merge this PR.

And sorry again for the delay.

he2ss commented 8 months ago

Hi @erwanval,

We released new rc and here is the docker image.

Do you think you can try using this image and see if it's working as you expected with this PR ?

erwanval commented 7 months ago

Hi @he2ss,

Sorry for the delay, I tested using the v1.6.1-rc1 image, and everything works as expected on my side. I have seen v1.6.1-rc2 has been released since, but there is no docker image for it, so I haven't tested that one.

Also, I resolved the conflict

he2ss commented 7 months ago

Hi @erwanval,

Nice, I just tested also the 1.6.1-rc1 and seems working well using your PR. So we can merge this PR and we will do a helm chart release when the 1.6.1 will be available in stable.

Is it ok for you @erwanval ?

erwanval commented 7 months ago

Yes, thank you !