Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Modified code for patches and patchJson6902 which will throw error if target is not found #4715

Closed
wants to merge 3 commits into from
Closed

Modified code for patches and patchJson6902 which will throw error if target is not found #4715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef32574
rebase
laxmikantbpandhare Dec 22, 2022
ef768e2
forgot to do generate-kustomize-builtin-plugins after removal of space
laxmikantbpandhare Aug 30, 2022
0c5e93e
updated error message according to new discussion
laxmikantbpandhare Jul 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/internal/builtins/PatchJson6902Transformer.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions api/internal/builtins/PatchTransformer.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

198 changes: 46 additions & 152 deletions api/krusty/extendedpatch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package krusty_test
import (
"testing"

"github.com/stretchr/testify/assert"
kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest"
)

Expand Down Expand Up @@ -821,82 +822,8 @@ metadata:
annotations:
new-key: new-value
`)
m := th.Run("base", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: nginx
name: nginx
spec:
template:
metadata:
labels:
app: nginx
spec:
containers:
- image: nginx
name: nginx
volumeMounts:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: busybox
name: busybox
spec:
template:
metadata:
labels:
app: busybox
spec:
containers:
- image: busybox
name: busybox
volumeMounts:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
---
apiVersion: v1
kind: Service
metadata:
labels:
app: nginx
name: nginx
spec:
ports:
- port: 80
selector:
app: nginx
---
apiVersion: v1
kind: Service
metadata:
labels:
app: busybox
name: busybox
spec:
ports:
- port: 8080
selector:
app: busybox
`)
err := th.RunWithErr("base", th.MakeDefaultOptions())
assert.Contains(t, err.Error(), "patches target not found for [noKind].[noVer].[noGrp]/no-match.[noNs]")
}

func TestExtendedPatchWithoutTarget(t *testing.T) {
Expand Down Expand Up @@ -1021,82 +948,8 @@ metadata:
annotations:
new-key: new-value
`)
m := th.Run("base", th.MakeDefaultOptions())
th.AssertActualEqualsExpected(m, `
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: nginx
name: nginx
spec:
template:
metadata:
labels:
app: nginx
spec:
containers:
- image: nginx
name: nginx
volumeMounts:
- mountPath: /tmp/ps
name: nginx-persistent-storage
volumes:
- emptyDir: {}
name: nginx-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
---
apiVersion: apps/v1
kind: Deployment
metadata:
labels:
app: busybox
name: busybox
spec:
template:
metadata:
labels:
app: busybox
spec:
containers:
- image: busybox
name: busybox
volumeMounts:
- mountPath: /tmp/ps
name: busybox-persistent-storage
volumes:
- emptyDir: {}
name: busybox-persistent-storage
- configMap:
name: configmap-in-base
name: configmap-in-base
---
apiVersion: v1
kind: Service
metadata:
labels:
app: nginx
name: nginx
spec:
ports:
- port: 80
selector:
app: nginx
---
apiVersion: v1
kind: Service
metadata:
labels:
app: busybox
name: busybox
spec:
ports:
- port: 8080
selector:
app: busybox
`)
err := th.RunWithErr("base", th.MakeDefaultOptions())
assert.Contains(t, err.Error(), "patches target not found for [noKind].[noVer].[noGrp]/no-match.[noNs]")
}

func TestExtendedPatchMultiplePatchOverlapping(t *testing.T) {
Expand Down Expand Up @@ -1213,3 +1066,44 @@ spec:
app: busybox
`)
}

func TestTargetMissingPatchJson6902Error(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to have tests for

  • allowNoTargetMatch
  • json patches under the patches field

th := kusttest_test.MakeHarness(t)
makeCommonFileForExtendedPatchTest(th)
th.WriteK("base", `
resources:
- servicemonitor.yaml
patchesJson6902:
- target:
group: monitoring.coreos.com
kind: ServiceMonitor
name: starboard-exporter
namespace: starboard
version: v2
path: patch.0.yaml
`)
th.WriteF("base/servicemonitor.yaml", `
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the existing files in makeCommonFileForExtendedPatchTest for this test?

apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
labels:
app: starboard-exporter
name: starboard-exporter
namespace: starboard
spec:
endpoints:
- path: /metrics
port: metrics
selector:
matchLabels:
app.kubernetes.io/instance: starboard-exporter
app.kubernetes.io/name: starboard-exporter
`)
th.WriteF("base/patch.0.yaml", `
- op: add
path: /metadata/labels/release
value: kube-prometheus-stack
`)
err := th.RunWithErr("base", th.MakeDefaultOptions())
assert.Contains(t, err.Error(), "patchesJson6902 target not found for ServiceMonitor.v2.monitoring.coreos.com/starboard-exporter.starboard")
}
5 changes: 5 additions & 0 deletions plugin/builtin/patchjson6902transformer/PatchJson6902Transformer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ func (p *plugin) Transform(m resmap.ResMap) error {
if err != nil {
return err
}

if len(resources) == 0 {
laxmikantbpandhare marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("patchesJson6902 target not found for %s", p.Target.ResId)
}

for _, res := range resources {
internalAnnotations := kioutil.GetInternalAnnotations(&res.RNode)

Expand Down
10 changes: 10 additions & 0 deletions plugin/builtin/patchtransformer/PatchTransformer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resour
if err != nil {
return err
}

if p.Options["allowNoTargetMatch"] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close. Instead, we want to emit this warning if allowNoTargetMatch is true AND there is no matching target (selected is empty).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totally agree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as below:

if p.Options["allowNoTargetMatch"] && len(selected) == 0 {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your comment, but this doesn't seem to be reflected in the code? Can you commit your changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that options is growing, can we define a struct under api/types to replace the map[string]bool? The field will be better documented that way and we'll be less likely to suffer from typo bugs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annasong20 - We can create a PatchTransformer struct under api/types. So, do you mean only the options field can be moved out of it and add it in api/types? Please let me know if I am wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I only need options to be a struct under api/types, but if you'd like you can move the PatchTransformer arguments into a PatchArgs. The latter is more work, but I think easier to read in the long run.

log.Println("Warning: patches target not found for Target")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we print to os.Stderr instead? You can use fmt.Fprintf.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as below:

	fmt.Fprintf(os.Stderr, "%v\n", "Warning: patches target not found for Target")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this change in the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we print for users the "Target" that we're looking for?

As a user with a complex Kustomization, I'd be confused and need to debug which patch target is missing.

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to return an error if allowNoTargetMatch is not set?


return m.ApplySmPatch(resource.MakeIdSet(selected), patch)
}

Expand All @@ -113,6 +118,11 @@ func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error
if err != nil {
return err
}

if p.Options["allowNoTargetMatch"] {
log.Println("Warning: patches target not found for Target")
}
Comment on lines +122 to +124
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated same as above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see the change in code.


for _, res := range resources {
res.StorePreviousId()
internalAnnotations := kioutil.GetInternalAnnotations(&res.RNode)
Expand Down