Skip to content

Commit

Permalink
Include review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
Ke Zhu committed Jul 28, 2021
1 parent c16db69 commit 6a048ee
Showing 1 changed file with 29 additions and 9 deletions.
38 changes: 29 additions & 9 deletions docs/design_proposals/configurable-transformable-allowlist.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,27 +37,47 @@ Option in `skaffold.yaml`
deploy:
config:
transformableAllowList:
- Group: example.com
Kind: Application
- Group: argoproj.io
Kind: Workflow
- Group: tekton.dev
Kind: Task
- type: pod # no group, implicitly all versions
- type: batch/Job # group, implicitly all versions
- type: openfaas.com/v1/Function
image: [spec.image]
labels: [spec.metadata.labels, spec.labels] # https://www.openfaas.com/blog/manage-functions-with-kubectl/
- type: apps/v1beta1/Deployment
image: [spec.template.spec.initContainers.*.image, spec.template.spec.containers.*.image]
labels: [spec.metadata.labels, spec.template.metadata.labels]
```
The value of `type` field points to a resource type. So it's case sensitive
and should support API groups and resource versions:

* When not specifying group, it will transform given resource type of any group or versions.
* When providing group but not resource version, it will transform given
resource type of any versions.

The value of `labels` field is a list of JSON-path-like paths to apply `labels`
block to. If no `labels` field configured, it will simply apply `labels` block
if missing.

The value of `image` field is also a list of JSON-path-like paths to rewrite. If
no `image` field configured, it will rewrite any field named `image`.

## Open issues/Questions

Since it is an allowlist, neither options could disable transformation on any
built-in resource like `ReplicaSet` or `Deployment`.
built-in resource like `ReplicaSet` or `Deployment`. However, it may need to
refactor [current allowlist](https://github.com/GoogleContainerTools/skaffold/blob/27c38228ab929ddaf2636637b43f17fda1686652/pkg/skaffold/kubernetes/manifest/visitor.go#L28-L43).

Is there any need to work out a deny list?

## Implementation plan

1. `pkg/skaffold/schema/latest/v1/config.go` - Add config option
`transformableAllowList` to `DeployConfig`.
2. `pkg/skaffold/kubernetes/manifest/visitor.go` - Add new parameter `transformableAllowList`
to `*ManifestList.Visit()` by appending it to existing coded `transformableAllowList`
2. `pkg/skaffold/kubernetes/manifest/visitor.go` - Refactor allowlist and add
new parameter `transformableAllowList` to `*ManifestList.Visit()` by appending
it to existing coded `transformableAllowList`
- Support `labels` field
- Support `image` field
3. `pkg/skaffold/kubernetes/manifest/images.go` - Add new parameter to `*ManifestList.ReplaceImages()`
to support given `transformableAllowList`
4. Instrument each deployer to use the new parameter `transformableAllowList`
Expand Down

0 comments on commit 6a048ee

Please sign in to comment.