Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(unleash): set DATABASE_SSL to false as default #161

Merged
merged 3 commits into from
Jul 31, 2024
Merged

fix(unleash): set DATABASE_SSL to false as default #161

merged 3 commits into from
Jul 31, 2024

Conversation

joel-teratis
Copy link
Contributor

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.

@ivarconr ivarconr requested a review from gastonfournier July 31, 2024 07:50
@gastonfournier
Copy link
Contributor

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

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

@gastonfournier
Copy link
Contributor

@joel-teratis 🎯 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 ☝️ helps

Copy link
Contributor

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

LG! Thanks!!

@gastonfournier gastonfournier merged commit 18d534c into Unleash:main Jul 31, 2024
11 checks passed
@joel-teratis joel-teratis deleted the hotfix/database-ssl branch July 31, 2024 14:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: The server does not support SSL connections
2 participants