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

Allow Postgres DB connection to enable SSL #2500

Merged
merged 3 commits into from
Jul 26, 2023
Merged

Allow Postgres DB connection to enable SSL #2500

merged 3 commits into from
Jul 26, 2023

Conversation

hardillb
Copy link
Contributor

part of FlowFuse/helm#151

This should make sequelize try and connect with SSL first then fall back to plain.

Description

This should make sequelize try and connect with SSL first then fall back to plain.

Shouldn't need a doc upgrade as this should "just work"

Likewise this shouldn't need a test change

Related Issue(s)

FlowFuse/helm#151

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

part of FlowFuse/helm#151

This should make sequelize try and connect with SSL first then fall back to plain.
@hardillb hardillb added this to the 1.10 milestone Jul 18, 2023
@hardillb hardillb self-assigned this Jul 18, 2023
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #2500 (5a82958) into main (7d58f58) will increase coverage by 72.80%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##            main    #2500       +/-   ##
==========================================
+ Coverage   1.55%   74.35%   +72.80%     
==========================================
  Files        489      224      -265     
  Lines      17133     8852     -8281     
  Branches    3976     1823     -2153     
==========================================
+ Hits         266     6582     +6316     
+ Misses     16867     2270    -14597     
Flag Coverage Δ
backend 74.35% <0.00%> (?)
frontend ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
forge/db/index.js 65.51% <0.00%> (+65.51%) ⬆️

... and 441 files with indirect coverage changes

@hardillb hardillb requested a review from knolleary July 18, 2023 14:31
@hardillb
Copy link
Contributor Author

Tested the option with:

const {Sequelize} = require('sequelize')

const dbOptions = {
	dialect: 'postgres',
	host: 'xxxx.8nj.cockroachlabs.cloud',
	port: 26257,
	username: 'xxx',
	password: 'xxxxxxxxxx',
	database: 'flowforge',
	dialectOptions: {
		ssl: {
			sslmode: 'prefer'
		}
	}
}

const sequelize = new Sequelize(dbOptions)

sequelize.authenticate()
.then(() => {
  console.log("connected")
})
.catch((err) => {
  console.log(err)
})

I have since shutdown the test cockroach db instance, but can spin up another free one if needed.

@hardillb
Copy link
Contributor Author

OK, this isn't doing what the docs suggest it should, sslmode: 'prefer' is supposed to gracefully fall back.

@hardillb hardillb marked this pull request as draft July 18, 2023 14:52
Adds `forge.db.ssl` as a boolean option
@hardillb hardillb changed the title Make sequelize prefer SSL for postgres Allow Postgres DB connection to enable SSL Jul 20, 2023
@hardillb hardillb marked this pull request as ready for review July 20, 2023 09:52
@hardillb
Copy link
Contributor Author

I've made this a simple toggle for now as the fallback options doesn't appear to have been implemented in the nodejs PG driver.

brianc/node-postgres#2720

Copy link
Contributor

@Pezmc Pezmc left a comment

Choose a reason for hiding this comment

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

Seems sensible

@hardillb hardillb merged commit 6039ec2 into main Jul 26, 2023
4 checks passed
@hardillb hardillb deleted the hardillb-patch-2 branch July 26, 2023 10:25
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.

None yet

2 participants