feat: add ssl support to sync service (#1479) #1501

alexandraoberaigner · 2025-01-07T16:35:48Z

This PR

Adds SSL support to the flagd sync service

Related Issues

Fixes #1479

How to test

run make test-flagd

Signed-off-by: Alexandra Oberaigner <[email protected]>

netlify · 2025-01-07T16:36:24Z

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	`ced5619`
🔍 Latest deploy log	https://app.netlify.com/sites/polite-licorice-3db33c/deploys/677d57e999f813000857e2d5

netlify · 2025-01-07T16:36:47Z

✅ Deploy Preview for polite-licorice-3db33c canceled.

Name	Link
🔨 Latest commit	`feebc44`
🔍 Latest deploy log	https://app.netlify.com/sites/polite-licorice-3db33c/deploys/677f9422c9900a0008915123

Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner · 2025-01-07T17:14:25Z

[QUESTION] can someone help me with a maintainable solution of the certificate generation. Is it okay to run a script at the beginning of the test to generate the files (see Makefile & script)? I figured that just committing these files is not really maintainable since they expire after the on creation specified number of days. What would be good practice?

toddbaert · 2025-01-07T17:55:31Z

[QUESTION] can someone help me with a maintainable solution of the certificate generation. Is it okay to run a script at the beginning of the test to generate the files (see Makefile & script)? I figured that just committing these files is not really maintainable since they expire after the on creation specified number of days. What would be good practice?

Is it okay to run a script at the beginning of the test to generate the files?

Yep, that's fine with me, BUT:

I can specify days 9999 in openssl which generates a cert until 2055. I'm OK with committing that, as long as it's in a folder clearly labelled "test" or something. We have other repos in the org that do the same thing, and I've seen it in many projects that do testing with SSL.

flagd/pkg/service/flag-sync/sync_service.go

aepfli · 2025-01-07T18:01:47Z

[QUESTION] can someone help me with a maintainable solution of the certificate generation. Is it okay to run a script at the beginning of the test to generate the files (see Makefile & script)? I figured that just committing these files is not really maintainable since they expire after the on creation specified number of days. What would be good practice?

Is it okay to run a script at the beginning of the test to generate the files?

Yep, that's fine with me, BUT:

I can specify days 9999 in openssl which generates a cert until 2055. I'm OK with committing that, as long as it's in a folder clearly labelled "test" or something. We have other repos in the org that do the same thing, and I've seen it in many projects that do testing with SSL.

should we maybe use the one from the flagd-testbed also within here?

toddbaert · 2025-01-07T18:08:15Z

[QUESTION] can someone help me with a maintainable solution of the certificate generation. Is it okay to run a script at the beginning of the test to generate the files (see Makefile & script)? I figured that just committing these files is not really maintainable since they expire after the on creation specified number of days. What would be good practice?

Is it okay to run a script at the beginning of the test to generate the files?

Yep, that's fine with me, BUT:
I can specify days 9999 in openssl which generates a cert until 2055. I'm OK with committing that, as long as it's in a folder clearly labelled "test" or something. We have other repos in the org that do the same thing, and I've seen it in many projects that do testing with SSL.

should we maybe use the one from the flagd-testbed also within here?

Yep we could do that. What's the expiry on that one?

…days expiration period (open-feature#1479) Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner · 2025-01-08T09:48:32Z

[QUESTION] can someone help me with a maintainable solution of the certificate generation. Is it okay to run a script at the beginning of the test to generate the files (see Makefile & script)? I figured that just committing these files is not really maintainable since they expire after the on creation specified number of days. What would be good practice?

Is it okay to run a script at the beginning of the test to generate the files?

Yep, that's fine with me, BUT:
I can specify days 9999 in openssl which generates a cert until 2055. I'm OK with committing that, as long as it's in a folder clearly labelled "test" or something. We have other repos in the org that do the same thing, and I've seen it in many projects that do testing with SSL.

should we maybe use the one from the flagd-testbed also within here?

I went the easy route & added a new certificate to the PR that expires in 9999 days as suggested by @toddbaert, let me know if there speaks something against it

alexandraoberaigner · 2025-01-08T09:52:27Z

flagd/pkg/service/flag-sync/sync_service_test.go

+				return
+			}
+
+			serviceClient := syncv1grpc.NewFlagSyncServiceClient(con)


[INFO] from this line on the test is the same as before

Signed-off-by: Alexandra Oberaigner <[email protected]>

flagd/pkg/service/flag-sync/sync_service.go

flagd/pkg/service/flag-sync/util_test.go

flagd/pkg/service/flag-sync/test-cert/server-ext.cnf

flagd/pkg/service/flag-sync/util_test.go

Signed-off-by: Alexandra Oberaigner <[email protected]>

aepfli · 2025-01-09T09:35:29Z

flagd/pkg/service/flag-sync/sync_service.go

 func NewSyncService(cfg SvcConfigurations) (*Service, error) {
 	l := cfg.Logger
 	mux, err := NewMux(cfg.Store, cfg.Sources)
 	if err != nil {
 		return nil, fmt.Errorf("error initializing multiplexer: %w", err)
 	}

-	server := grpc.NewServer()
+	var server *grpc.Server
+	tlsCredentials, err := loadTLSCredentials(cfg.CertPath, cfg.KeyPath)


As tls.loadX509...(...) could imply side effects with loading, we should check if CertPath and KeyPath are empty upfront, rather relying on the method itself. Specifically as implementation behind this method can change, and we should only invoke it, if the parameters are set.

aepfli

Thank you. It looks really good, and I love the parameterized test. There is just one thing that we should address, just to be safe.

alexandraoberaigner requested a review from a team as a code owner January 7, 2025 16:35

dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 7, 2025

fix: add ssl support to sync service (open-feature#1479)

80ff0da

Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner force-pushed the fix/1479-bug-not-supporting-ssl branch from ced5619 to 80ff0da Compare January 7, 2025 16:36

fix: lint fixes (open-feature#1479)

6be6990

Signed-off-by: Alexandra Oberaigner <[email protected]>

toddbaert reviewed Jan 7, 2025

View reviewed changes

flagd/pkg/service/flag-sync/sync_service.go Show resolved Hide resolved

beeme1mr changed the title ~~fix: add ssl support to sync service (#1479)~~ feat: add ssl support to sync service (#1479) Jan 7, 2025

toddbaert requested review from bacherfl, Kavindu-Dodan and alemrtv January 7, 2025 18:47

fix: fix lint errors, rename test-cert dir, add certificate with max …

d7a94b4

…days expiration period (open-feature#1479) Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner commented Jan 8, 2025

View reviewed changes

fix: fix lint errors (open-feature#1479)

d97fc8d

Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner force-pushed the fix/1479-bug-not-supporting-ssl branch from ef58650 to d97fc8d Compare January 8, 2025 12:26

fix: fix lint errors (open-feature#1479)

3e7e16e

Signed-off-by: Alexandra Oberaigner <[email protected]>

alexandraoberaigner requested a review from toddbaert January 8, 2025 15:46

warber approved these changes Jan 9, 2025

View reviewed changes

fix: apply review suggestions (open-feature#1479)

feebc44

Signed-off-by: Alexandra Oberaigner <[email protected]>

aepfli reviewed Jan 9, 2025

View reviewed changes

aepfli approved these changes Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ssl support to sync service (#1479) #1501

feat: add ssl support to sync service (#1479) #1501

alexandraoberaigner commented Jan 7, 2025

netlify bot commented Jan 7, 2025 •

edited

Loading

netlify bot commented Jan 7, 2025 •

edited

Loading

alexandraoberaigner commented Jan 7, 2025 •

edited

Loading

toddbaert commented Jan 7, 2025

aepfli commented Jan 7, 2025

toddbaert commented Jan 7, 2025

alexandraoberaigner commented Jan 8, 2025

alexandraoberaigner Jan 8, 2025

aepfli Jan 9, 2025

aepfli left a comment •

edited

Loading

feat: add ssl support to sync service (#1479) #1501

Are you sure you want to change the base?

feat: add ssl support to sync service (#1479) #1501

Conversation

alexandraoberaigner commented Jan 7, 2025

This PR

Related Issues

How to test

netlify bot commented Jan 7, 2025 • edited Loading

✅ Deploy Preview for polite-licorice-3db33c canceled.

netlify bot commented Jan 7, 2025 • edited Loading

✅ Deploy Preview for polite-licorice-3db33c canceled.

alexandraoberaigner commented Jan 7, 2025 • edited Loading

toddbaert commented Jan 7, 2025

aepfli commented Jan 7, 2025

toddbaert commented Jan 7, 2025

alexandraoberaigner commented Jan 8, 2025

alexandraoberaigner Jan 8, 2025

Choose a reason for hiding this comment

aepfli Jan 9, 2025

Choose a reason for hiding this comment

aepfli left a comment • edited Loading

Choose a reason for hiding this comment

netlify bot commented Jan 7, 2025 •

edited

Loading

netlify bot commented Jan 7, 2025 •

edited

Loading

alexandraoberaigner commented Jan 7, 2025 •

edited

Loading

aepfli left a comment •

edited

Loading