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	`3e7e16e`
🔍 Latest deploy log	https://app.netlify.com/sites/polite-licorice-3db33c/deploys/677e9c8e843f3e0008842a4b

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]>

warber · 2025-01-09T08:18:32Z

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

+	// Load server's certificate and private key
+	serverCert, err := tls.LoadX509KeyPair(certPath, keyPath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to load key pair from given certificate paths: '%s' and '%s'", certPath, keyPath)


Suggested change

return nil, fmt.Errorf("failed to load key pair from given certificate paths: '%s' and '%s'", certPath, keyPath)

return nil, fmt.Errorf("failed to load key pair from certificate paths '%s' and '%s': %w", certPath, keyPath, err)

you could consider wrapping the error returned by LoadX509KeyPair() to preserve the context.

warber · 2025-01-09T08:36:26Z

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

@@ -30,3 +36,24 @@ func getSimpleFlagStore() (*store.Flags, []string) {

 	return flagStore, []string{"A", "B", "C"}
 }
+
+func LoadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {


Suggested change

func LoadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {

func loadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {

warber · 2025-01-09T08:36:54Z

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

@@ -0,0 +1 @@
+subjectAltName=IP:0.0.0.0


nit: missing new line

warber · 2025-01-09T08:39:03Z

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

+
+func LoadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {
+	// Load certificate of the CA who signed server's certificate
+	pemServerCA, err := ioutil.ReadFile(certPath)


Suggested change

pemServerCA, err := ioutil.ReadFile(certPath)

pemServerCA, err := os.ReadFile(certPath)

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

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

warber Jan 9, 2025

warber Jan 9, 2025

warber Jan 9, 2025

warber Jan 9, 2025

	return nil, fmt.Errorf("failed to load key pair from given certificate paths: '%s' and '%s'", certPath, keyPath)
	return nil, fmt.Errorf("failed to load key pair from certificate paths '%s' and '%s': %w", certPath, keyPath, err)

	func LoadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {
	func loadTLSClientCredentials(certPath string) (credentials.TransportCredentials, error) {

	pemServerCA, err := ioutil.ReadFile(certPath)
	pemServerCA, err := os.ReadFile(certPath)

		@@ -0,0 +1 @@
		subjectAltName=IP:0.0.0.0

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

warber Jan 9, 2025

Choose a reason for hiding this comment

warber Jan 9, 2025

Choose a reason for hiding this comment

warber Jan 9, 2025

Choose a reason for hiding this comment

warber Jan 9, 2025

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