fix(unleash): set DATABASE_SSL to false as default

joel-teratis commented 3 months ago

This is related to the bug described in #144. When using an external postgres without SSL the Unleash Pods throw the following error:

 Error: The server does not support SSL connections                                                                                                                                                             
     at Socket.<anonymous> (/unleash/node_modules/pg/lib/connection.js:77:37)                                                                                                                                   
     at Object.onceWrapper (node:events:634:26)                                                                                                                                                                 
     at Socket.emit (node:events:519:28)                                                                                                                                                                        
     at addChunk (node:internal/streams/readable:559:12)                                                                                                                                                        
     at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)                                                                                                                                     
     at Readable.push (node:internal/streams/readable:390:5)                                                                                                                                                    
     at TCP.onStreamRead (node:internal/stream_base_commons:191:23)

Currently it seems like you are not able to disable SSL on an external database connection because setting .Values.dbConfig.ssl to false in the values.yaml will not be passed to the deployment because of an if statement in the deployment.yaml template.

About the changes

I changed the else if statement to an else so that DATABASE_SSL will default to "false".

Closes #144

Important files

charts/unleash/templates/deployment.yaml

Discussion points

I am not sure if this is the correct approach to solve this issue. If you have any other ideas, please let me know.

gastonfournier commented 3 months ago

Hi @joel-teratis I think this is the right thing to do: by default assume there's no SSL configured unless you configure it. But there are other was of configuring SSL, such as with an sslCertFile. So I'm considering that maybe this has to be set to false only if all the ways of configuring SSL are not set, so we prevent having DATABASE_SSL=false while attempting to configure SSL by other means.

Initially we only set SSL to false when we know it's our helm-managed postgress instance: https://github.com/Unleash/helm-charts/pull/145 but we have not considered the case of an external one without SSL. Obviously, the recommendation would be to use SSL over not using it, but in your own infrastructure you should be allowed to set this up as you wish.

My only ask would be to consider cases where other SSL configuration is defined, and in such situation avoid setting DATABASE_SSL to false

joel-teratis commented 3 months ago

@gastonfournier I changed the check to include all possible ssl fields. Is that what you meant?

gastonfournier commented 3 months ago

@joel-teratis :dart: exactly! It's awkward to see such a large if statement, but I believe we can look at it and improve later.

The linter is complaining about that if statement. I'm not expert in this but I got a suggestion to change it to:

{{- if and (not .Values.dbConfig.sslConfigFile) (not .Values.dbConfig.sslCaFile) (not .Values.dbConfig.sslKeyFile) (not .Values.dbConfig.sslCertFile) (not .Values.dbConfig.sslRejectUnauthorized) }}

I hope :point_up: helps

Unleash / helm-charts