Add configuration for data residency #1681

zhongduo · 2020-09-10T16:00:20Z

Proposed Changes

Add a new configuration map for data residency
Update Topic CRD reconciler to use data residency configuration map
Update broker and trigger reconciler to use data residency configuration when creating topics.

Release Note

- 🎁 Add data residency support through configuration map.

Docs

zhongduo · 2020-09-10T16:02:17Z

This PR and the issue is open to input about if we should keep a config map called "config-data-residency" or make it larger scope to "config-gcp-features" which is what I am using now.

zhongduo · 2020-09-10T19:36:08Z

/assign @yolocs

Harwayne · 2020-09-11T05:57:13Z

config/core/configmaps/data-residency.yaml

+    # messagestoragepolicy.allowedpersistenceregions field specifies
+    # all the allowed regions for data residency. The default or an empty value will 
+    # mean no data residency requirement.
+    messagestoragepolicy.allowedpersistenceregions: "us-east1,us-west1"


Since we are essentially dealing with YAML, can this be a list of strings, rather than a csv?

Suggested change

messagestoragepolicy.allowedpersistenceregions: "us-east1,us-west1"

messagestoragepolicy.allowedpersistenceregions:

- us-east1

- us-west1

Agreed. This field is a field used in the topic config, I was thinking to simply pass whatever in the field to the topic config, but I think the topic is also using []string.

yolocs

Update the release note section in the description with something like "Add data residency configmap placeholder"

config/core/configmaps/data-residency.yaml

yolocs · 2020-09-11T15:46:30Z

config/core/configmaps/data-residency.yaml

+  name: config-data-residency
+  namespace: cloud-run-events
+data:
+  _example: |


I'm fine with creating such a placeholder since it does nothing. But I'm looking for more details on how this will be used.

How this configmap will be read? Volume mount? Watch?

Do you allow finer grained control? Like per broker? How it would impact the configmap structure?

I will add the actual code for this configuration to this PR too. For your question:

This is still being discussed, but watch seems better since our system already uses watch for other config maps, also, it provides easier interface for reacting to the change.

Personally I think even if we allow per broker configuration, this will still serve as a default and same configuration will be added to per broker and get reconciled by the broker controller instead.

This PR has been extended to have the full implementation, so to answer this original question:

The configmap is read through watch from Store object.

For now, we only support cluster wide, but we can for sure add per namespace or broker in the future. Note that triggers also create new topics, but topics for retry should be same as the main broker topic.

zhongduo · 2020-09-11T22:21:30Z

Open for suggestions: I have added a default config to the data residency. However, since the data residency really is optional, I am treating empty or non key as no configuration so that it will allow all region too. The only downside I can think of is that it will not catch typos in keys like

"datareside.error-key':
    - us-east-1

Any suggestions?

zhongduo · 2020-09-14T14:33:59Z

I personally think it is better to make this its own commit, and the logic to propagate to controller etc another one, as it will make the history look cleaner.

…ration

…ult.go

…em, rename config-data-residency.yaml to make it consistent with other files

grantr

Thanks @zhongduo!

Looks like we're missing a few tests. All my other comments are optional nits.

grantr · 2020-09-17T20:28:17Z

config/core/configmaps/data-residency.yaml

+
+    # default-dataresidency-config is the configuration for determining the default
+    # data residency to apply to all objects that require data residency but,
+    # do not specify it. This is expected to be Channels and Sources.


Remove but, do not specify it. That is already implied by "default data residency".

Fix as suggested.

grantr · 2020-09-17T20:30:18Z

config/core/configmaps/data-residency.yaml

+
+    # default-dataresidency-config is the configuration for determining the default
+    # data residency to apply to all objects that require data residency but,
+    # do not specify it. This is expected to be Channels and Sources.


I think it's Channels, Sources, and Brokers

Fixed as suggested.

grantr · 2020-09-17T20:32:49Z

pkg/apis/configs/dataresidency/dataresidency_defaults.go

+
+const (
+	// configName is the name of config map for the default data residency that
+	// Sources and brokers should use.


Maybe clearer to say "the default data residency that GCP resources should use"

Fixed as suggested.

pkg/apis/configs/dataresidency/defaults.go

pkg/reconciler/broker/broker.go

pkg/reconciler/intevents/topic/topic.go

pkg/reconciler/trigger/trigger.go

pkg/reconciler/intevents/topic/topic.go

pkg/reconciler/trigger/trigger.go

grantr

Very close. I have one more testing request (test for empty AllowedPersistenceRegions) and one optional suggestion.

grantr · 2020-09-18T18:12:39Z

pkg/reconciler/broker/broker_test.go

+    messagestoragepolicy.allowedpersistenceregions:
+      - us-east1`,
+				},
+			},


Optional: I recommend moving this declaration to a test helper function so it takes up less space and can be shared across multiple reconcilers. Something like:

"dataResidencyConfigMap": DataResidencyConfigMap("us-east1")

Test helpers are in pkg/reconciler/testing.

Fixed as suggested.

grantr · 2020-09-18T18:15:51Z

pkg/reconciler/broker/broker_test.go

@@ -304,6 +308,63 @@ func TestAllCases(t *testing.T) {
 			TopicExists("cre-bkr_testnamespace_test-broker_abc123"),


We can increase coverage by changing this to TopicExistsWithConfig so it verifies AllowedPersistenceRegions is empty by default. Would you mind making that change here and in the trigger tests?

Fixed as suggested.

pkg/reconciler/broker/broker_test.go

pkg/reconciler/intevents/topic/topic.go

grantr · 2020-09-18T19:18:12Z

The only downside I can think of is that it will not catch typos in keys like

This is fine for now - it's the same behavior for our other configmaps. A future improvement can add validation webhooks to verify configmap structure.

grantr · 2020-09-18T19:19:43Z

it's the same behavior for our other configmaps.

Oops, I was wrong about this. We do validate the structure of other configmaps, see https://github.com/google/knative-gcp/blob/master/cmd/webhook/main.go#L171. Adding that can be a followup PR.

knative-metrics-robot · 2020-09-18T20:11:57Z

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/configs/dataresidency/dataresidency_defaults.go	Do not exist	84.6%
pkg/apis/configs/dataresidency/defaults.go	Do not exist	100.0%
pkg/apis/configs/dataresidency/singleton.go	Do not exist	100.0%
pkg/apis/configs/dataresidency/store.go	Do not exist	76.9%
pkg/reconciler/broker/broker.go	62.0%	64.0%	2.0
pkg/reconciler/broker/controller.go	59.4%	60.6%	1.2
pkg/reconciler/intevents/topic/topic.go	69.6%	68.4%	-1.3
pkg/reconciler/trigger/controller.go	68.4%	69.2%	0.8
pkg/reconciler/trigger/trigger.go	56.4%	57.7%	1.3

grantr

Thanks @zhongduo!

/lgtm

knative-prow-robot · 2020-09-18T21:07:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, zhongduo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [grantr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zhongduo · 2020-09-18T22:30:21Z

/test pull-google-knative-gcp-wi-tests

zhongduo · 2020-09-19T00:56:56Z

/retest

zhongduo · 2020-09-19T01:33:21Z

/retest

knative-test-reporter-robot · 2020-09-19T01:43:12Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-wi-tests	2020-09-18 20:20:08.416 +0000 UTC 2020-09-18 21:00:38.547 +0000 UTC 2020-09-18 21:13:08.231 +0000 UTC 2020-09-18 21:56:08.487 +0000 UTC 2020-09-18 23:12:08.23 +0000 UTC 2020-09-19 01:43:08.462 +0000 UTC	3/3

Job pull-google-knative-gcp-wi-tests expended all 3 retries without success.

zhongduo · 2020-09-19T01:46:41Z

/test pull-google-knative-gcp-wi-tests

knative-prow-robot requested review from capri-xiyue and grantr September 10, 2020 16:00

google-cla bot added the cla: yes (override cla status due to multiple authors bug) label Sep 10, 2020

knative-prow-robot added the size/M label Sep 10, 2020

knative-prow-robot assigned yolocs Sep 10, 2020

zhongduo force-pushed the data-residency-config branch from 8e15fb7 to 0b884c2 Compare September 10, 2020 20:05

Harwayne reviewed Sep 11, 2020

View reviewed changes

zhongduo force-pushed the data-residency-config branch from e4b205d to 4179e22 Compare September 11, 2020 13:27

yolocs reviewed Sep 11, 2020

View reviewed changes

zhongduo changed the title ~~add configuration for data residency~~ WIP: add configuration for data residency Sep 11, 2020

knative-prow-robot added do-not-merge/work-in-progress size/L and removed size/M labels Sep 11, 2020

knative-prow-robot added size/XL and removed size/L labels Sep 14, 2020

zhongduo changed the title ~~WIP: add configuration for data residency~~ Add configuration for data residency Sep 14, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Sep 14, 2020

zhongduo requested review from yolocs and Harwayne September 14, 2020 22:04

zhongduo changed the title ~~Add configuration for data residency~~ WIP: Add configuration for data residency Sep 16, 2020

knative-prow-robot added the do-not-merge/work-in-progress label Sep 16, 2020

zhongduo changed the title ~~WIP: Add configuration for data residency~~ Add configuration for data residency Sep 16, 2020

knative-prow-robot removed the do-not-merge/work-in-progress label Sep 16, 2020

Jimmy Lin added 3 commits September 16, 2020 13:11

add configuration map for data residency

d80e60f

Use array instead of csv string for allowedpersistenceregions configu…

6df2667

…ration

Add default to the data residency config

42b688b

Jimmy Lin added 2 commits September 16, 2020 13:11

Add data residency support to broker and trigger, not using injection

2a5488d

Move the common AllowedRegionPolicy computation to dataresidency/defa…

a61b0d5

…ult.go

zhongduo force-pushed the data-residency-config branch from db592fc to a61b0d5 Compare September 16, 2020 17:29

Jimmy Lin added 3 commits September 16, 2020 14:38

Fix unit test failure for broker and trigger

6598081

Add more test to TestComputeAllowedPersistenceRegions

061dc41

Add singleton test for data residency to work around the covery probl…

f39db1d

…em, rename config-data-residency.yaml to make it consistent with other files

zhongduo mentioned this pull request Sep 17, 2020

Use wire-gen injection for all controllers #1733

Merged

grantr reviewed Sep 17, 2020

View reviewed changes

Jimmy Lin added 3 commits September 17, 2020 19:13

Resolve comments from PR

eec7f56

Add test to broker to test data residency configuration and labels

450db2f

DataResidency test added to trigger

f5035a5

grantr reviewed Sep 18, 2020

View reviewed changes

This was referenced Sep 18, 2020

No unit tests for data residency config in Topic reconciler #1740

Closed

Data residency configmap is not validated by webhook #1741

Closed

Jimmy Lin added 2 commits September 18, 2020 15:46

Add empty data residency config

dce71de

Add helper function for data residency configuration map creation

e4dbb1d

grantr approved these changes Sep 18, 2020

View reviewed changes

knative-prow-robot assigned grantr Sep 18, 2020

knative-prow-robot added the lgtm label Sep 18, 2020

knative-prow-robot added the approved label Sep 18, 2020

knative-prow-robot merged commit 1281791 into google:master Sep 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration for data residency #1681

Add configuration for data residency #1681

zhongduo commented Sep 10, 2020 •

edited

Loading

zhongduo commented Sep 10, 2020

zhongduo commented Sep 10, 2020

Harwayne Sep 11, 2020

zhongduo Sep 11, 2020

yolocs left a comment

yolocs Sep 11, 2020

zhongduo Sep 11, 2020

zhongduo Sep 18, 2020

zhongduo commented Sep 11, 2020

zhongduo commented Sep 14, 2020

grantr left a comment

grantr Sep 17, 2020

zhongduo Sep 17, 2020

grantr Sep 17, 2020

zhongduo Sep 17, 2020

grantr Sep 17, 2020

zhongduo Sep 17, 2020

grantr left a comment

grantr Sep 18, 2020

zhongduo Sep 18, 2020

grantr Sep 18, 2020

zhongduo Sep 18, 2020

grantr commented Sep 18, 2020

grantr commented Sep 18, 2020

knative-metrics-robot commented Sep 18, 2020

grantr left a comment

knative-prow-robot commented Sep 18, 2020

zhongduo commented Sep 18, 2020

zhongduo commented Sep 19, 2020

zhongduo commented Sep 19, 2020

knative-test-reporter-robot commented Sep 19, 2020

zhongduo commented Sep 19, 2020

		@@ -304,6 +308,63 @@ func TestAllCases(t *testing.T) {
		TopicExists("cre-bkr_testnamespace_test-broker_abc123"),

Add configuration for data residency #1681

Add configuration for data residency #1681

Conversation

zhongduo commented Sep 10, 2020 • edited Loading

Proposed Changes

zhongduo commented Sep 10, 2020

zhongduo commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yolocs left a 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

zhongduo commented Sep 11, 2020

zhongduo commented Sep 14, 2020

grantr left a 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

grantr left a 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

grantr commented Sep 18, 2020

grantr commented Sep 18, 2020

knative-metrics-robot commented Sep 18, 2020

grantr left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 18, 2020

zhongduo commented Sep 18, 2020

zhongduo commented Sep 19, 2020

zhongduo commented Sep 19, 2020

knative-test-reporter-robot commented Sep 19, 2020

zhongduo commented Sep 19, 2020

zhongduo commented Sep 10, 2020 •

edited

Loading