feature/2268: added support for merging fields from multiple secrets files #9590

patrickhuie19 · 2023-06-14T14:04:40Z

https://smartcontract-it.atlassian.net/browse/BCF-2268

Currently multiple secrets files may be provided, but only the fields from the first provided file are processed.

With this change, secrets configuration fields split into multiple secrets files will be merged together. Overrides will fail. For maps and lists, true merging behavior is honored. There is only a map element currently, so if list secrets configuration fields are added in the future it will have to respect this pattern.

Validation is performed on the final merged secrets file. Duplicates within files err out as they do today.

github-actions · 2023-06-14T14:05:14Z

I see that you haven't updated any README files. Would it make sense to do so?

core/config/v2/types.go

core/services/chainlink/config_general.go

zytek · 2023-06-14T19:01:08Z

Overrides will fail.

why ?
is this similar to config behaviour?

is there any reason to have this behave differently than config.toml merging? just/mostly curious. not sure if I see a pattern for overriding secrets, but this is something we DO use for config - to provide default values, but give ability to override. I'd be +1 on making this consistent with config, but if you have a scenario where this is unsafe I won't fight for it.

jmank88 · 2023-06-14T20:20:29Z

Overrides will fail.

why ? is this similar to config behaviour?

is there any reason to have this behave differently than config.toml merging? just/mostly curious. not sure if I see a pattern for overriding secrets, but this is something we DO use for config - to provide default values, but give ability to override. I'd be +1 on making this consistent with config, but if you have a scenario where this is unsafe I won't fight for it.

The thinking was that if you override a secret value that means you are leaking extra secrets somewhere they are not necessary, which seems inherently unsafe.

core/config/v2/types.go

jmank88 · 2023-06-20T16:55:50Z

core/cmd/app_test.go

+				URL:                  models.NewSecretURL(models.MustParseURL("xxxxx")),
+				BackupURL:            models.NewSecretURL(models.MustParseURL("xxxxx")),


Why does explicit xxxxx need to be used here? I'm worried that could mask problems with the printing protection, since it matches w/o conversion and could lead to a false positive. Comparing either the raw TOML strings, or Go types with real values would be cleaner, if possible.

Hmm, yea - good point!

IMO, three things should be tested here:

Merge is successful for all non-overriding operations

Merge fails in presence of any overriding operations

Verify merge results in the correct ending value of secrets

The first two test cases are addressed sufficiently through the current changes. The third test case is not. The crux is that when the new merging is performed, New(), is also functionally coupled with the redaction of the secrets. The redaction logic is defined in core/store/models/secrets.go. As an aside, I believe it makes sense that tests 1 and 2 are performed in app_test.go since end to end error and success conditions for use of the node are tested here.

I've split the merging and redaction functionality in the next commit to isolate and unit test the merging functionality in config_general_test.go. However - given how the secret types are implemented, there is no direct way to compare by types or by toml string. I'd either have to create a new type for testing that is similar to the Secrets type, or implement a new toml string outputter that won't redact the secret values. The latter approach seems like it could be potentially abused. The solution I’ve landed on is comparing the values in opts before New() is called and the values behind the Secret and SecretURL types in opts after New() is called through the use of a new TestSecrets type.

I'm not sure that I follow. The redaction logic prevents printing/marshaling directly, but the values are still directly available and comparable. If these tests depend on the literal value being xxxxx, then something seems wrong. If they don't depend on any particular values, then that also seems wrong. I thought we would have real values here in the Go types, and a raw TOML string of redacted types elsewhere. Are we reading in a redacted file somewhere or something like that?

No, like you mentioned on a comment below - by design it's not possible to go from the secret types to non-redacted values in the TOML. I'll use raw TOML as you suggested, it should simplify all of the ...workarounds I used for this revision to test that third case 😆

core/store/models/secrets.go

core/config/v2/validate.go

jmank88 · 2023-06-21T01:27:20Z

core/utils/files.go

+func MakeTestFile(t *testing.T, contents any, fileName string) string {
+	d := t.TempDir()
+	p := filepath.Join(d, fileName)
+
+	b, err := toml.Marshal(contents)


Oh, this is the problem - you can't write secrets like this via the existing types, by design. It will always just print redacted values. Can we use raw TOML here instead?

You can use package embed https://pkg.go.dev/embed to make regular files available to the test.

core/services/chainlink/config_general_test.go

core/cmd/app_test.go

jmank88 · 2023-07-10T22:27:59Z

core/utils/files.go

@@ -170,3 +175,14 @@ func (s FileSize) String() string {
 	str, _ := s.MarshalText()
 	return string(str)
 }
+
+func MakeTestFile(t *testing.T, contents any, fileName string) string {


Does this need to be a general util func? Can we limit it to tests instead? Or even a toml config test package?
I think we should document the expectations with secrets too, so this isn't mistakenly assumed to be able to marshal secret values.

Suggested change

func MakeTestFile(t *testing.T, contents any, fileName string) string {

func WriteTOMLFile(t *testing.T, contents any, fileName string) string {

Thank you, good call. Addressed in the next revision, added a comment in the following commit.

jmank88 · 2023-07-12T21:55:10Z

core/cmd/app_test.go

+					testtomlutils.WriteTOMLFile(t, chainlink.Secrets{Secrets: toml.Secrets{Threshold: testSecretsFileContentsComplete.Threshold}}, "test_secrets7.toml"),
+				},
+			},
+			wantCfg: withDefaults(t, testConfigFileContents, testSecretsRedactedContentsComplete),


Maybe I lost the thread somewhere:

Why do we still need to marshal redacted values? I thought that reading real values from files would work around that.

If we are stuck with this, then can we at least use the redacted Go values for both parts (both testSecretsRedactedContentsComplete instead of testSecretsFileContentsComplete too)? I think it will be confusing for future readers to see real values being serialized and implicitly converted.

I was sticking with the earlier pattern, but I agree that reading real values from files is a better approach. I've updated in the next revision, thanks!

…files

…tion test

…in test

…eTOMLFile and from general utilities to testtomlutils

…ead of implementing it ourselves

… to use persistent toml test fixtures

…o chainlink package

cl-sonarqube-production · 2023-07-17T17:53:21Z

SonarQube Quality Gate

79.5% Coverage on New Code (is less than 80%)

See analysis details on SonarQube

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch 5 times, most recently from fcb4f00 to b16b364 Compare June 14, 2023 17:32

patrickhuie19 marked this pull request as ready for review June 14, 2023 17:51

patrickhuie19 requested review from jkongie, jmank88 and samsondav as code owners June 14, 2023 17:51

jmank88 reviewed Jun 14, 2023

View reviewed changes

core/config/v2/types.go Outdated Show resolved Hide resolved

jmank88 reviewed Jun 14, 2023

View reviewed changes

core/config/v2/types.go Outdated Show resolved Hide resolved

jmank88 reviewed Jun 14, 2023

View reviewed changes

core/services/chainlink/config_general.go Outdated Show resolved Hide resolved

patrickhuie19 requested a review from jmank88 June 20, 2023 16:04

jmank88 reviewed Jun 20, 2023

View reviewed changes

core/config/v2/types.go Outdated Show resolved Hide resolved

jmank88 reviewed Jun 20, 2023

View reviewed changes

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch from 6912bb4 to 48cdd38 Compare June 21, 2023 00:53

patrickhuie19 commented Jun 21, 2023

View reviewed changes

core/store/models/secrets.go Outdated Show resolved Hide resolved

jmank88 reviewed Jun 21, 2023

View reviewed changes

core/config/v2/validate.go Outdated Show resolved Hide resolved

jmank88 reviewed Jun 21, 2023

View reviewed changes

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch from c22bbe5 to f4caebf Compare June 28, 2023 15:20

patrickhuie19 requested a review from jmank88 July 10, 2023 16:17

jmank88 reviewed Jul 10, 2023

View reviewed changes

core/services/chainlink/config_general_test.go Outdated Show resolved Hide resolved

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch from 719daa8 to 3907c49 Compare July 10, 2023 20:52

patrickhuie19 requested a review from jmank88 July 10, 2023 21:55

jmank88 reviewed Jul 10, 2023

View reviewed changes

core/cmd/app_test.go Outdated Show resolved Hide resolved

jmank88 reviewed Jul 10, 2023

View reviewed changes

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch from 7050a83 to 11604ac Compare July 11, 2023 14:03

patrickhuie19 requested a review from a team as a code owner July 11, 2023 21:20

patrickhuie19 requested a review from jmank88 July 12, 2023 13:36

jmank88 reviewed Jul 12, 2023

View reviewed changes

patrickhuie19 added 13 commits July 17, 2023 10:17

feature/2268: added support for merging fields from multiple secrets …

408f8a6

…files

feature/2268: minor refactoring

0a3b9d5

feature/2268-multiple-secrets-files: adding non-redacted merge valida…

cef366a

…tion test

feature/2268-multiple-secrets-files: simplifying merge validation test

c7b75c9

feature/2268-multiple-secrets-files: minor refactoring

543a1d8

feature/2268-multiple-secrets-files: fixing incorrect equality check …

df9114f

…in test

feature/2268-multiple-secrets-files: minor refactoring

b6521ba

feature/2268-multiple-secrets-files: refactoring MakeTestFile => Writ…

03514dc

…eTOMLFile and from general utilities to testtomlutils

feature/2268-multiple-secrets-files: fixing failing tests

2a1edd5

feature/2268-multiple-secrets-files: cleaning up unused import

e55e58a

feature/2268-multiple-secrets-files: using golang.org/x/exp/maps inst…

9f0baeb

…ead of implementing it ourselves

feature/2268-multiple-secrets-files: refactoring

4afd232

feature/2268-multiple-secrets-files: refactoring core/cmd/app_test.go…

4ee98cc

… to use persistent toml test fixtures

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch 2 times, most recently from 4e7dee0 to a432c83 Compare July 17, 2023 16:38

feature/2268-multiple-secrets-files: refactoring test fixtures back t…

b61652a

…o chainlink package

patrickhuie19 force-pushed the feature/2268-multiple-secrets-files branch from a432c83 to b61652a Compare July 17, 2023 17:37

patrickhuie19 requested a review from jmank88 July 17, 2023 18:11

jmank88 approved these changes Jul 18, 2023

View reviewed changes

patrickhuie19 added this pull request to the merge queue Jul 18, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 18, 2023

patrickhuie19 added this pull request to the merge queue Jul 18, 2023

Merged via the queue into develop with commit e67f492 Jul 18, 2023
94 of 95 checks passed

patrickhuie19 deleted the feature/2268-multiple-secrets-files branch July 18, 2023 14:08

patrickhuie19 mentioned this pull request Aug 7, 2023

TT-467 migrate VRFv2 smoke test #10067

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/2268: added support for merging fields from multiple secrets files #9590

feature/2268: added support for merging fields from multiple secrets files #9590

patrickhuie19 commented Jun 14, 2023 •

edited

Loading

github-actions bot commented Jun 14, 2023

zytek commented Jun 14, 2023

jmank88 commented Jun 14, 2023

jmank88 Jun 20, 2023

patrickhuie19 Jun 21, 2023

jmank88 Jun 21, 2023

patrickhuie19 Jun 26, 2023

jmank88 Jun 21, 2023

jmank88 Jun 21, 2023

jmank88 Jul 10, 2023

patrickhuie19 Jul 11, 2023

jmank88 Jul 12, 2023

patrickhuie19 Jul 17, 2023

cl-sonarqube-production bot commented Jul 17, 2023

		URL: models.NewSecretURL(models.MustParseURL("xxxxx")),
		BackupURL: models.NewSecretURL(models.MustParseURL("xxxxx")),

	func MakeTestFile(t *testing.T, contents any, fileName string) string {
	func WriteTOMLFile(t *testing.T, contents any, fileName string) string {

feature/2268: added support for merging fields from multiple secrets files #9590

feature/2268: added support for merging fields from multiple secrets files #9590

Conversation

patrickhuie19 commented Jun 14, 2023 • edited Loading

github-actions bot commented Jun 14, 2023

zytek commented Jun 14, 2023

jmank88 commented Jun 14, 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

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Jul 17, 2023

patrickhuie19 commented Jun 14, 2023 •

edited

Loading