Fixes #10234 - Postgres SSL Certificate fix #10300

reddymh · 2023-01-02T20:42:16Z

Signed-off-by: Rajshekar Reddy reddymh@gmail.com

Fixes #10234

Please do not open a pull request until you have checked ALL of these:

Create the PR as draft .
Run make pre-commit -B to fix codegen and lint problems.
Sign-off your commits (otherwise the DCO check will fail).
Use a conventional commit message (otherwise the commit message check will fail).
"Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
Github checks are green.
[] Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

rohankmr414 · 2023-01-04T22:33:31Z

@reddymh have you tested this on your end?

rohankmr414 · 2023-01-04T22:36:01Z

Dockerfile

+WORKDIR /home/argo
+


do we really need this? can't we use absolute paths for files in persist/sqldb/sqldb.go

WorkDir is needed to refer to where all the binary and files need to be copied. Can we config the path?

rohankmr414 · 2023-01-04T22:37:09Z

config/config.go

+	SSL              bool                    `json:"ssl,omitempty"`
+	SSLMode          string                  `json:"sslMode,omitempty"`
+        CaCertSecret     apiv1.SecretKeySelector `json:"caCertSecret,omitempty"`
+        ServerCertSecret apiv1.SecretKeySelector `json:"serverCertSecret,omitempty"`
+        ServerKeySecret  apiv1.SecretKeySelector `json:"serverKeySecret,omitempty"`


can you fix the lint here please https://github.com/argoproj/argo-workflows/actions/runs/3832454101/jobs/6522907398

@rohankmr414 tried in visual studio and not getting any lint error but again I will cross check on lint errors.

Can you run make lint in the repo directory? That'll fix it

rohankmr414 · 2023-01-04T22:38:39Z

persist/sqldb/sqldb.go

@@ -53,9 +54,44 @@ func CreatePostGresDBSession(kubectlConfig kubernetes.Interface, namespace strin
 	}

 	if cfg.SSL {
-		if cfg.SSLMode != "" {
+		if cfg.SSLMode != "" && cfg.SSLMode != "disable" {
+			pgCertPath := "./pgcerts"


can we use the absolute path here and get rid of WORKDIR /home/argo from Dockerfile?

@rohankmr414 we ca use absolute path. I thought argo server already using workdir in Dockerfile so I thought same thing we can use for argo controller.

I don't see any issues with it as well, it's just an extra change that can be avoided.

cc: @sarabala1979

Can we add another element in structs to configure the path and have a default value too?

@sarabala1979 @rohankmr414 then we need to use constructor for this field in the struct.
Do you need this change?

@reddymh yes you can add a field to the spec

@rohankmr414 / @sarabala1979 added field to the spec and default vault if the variable is not configured in configmap or not defined.
Please review and approve the same

rohankmr414 · 2023-01-04T22:42:30Z

can you also sign off your commits for DCO https://github.com/argoproj/argoproj/blob/master/community/CONTRIBUTING.md#legal

reddymh · 2023-01-05T02:45:13Z

yes . we have tested and running in prod as well

reddymh · 2023-01-26T17:35:27Z

can you also sign off your commits for DCO https://github.com/argoproj/argoproj/blob/master/community/CONTRIBUTING.md#legal

I have added signoff but still showing DCO - action required

vladlosev · 2023-01-29T20:40:38Z

config/config.go

+	SSL              bool                    `json:"ssl,omitempty"`
+	SSLMode          string                  `json:"sslMode,omitempty"`
+	CaCertSecret     apiv1.SecretKeySelector `json:"caCertSecret,omitempty"`
+	ServerCertSecret apiv1.SecretKeySelector `json:"serverCertSecret,omitempty"`


This field and the next are mapped to the sslcert and sslkey options in the PostgreSQL connection options. The PostgreSQL documentation defines them as client certificate and key filename, so naming them server cert and server key is incorrect.

@vladlosev changed the ServerCertSecret and ServerKeySecret as ClientCertSecret and ClientCertSecret and will change respective helm chart as well.

cc: @sarabala1979 @rohankmr414

vladlosev

FWIW

rohankmr414 · 2023-01-31T18:10:26Z

I have added signoff but still showing DCO - action required
@reddymh you'll have to sign off all your commits, you can also squash all your commits into one and then sign off the commit, you can also go to the details of the failing check and click the button to accept the DCO

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

reddymh · 2023-02-01T04:21:35Z

I have added signoff but still showing DCO - action required
@reddymh you'll have to sign off all your commits, you can also squash all your commits into one and then sign off the commit, you can also go to the details of the failing check and click the button to accept the DCO

@rohankmr414 @sarabala1979 @vladlosev Can we merge the PR?

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com> Signed-off-by: goshado <goshatoo@gmail.com>

tooptoop4 · 2023-02-25T21:53:25Z

@reddymh do u think this caused #10498?

…#10300)" This reverts commit 5d0db00.

…#10300)" (#16) This PR: - reverts the broken postgres connection PR

This reverts commit 5d0db00.

rohankmr414 suggested changes Jan 4, 2023

View reviewed changes

rohankmr414 approved these changes Jan 20, 2023

View reviewed changes

reddymh mentioned this pull request Jan 28, 2023

Argo Workflow: Postgres SSL Certificate Configurations if SSL is enabled argoproj/argo-helm#1806

Closed

vladlosev reviewed Jan 29, 2023

View reviewed changes

vladlosev mentioned this pull request Jan 29, 2023

feat(argo-workflows): Add support for Persistence Postgres SSL certificate if ssl is enabled argoproj/argo-helm#1807

Closed

6 tasks

sarabala1979 approved these changes Jan 30, 2023

View reviewed changes

sarabala1979 enabled auto-merge (squash) January 30, 2023 19:08

auto-merge was automatically disabled January 31, 2023 13:52
Head branch was pushed to by a user without write access

reddymh requested a review from vladlosev January 31, 2023 13:57

vladlosev approved these changes Jan 31, 2023

View reviewed changes

reddymh force-pushed the master branch 2 times, most recently from 0eaaad7 to b0354e7 Compare January 31, 2023 19:12

reddymh requested review from jessesuen and alexec as code owners January 31, 2023 19:12

reddymh force-pushed the master branch 2 times, most recently from f998315 to 6d4bcb3 Compare January 31, 2023 20:14

reddymh closed this Jan 31, 2023

reddymh force-pushed the master branch from 45f71a0 to e2c2ad3 Compare January 31, 2023 20:25

reddymh reopened this Jan 31, 2023

reddymh added 3 commits February 1, 2023 02:33

Postgres SSL Certificate fix

490f7c2

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

Re-Run The Build

4a8496b

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

Re-Run The Build

f8f46eb

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

reddymh force-pushed the master branch from a520b8e to f8f46eb Compare January 31, 2023 21:03

reddymh removed the request for review from alexec February 6, 2023 10:23

reddymh requested review from sarabala1979 and removed request for jessesuen February 6, 2023 10:23

sarabala1979 merged commit 5d0db00 into argoproj:master Feb 7, 2023

isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 9, 2023

Fixes argoproj#10234 - Postgres SSL Certificate fix (argoproj#10300)

39a29b6

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com>

GoshaDo pushed a commit to GoshaDo/argo-workflows that referenced this pull request Feb 9, 2023

Fixes argoproj#10234 - Postgres SSL Certificate fix (argoproj#10300)

1ad71cb

Signed-off-by: Rajshekar Reddy <reddymh@gmail.com> Signed-off-by: goshado <goshatoo@gmail.com>

tooptoop4 mentioned this pull request Feb 25, 2023

Workflow Archive database connections are not closed #10498

Closed

3 tasks

JPZ13 added a commit to pipekit/argo-workflows that referenced this pull request Mar 22, 2023

Revert "Fixes argoproj#10234 - Postgres SSL Certificate fix (argoproj…

2f8cd52

…#10300)" This reverts commit 5d0db00.

tico24 mentioned this pull request Mar 22, 2023

argo-server doesn't start with secure postgres setup #10731

Closed

3 tasks

JPZ13 added a commit to pipekit/argo-workflows that referenced this pull request Mar 22, 2023

Revert "Fixes argoproj#10234 - Postgres SSL Certificate fix (argoproj…

5d67218

…#10300)" (#16) This PR: - reverts the broken postgres connection PR

terrytangyuan added a commit that referenced this pull request Mar 23, 2023

Revert "Fixes #10234 - Postgres SSL Certificate fix (#10300)"

aa0e014

This reverts commit 5d0db00.

terrytangyuan mentioned this pull request Mar 23, 2023

Revert "Fixes #10234 - Postgres SSL Certificate fix" #10736

Merged

sarabala1979 mentioned this pull request May 4, 2023

REQUEST: Promotion to reviewer for rohankmr414 argoproj/argoproj#188

Closed

5 tasks

agilgur5 added the area/server label Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #10234 - Postgres SSL Certificate fix #10300

Fixes #10234 - Postgres SSL Certificate fix #10300

reddymh commented Jan 2, 2023 •

edited

Loading

rohankmr414 commented Jan 4, 2023

rohankmr414 Jan 4, 2023

sarabala1979 Jan 5, 2023

rohankmr414 Jan 4, 2023

reddymh Jan 5, 2023

rohankmr414 Jan 5, 2023

rohankmr414 Jan 4, 2023

reddymh Jan 5, 2023

rohankmr414 Jan 5, 2023

sarabala1979 Jan 5, 2023

reddymh Jan 7, 2023 •

edited

Loading

rohankmr414 Jan 9, 2023

reddymh Jan 26, 2023 •

edited

Loading

rohankmr414 commented Jan 4, 2023

reddymh commented Jan 5, 2023

reddymh commented Jan 26, 2023

vladlosev Jan 29, 2023

reddymh Jan 31, 2023

reddymh Jan 31, 2023

vladlosev left a comment

rohankmr414 commented Jan 31, 2023

reddymh commented Feb 1, 2023 •

edited

Loading

tooptoop4 commented Feb 25, 2023

		WORKDIR /home/argo

Fixes #10234 - Postgres SSL Certificate fix #10300

Fixes #10234 - Postgres SSL Certificate fix #10300

Conversation

reddymh commented Jan 2, 2023 • edited Loading

rohankmr414 commented Jan 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddymh Jan 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reddymh Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

rohankmr414 commented Jan 4, 2023

reddymh commented Jan 5, 2023

reddymh commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladlosev left a comment

Choose a reason for hiding this comment

rohankmr414 commented Jan 31, 2023

reddymh commented Feb 1, 2023 • edited Loading

tooptoop4 commented Feb 25, 2023

reddymh commented Jan 2, 2023 •

edited

Loading

reddymh Jan 7, 2023 •

edited

Loading

reddymh Jan 26, 2023 •

edited

Loading

reddymh commented Feb 1, 2023 •

edited

Loading