more changes

cmd/skaffold/app/cmd/flags.go

@@ -600,17 +600,17 @@ var flagRegistry = []Flag{
 		DefinedOn:     []string{"deploy"},
 	},
 	{
-		Name:          "transformableAllowListFile",
+		Name:          "transformAllowFile",
 		Usage:         "Path to JSON file specifying the allow list of yaml objects for skaffold to NOT transform with 'image' and 'label' field replacements. NOTE: this list is additive to skaffold's default allowlist",
-		Value:         &opts.TransformAllowListFile,
+		Value:         &opts.TransformAllowFile,
 		DefValue:      "",
 		FlagAddMethod: "StringVar",
 		DefinedOn:     []string{"dev", "run", "debug", "deploy"},
 	},
 	{
-		Name:          "transformableDenyListFile",
+		Name:          "transformDenyFile",
 		Usage:         "Path to JSON file specifying the deny list of yaml objects for skaffold to NOT transform with 'image' and 'label' field replacements.  NOTE: this list is additive to skaffold's default denylist",
-		Value:         &opts.TransformDenyListFile,
+		Value:         &opts.TransformDenyFile,
 		DefValue:      "",
 		FlagAddMethod: "StringVar",
 		DefinedOn:     []string{"dev", "run", "debug", "deploy"},

pkg/skaffold/config/options.go

@@ -52,8 +52,8 @@ type SkaffoldOptions struct {
 	RepoCacheDir               string
 	Trigger                    string
 	User                       string
-	TransformAllowListFile string
-	TransformDenyListFile  string
+	TransformAllowFile string
+	TransformDenyFile  string
 
 	ConfigurationFilter []string
 	CustomLabels        []string

pkg/skaffold/deploy/kpt/kpt.go

@@ -126,22 +126,35 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.KptDep
 	logger := component.NewLogger(cfg, kubectl, podSelector, &namespaces)
 
 	transformableAllowlist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add default values
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+	}
 	for _, rf := range cfg.TransformAllowList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableAllowlist[groupKind] = rf
 	}
 
-	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add user schema values, override defaults
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+		delete(transformableDenylist, groupKind)
+	}
 	for _, rf := range cfg.TransformDenyList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableDenylist[groupKind] = rf
+		delete(transformableAllowlist, groupKind)
 	}
 
+	// add user flag values, override user schema values and defaults
 	// TODO(aaron-prindle) verfiy if workdir needs to be considered more in this read...
 	if cfg.GetTransformAllowListFile() != "" {
 		transformableAllowlistFromFile, err := ioutil.ReadFile(cfg.GetTransformAllowListFile())
 		if err != nil {
-			olog.Entry(context.TODO()).Errorf("encountered error parsing --transformableAllowListFile=%s: %v")
+			return nil, err
 		}
 		rfs := []latestV1.ResourceFilter{}
 		err = json.Unmarshal(transformableAllowlistFromFile, &rfs)
@@ -150,7 +163,8 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.KptDep
 		}
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
-			transformableDenylist[groupKind] = rf
+			transformableAllowlist[groupKind] = rf
+			delete(transformableDenylist, groupKind)
 		}
 	}
 
@@ -169,6 +183,7 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.KptDep
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
 			transformableDenylist[groupKind] = rf
+			delete(transformableAllowlist, groupKind)
 		}
 	}
 	return &Deployer{

pkg/skaffold/deploy/kubectl/kubectl.go

@@ -108,17 +108,30 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.Kubect
 	logger := component.NewLogger(cfg, kubectl.CLI, podSelector, &namespaces)
 
 	transformableAllowlist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add default values
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+	}
 	for _, rf := range cfg.TransformAllowList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableAllowlist[groupKind] = rf
 	}
 
-	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add user schema values, override defaults
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+		delete(transformableDenylist, groupKind)
+	}
 	for _, rf := range cfg.TransformDenyList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableDenylist[groupKind] = rf
+		delete(transformableAllowlist, groupKind)
 	}
 
+	// add user flag values, override user schema values and defaults
 	// TODO(aaron-prindle) verfiy if workdir needs to be considered more in this read...
 	if cfg.GetTransformAllowListFile() != "" {
 		transformableAllowlistFromFile, err := ioutil.ReadFile(cfg.GetTransformAllowListFile())
@@ -132,7 +145,8 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.Kubect
 		}
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
-			transformableDenylist[groupKind] = rf
+			transformableAllowlist[groupKind] = rf
+			delete(transformableDenylist, groupKind)
 		}
 	}
 
@@ -151,6 +165,7 @@ func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latestV1.Kubect
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
 			transformableDenylist[groupKind] = rf
+			delete(transformableAllowlist, groupKind)
 		}
 	}
 

pkg/skaffold/deploy/kustomize/kustomize.go

@@ -156,17 +156,30 @@ func NewDeployer(cfg kubectl.Config, labeller *label.DefaultLabeller, d *latestV
 	}
 	logger := component.NewLogger(cfg, kubectl.CLI, podSelector, &namespaces)
 	transformableAllowlist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add default values
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+	}
 	for _, rf := range cfg.TransformAllowList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableAllowlist[groupKind] = rf
 	}
 
-	transformableDenylist := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
+	// add user schema values, override defaults
+	for _, rf := range cfg.TransformAllowList() {
+		groupKind := apimachinery.ParseGroupKind(rf.Type)
+		transformableAllowlist[groupKind] = rf
+		delete(transformableDenylist, groupKind)
+	}
 	for _, rf := range cfg.TransformDenyList() {
 		groupKind := apimachinery.ParseGroupKind(rf.Type)
 		transformableDenylist[groupKind] = rf
+		delete(transformableAllowlist, groupKind)
 	}
 
+	// add user flag values, override user schema values and defaults
 	// TODO(aaron-prindle) verfiy if workdir needs to be considered more in this read...
 	if cfg.GetTransformAllowListFile() != "" {
 		transformableAllowlistFromFile, err := ioutil.ReadFile(cfg.GetTransformAllowListFile())
@@ -180,7 +193,8 @@ func NewDeployer(cfg kubectl.Config, labeller *label.DefaultLabeller, d *latestV
 		}
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
-			transformableDenylist[groupKind] = rf
+			transformableAllowlist[groupKind] = rf
+			delete(transformableDenylist, groupKind)
 		}
 	}
 
@@ -199,6 +213,7 @@ func NewDeployer(cfg kubectl.Config, labeller *label.DefaultLabeller, d *latestV
 		for _, rf := range rfs {
 			groupKind := apimachinery.ParseGroupKind(rf.Type)
 			transformableDenylist[groupKind] = rf
+			delete(transformableAllowlist, groupKind)
 		}
 	}
 	return &Deployer{

pkg/skaffold/kubernetes/manifest/images.go

@@ -128,7 +128,6 @@ func (r *imageReplacer) Visit(gk apimachinery.GroupKind, navpath string, o map[s
 	}
 
 	curFieldAllowlisted := false
-	// TODO(aaron-prindle) verify adding the allowlist logic here works as intended
 	if rf, ok := allowlist[gk]; ok {
 		if len(rf.Image) == 0 {
 			curFieldAllowlisted = true

pkg/skaffold/kubernetes/manifest/labels.go

@@ -54,6 +54,7 @@ func newLabelsSetter(labels map[string]string) *labelsSetter {
 }
 
 func (r *labelsSetter) Visit(gk apimachinery.GroupKind, navpath string, o map[string]interface{}, k string, v interface{}, allowlist map[apimachinery.GroupKind]latestV1.ResourceFilter, denylist map[apimachinery.GroupKind]latestV1.ResourceFilter) bool {
+	labelsField := "labels"
 	if rf, ok := denylist[gk]; ok {
 		if len(rf.Labels) == 0 {
 			return true
@@ -62,30 +63,31 @@ func (r *labelsSetter) Visit(gk apimachinery.GroupKind, navpath string, o map[st
 			// TODO(aaron-prindle) somewhat of a hack, truncate the last part of the labels path and see if this matches
 			// this is because the Visit label overwriting actually process/checks the 'metadata' field node, not the metadata.labels node
 			if navpath == path.Dir(denypath) {
-				// if navpath == path {
 				return true
 			}
 		}
 	}
 
-	// TODO(aaron-prindle) can we just check "labels" instead vs "metadata.labels"?
-	// TODO(aaron-prindle) see if allowlist logic should be moved up higher in the check order here?
 	curFieldInAllowlist := false
 	if rf, ok := allowlist[gk]; ok {
 		if len(rf.Labels) == 0 {
 			curFieldInAllowlist = true
 		}
-		// TODO(aaron-prindle) change this to better be in line with "labels" node - metadata.labels?
 		if k != "metadata" {
 			return true
 		}
 		for _, allowpath := range rf.Labels {
-			if navpath == allowpath {
+			// TODO(aaron-prindle) somewhat of a hack, truncate the last part of the labels path and see if this matches
+			// this is because the Visit label overwriting actually process/checks the 'metadata' field node, not the metadata.labels node
+			if navpath == path.Dir(allowpath) {
+				labelsField = path.Base(allowpath)
 				curFieldInAllowlist = true
 			}
 		}
 	}
 
+	// TODO(aaron-prindle) add logic that says if allowlist defines a path, use the last part of that path instead of "labels"
+
 	if !curFieldInAllowlist {
 		return true
 	}
@@ -99,9 +101,9 @@ func (r *labelsSetter) Visit(gk apimachinery.GroupKind, navpath string, o map[st
 		return true
 	}
 
-	l, present := metadata["labels"]
+	l, present := metadata[labelsField]
 	if !present {
-		metadata["labels"] = r.labels
+		metadata[labelsField] = r.labels
 		return false
 	}
 

pkg/skaffold/kubernetes/manifest/visitor.go

@@ -26,10 +26,15 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/yaml"
 )
 
-// transformableAllowlist is the set of kinds that can be transformed by Skaffold.
-var transformableAllowlist = map[apimachinery.GroupKind]latestV1.ResourceFilter{
+// TransformAllowlist is the default allowlist of kinds that can be transformed by Skaffold.
+var TransformAllowlist = map[apimachinery.GroupKind]latestV1.ResourceFilter{
+	// {Group: "", Kind: "Pod"}: {
+	// 	Type: "Pod",
+	// },
 	{Group: "", Kind: "Pod"}: {
 		Type: "Pod",
+		// Labels: []string{"/spec/volumeClaimTemplates/metadata/labels"},
+		Image: []string{"/spec/containers/name", "/spec/containers/image"},
 	},
 	// TODO(aaron-prindle) need to make sure the '.' notations works properly.. (there is maybe also a "/" gvk notation?)
 	{Group: "apps", Kind: "DaemonSet"}: {
@@ -73,13 +78,11 @@ var transformableAllowlist = map[apimachinery.GroupKind]latestV1.ResourceFilter{
 	},
 }
 
-// transformableDenylist is the set of kinds that can be transformed by Skaffold.
-var transformableDenylist = map[apimachinery.GroupKind]latestV1.ResourceFilter{
+// TransformDenylist is the default denylist on the set of kinds that can be transformed by Skaffold.
+var TransformDenylist = map[apimachinery.GroupKind]latestV1.ResourceFilter{
 	{Group: "apps", Kind: "StatefulSet"}: {
 		Type:   "StatefulSet.apps",
 		Labels: []string{"/spec/volumeClaimTemplates/metadata/labels"},
-		// TODO(aaron-prindle) this should HAVE to be the below filter vv
-		// Labels: []string{"/spec/volumeClaimTemplates/metadata/labels"},
 	},
 }
 
@@ -96,15 +99,15 @@ func (l *ManifestList) Visit(visitor FieldVisitor, allowlist map[apimachinery.Gr
 
 	// TODO(aaron-prindle) make sure this is the right place to do this union (defaults are correct/applied in right places...)
 	unionAllowList := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
-	for k, v := range transformableAllowlist {
+	for k, v := range TransformAllowlist {
 		unionAllowList[k] = v
 	}
 	for k, v := range allowlist {
 		unionAllowList[k] = v
 	}
 
 	unionDenyList := map[apimachinery.GroupKind]latestV1.ResourceFilter{}
-	for k, v := range transformableDenylist {
+	for k, v := range TransformDenylist {
 		unionDenyList[k] = v
 	}
 	for k, v := range denylist {
@@ -181,9 +184,9 @@ func shouldTransformManifest(manifest map[string]interface{}) bool {
 		Kind:  gvk.Kind,
 	}
 
-	if _, allowed := transformableAllowlist[groupKind]; allowed {
+	if _, allowed := TransformAllowlist[groupKind]; allowed {
 		// denylist has higher priority than allowlist
-		if rf, disallowed := transformableDenylist[groupKind]; disallowed {
+		if rf, disallowed := TransformDenylist[groupKind]; disallowed {
 			// TODO(aaron-prindle) verify that denylist should have higher priority than allowlist
 			if len(rf.Labels) == 0 && len(rf.Image) == 0 {
 				return false
@@ -223,13 +226,7 @@ func visitFields(gk apimachinery.GroupKind, navpath string, o interface{}, visit
 		}
 	case map[string]interface{}:
 		for k, v := range entries {
-			// visitor.Visit(gk, path.Join(navpath, k), entries, k, v, transformableAllowlist, transformableDenylist)
-			// TODO(aaron-prindle) change to actually used the parameters allowlist & denylist vs the hardcoded lists
 			visitor.Visit(gk, path.Join(navpath, k), entries, k, v, allowlist, denylist)
-
-			// TODO(aaron-prindle) remove the debug statement, hack to get linter passing
-			// log.Entry(context.TODO()).Debugf("in visitFields - case map[string]interface{} with allowlist: %v and denylist: %v", allowlist, denylist)
-			// log.Entry(context.TODO()).Debugf("in visitFields - case map[string]interface{} with allowlist: %v", allowlist)
 		}
 	}
 }

pkg/skaffold/runner/runcontext/context.go

@@ -244,9 +244,9 @@ func (rc *RunContext) RPCPort() *int                                 { return rc
 func (rc *RunContext) RPCHTTPPort() *int                             { return rc.Opts.RPCHTTPPort.Value() }
 func (rc *RunContext) PushImages() config.BoolOrUndefined            { return rc.Opts.PushImages }
 func (rc *RunContext) GetTransformAllowListFile() string {
-	return rc.Opts.TransformAllowListFile
+	return rc.Opts.TransformAllowFile
 }
-func (rc *RunContext) GetTransformDenyListFile() string { return rc.Opts.TransformDenyListFile }
+func (rc *RunContext) GetTransformDenyListFile() string { return rc.Opts.TransformDenyFile }
 
 func GetRunContext(ctx context.Context, opts config.SkaffoldOptions, configs []schemaUtil.VersionedConfig) (*RunContext, error) {
 	var pipelines []latestV1.Pipeline