feat: external data mutation #1506

sozercan · 2021-08-18T17:12:46Z

Signed-off-by: Sertac Ozercan sozercan@gmail.com

What this PR does / why we need it:
Design doc: https://docs.google.com/document/d/1hPi86jdsCKg8puYT5_s_73mPGExUJeZfyKmvG-XWtPc/edit

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #15 #1293

Special notes for your reviewer:
corresponding frameworks pr: open-policy-agent/frameworks#134

Sample providers:
validation:
https://github.com/sozercan/cosign-provider
https://github.com/sozercan/trivy-provider
mutation:
https://github.com/sozercan/tagToDigest-provider (valueAtLocation)
https://github.com/sozercan/aad-provider (username)

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

codecov-commenter · 2021-08-18T20:13:57Z

Codecov Report

Merging #1506 (b99bf13) into master (a1b50a0) will decrease coverage by 1.27%.
The diff coverage is 18.15%.

@@            Coverage Diff             @@
##           master    #1506      +/-   ##
==========================================
- Coverage   52.25%   50.98%   -1.28%     
==========================================
  Files          82       84       +2     
  Lines        7377     7675     +298     
==========================================
+ Hits         3855     3913      +58     
- Misses       3163     3397     +234     
- Partials      359      365       +6

Flag	Coverage Δ
unittests	`50.98% <18.15%> (-1.28%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/config/config_controller.go	`65.04% <0.00%> (-0.29%)`	⬇️
...onstrainttemplate/constrainttemplate_controller.go	`52.02% <0.00%> (-0.61%)`	⬇️
pkg/mutation/provider_parser.go	`0.00% <0.00%> (ø)`
pkg/mutation/types/mutator.go	`29.16% <0.00%> (-9.73%)`	⬇️
pkg/webhook/policy.go	`30.83% <0.00%> (ø)`
pkg/mutation/mutators/assignmeta_mutator.go	`42.10% <9.09%> (-6.86%)`	⬇️
...controller/externaldata/externaldata_controller.go	`10.44% <10.44%> (ø)`
pkg/mutation/mutators/core/mutation_function.go	`44.11% <18.98%> (-13.28%)`	⬇️
pkg/readiness/ready_tracker.go	`64.55% <23.72%> (-5.94%)`	⬇️
pkg/mutation/system.go	`73.20% <46.66%> (+12.78%)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0238780...b99bf13. Read the comment docs.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

maxsmythe

I had a bunch of pending comments b/c this review was deferred pending a review of the CF PR.

Releasing them to add some more context to the external data discussion.

maxsmythe · 2021-08-19T01:41:53Z

apis/mutations/v1alpha1/assign_types.go

@@ -48,6 +48,8 @@ type Parameters struct {
 	// Assign.value holds the value to be assigned
 	// +kubebuilder:validation:XPreserveUnknownFields
 	Assign runtime.RawExtension `json:"assign,omitempty"`
+
+	ExternalData ExternalData `json:"externalData,omitempty"`


We should add docstrings here so it gets documented in the generated CRD (and thus by kubectl explain), also should ExternalData be a member of Assign? Assign No longer needs to be a RawExtension, now that we have the Schemaless marker for controller-gen.

maxsmythe · 2021-08-19T01:43:37Z

apis/mutations/v1alpha1/assignmetadata_types.go

@@ -36,6 +36,8 @@ type MetadataParameters struct {
 	// Assign.value holds the value to be assigned
 	// +kubebuilder:validation:XPreserveUnknownFields
 	Assign runtime.RawExtension `json:"assign,omitempty"`
+
+	ExternalData ExternalData `json:"externalData,omitempty"`


Same comments about ExternalData as in Assign

maxsmythe · 2021-08-19T01:47:55Z

go.mod

 	sigs.k8s.io/controller-runtime v0.8.3
 	sigs.k8s.io/yaml v1.2.0
 )
+
+replace github.com/open-policy-agent/frameworks/constraint => github.com/sozercan/frameworks/constraint v0.0.0-20210818181827-bf04f25ec1e9


This should be removed before we merge

maxsmythe · 2021-08-19T02:02:13Z

pkg/controller/externaldata/externaldata_controller.go

+	}
+
+	// Watch for changes to Provider
+	return c.Watch(


Eventually we'll want to report status on a provider

maxsmythe · 2021-08-19T02:10:35Z

pkg/controller/mutators/assign/assign_controller.go

 		Kind:           "Assign",
 		NewMutationObj: func() client.Object { return &mutationsv1alpha1.Assign{} },
 		MutatorFor: func(obj client.Object) (types.Mutator, error) {
 			// The type is provided by the `NewObj` function above. If we
 			// are fed the wrong type, this is a non-recoverable error and we
 			// may as well crash for visibility
 			assign := obj.(*mutationsv1alpha1.Assign) // nolint:forcetypeassert
-			return mutators.MutatorForAssign(assign)
+			return mutators.MutatorForAssign(assign, a.ProviderCache)


Injecting the provider cache into the mutators seems brittle... could we inject them into the mutation system and system can provide the context at mutation time? Unless there would be a reason for different mutators to have different provider caches?

maxsmythe · 2021-08-19T02:29:58Z

pkg/mutation/mutators/assignmeta_mutator.go

@@ -114,8 +120,24 @@ func (m *AssignMetadataMutator) String() string {
 	return fmt.Sprintf("%s/%s/%s:%d", m.id.Kind, m.id.Namespace, m.id.Name, m.assignMetadata.GetGeneration())
 }

+func (m *AssignMetadataMutator) GetExternalDataProvider() string {


Because this is an implementation detail of how mutators work, maybe we can hide this?

Maybe we can leverage the Setter interface I propose in the ModifySet PR?

#1508

maxsmythe · 2021-08-19T02:30:39Z

pkg/mutation/mutators/assignmeta_mutator.go

+}
+
+func (m *AssignMetadataMutator) GetExternalDataCache(name string) (*externaldatav1alpha1.Provider, error) {
+	data, err := m.providerCache.Get(name)


What happens if the data provider hasn't yet been cached due to eventual consistency?

maxsmythe · 2021-08-19T02:32:22Z

pkg/mutation/mutators/assignmeta_mutator.go

@@ -173,10 +202,14 @@ func isValidMetadataPath(path parser.Path) bool {
 	return false
 }

+func isValidExternalDataSource(source types.DataSource) bool {
+	return source == types.Username


Can we also enforce this at the JSONSchema validation level?

maxsmythe · 2021-08-19T02:35:38Z

pkg/mutation/mutators/core/mutation_function.go

@@ -14,18 +18,83 @@ import (

 var log = logf.Log.WithName("mutation").WithValues(logging.Process, "mutation")

-func Mutate(mutator types.Mutator, tester *path.Tester, valueTest func(interface{}, bool) bool, obj *unstructured.Unstructured) (bool, error) {
+func Mutate(mutator types.Mutator, tester *path.Tester, valueTest func(interface{}, bool) bool, obj *unstructured.Unstructured, providerResponseCache map[types.ProviderCacheKey]string) (bool, error) {


We can also avoid the need to pass providerResponseCache explicitly with the setter interface

maxsmythe · 2021-08-19T02:39:16Z

pkg/mutation/mutators/core/mutation_function.go

+		if providerName != "" {
+			providerStore, err := mutator.GetExternalDataCache(providerName)
+			if err != nil {
+				return nil, false, fmt.Errorf("failed to get external data provider cache: %v", err)


This will cause requests to fail due to eventual consistency whenever a new provider is added, which might nullify all mutators. Is this the behavior that we want?

sozercan · 2022-03-04T01:20:39Z

closing in favor of #1891

sozercan added 3 commits August 18, 2021 03:38

feat: external data

78d9255

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Merge branch 'master' into edp

7bcfa05

vendor frameworks 061d1400c729

4925cbe

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan requested review from ritazh and maxsmythe August 18, 2021 17:12

fix lint and vendor

710c556

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan linked an issue Aug 18, 2021 that may be closed by this pull request

Access external data to build a secure supply chain #1293

Closed

fix helm

dacdce8

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan and others added 2 commits August 19, 2021 09:48

Merge branch 'master' into edp

f7d1668

makefile update and e2e tests

b99bf13

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan force-pushed the edp branch from 96ec5a8 to b99bf13 Compare August 20, 2021 23:19

vendor frameworks e35c157

56189af

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

sozercan force-pushed the edp branch from 24434e8 to 56189af Compare August 23, 2021 22:03

maxsmythe requested changes Sep 7, 2021

View reviewed changes

This was referenced Sep 23, 2021

initial external data integration for validation #1573

Merged

Tracking external data features #1576

Closed

ritazh added this to the External data alpha milestone Sep 27, 2021

sozercan marked this pull request as draft October 2, 2021 00:39

sozercan changed the title ~~feat: external data~~ feat: external data mutation Oct 26, 2021

chewong mentioned this pull request Mar 3, 2022

feat: external data mutation #1891

Merged

3 tasks

sozercan closed this Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: external data mutation #1506

feat: external data mutation #1506

sozercan commented Aug 18, 2021 •

edited

Loading

codecov-commenter commented Aug 18, 2021 •

edited

Loading

maxsmythe left a comment

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

maxsmythe Aug 19, 2021

sozercan commented Mar 4, 2022

feat: external data mutation #1506

feat: external data mutation #1506

Conversation

sozercan commented Aug 18, 2021 • edited Loading

codecov-commenter commented Aug 18, 2021 • edited Loading

Codecov Report

maxsmythe 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

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

sozercan commented Mar 4, 2022

sozercan commented Aug 18, 2021 •

edited

Loading

codecov-commenter commented Aug 18, 2021 •

edited

Loading