Skip to content

Commit

Permalink
Merge pull request #128 from nirmata/ebay-fix
Browse files Browse the repository at this point in the history
NDEV-19576 : mutation fix
  • Loading branch information
anushkamittal2001 authored Jul 3, 2024
2 parents 221658c + 416684e commit 79744f1
Show file tree
Hide file tree
Showing 7 changed files with 22 additions and 279 deletions.
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ require (
github.com/kataras/tablewriter v0.0.0-20180708051242-e063d29b7c23
github.com/kyverno/go-jmespath v0.4.1-0.20231124160150-95e59c162877
github.com/lensesio/tableprinter v0.0.0-20201125135848-89e81fc956e7
github.com/mattbaird/jsonpatch v0.0.0-20200820163806-098863c1fc24
github.com/notaryproject/notation-core-go v1.0.0-rc.4
github.com/notaryproject/notation-go v1.0.0-rc.6
github.com/onsi/ginkgo v1.16.5
Expand Down Expand Up @@ -64,6 +63,7 @@ require (
go.uber.org/zap v1.27.0
golang.org/x/crypto v0.24.0
golang.org/x/exp v0.0.0-20231108232855-2478ac86f678
gomodules.xyz/jsonpatch/v2 v2.3.0
google.golang.org/grpc v1.62.1
gopkg.in/inf.v0 v0.9.1
gopkg.in/yaml.v2 v2.4.0
Expand Down
2 changes: 0 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -2945,8 +2945,6 @@ github.com/matoous/godox v0.0.0-20210227103229-6504466cf951/go.mod h1:1BELzlh859
github.com/matryer/is v1.2.0/go.mod h1:2fLPjFQM9rhQ15aVEtbuwhJinnOqrmgXPNdZsdwlWXA=
github.com/matryer/is v1.4.0 h1:sosSmIWwkYITGrxZ25ULNDeKiMNzFSr4V/eqBQP0PeE=
github.com/matryer/is v1.4.0/go.mod h1:8I/i5uYgLzgsgEloJE1U6xx5HkBQpAZvepWuujKwMRU=
github.com/mattbaird/jsonpatch v0.0.0-20200820163806-098863c1fc24 h1:uYuGXJBAi1umT+ZS4oQJUgKtfXCAYTR+n9zw1ViT0vA=
github.com/mattbaird/jsonpatch v0.0.0-20200820163806-098863c1fc24/go.mod h1:M1qoD/MqPgTZIk0EWKB38wE28ACRfVcn+cU08jyArI0=
github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
github.com/mattn/go-colorable v0.1.1/go.mod h1:FuOcm+DKB9mbwrcAfNl7/TZVBZ6rcnceauSikq3lYCQ=
github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/handlers/mutation/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"github.com/kyverno/kyverno/pkg/engine/mutate/patch"
engineutils "github.com/kyverno/kyverno/pkg/engine/utils"
"github.com/kyverno/kyverno/pkg/utils/api"
"github.com/mattbaird/jsonpatch"
"gomodules.xyz/jsonpatch/v2"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/mutate/mutation.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"github.com/kyverno/kyverno/pkg/engine/mutate/patch"
"github.com/kyverno/kyverno/pkg/engine/variables"
datautils "github.com/kyverno/kyverno/pkg/utils/data"
"github.com/mattbaird/jsonpatch"
"gomodules.xyz/jsonpatch/v2"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/mutate/patch/patches.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package patch

import (
"github.com/go-logr/logr"
"github.com/mattbaird/jsonpatch"
"gomodules.xyz/jsonpatch/v2"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
)

Expand Down
87 changes: 2 additions & 85 deletions pkg/engine/mutate/patch/patchesUtils.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
package patch

import (
"path/filepath"
"regexp"
"strings"

"github.com/kyverno/kyverno/pkg/utils/wildcard"
"github.com/mattbaird/jsonpatch"
"gomodules.xyz/jsonpatch/v2"
)

func ConvertPatches(in ...jsonpatch.JsonPatchOperation) [][]byte {
Expand All @@ -23,91 +21,10 @@ func generatePatches(src, dst []byte) ([]jsonpatch.JsonPatchOperation, error) {
if pp, err := jsonpatch.CreatePatch(src, dst); err != nil {
return nil, err
} else {
return filterAndSortPatches(pp), err
return filterInvalidPatches(pp), err
}
}

// filterAndSortPatches
// 1. filters out patches with the certain paths
// 2. sorts the removal patches(with same path) by the key of index
// in descending order. The sort is required as when removing multiple
// elements from an array, the elements must be removed in descending
// order to preserve each index value.
func filterAndSortPatches(originalPatches []jsonpatch.JsonPatchOperation) []jsonpatch.JsonPatchOperation {
patches := filterInvalidPatches(originalPatches)

result := make([]jsonpatch.JsonPatchOperation, len(patches))
index := getIndexToBeReversed(patches)

if len(index) == 0 {
return patches
}

start := 0
for _, idx := range index {
end := idx[0]
copy(result[start:end], patches[start:end])
reversedPatches := reverse(patches, idx)
copy(result[end:], reversedPatches)
start = idx[1] + 1
}
copy(result[start:], patches[start:])
return result
}

func getIndexToBeReversed(patches []jsonpatch.JsonPatchOperation) [][]int {
removePaths := make([]string, len(patches))

for i, patch := range patches {
if patch.Operation == "remove" {
removePaths[i] = patch.Path
}
}
return getRemoveInterval(removePaths)
}

func getRemoveInterval(removePaths []string) [][]int {
// get paths end with '/number'
regex := regexp.MustCompile(`\/\d+$`)
for i, path := range removePaths {
if !regex.Match([]byte(path)) {
removePaths[i] = ""
}
}

res := [][]int{}
for i := 0; i < len(removePaths); {
if removePaths[i] != "" {
baseDir := filepath.Dir(removePaths[i])
j := i + 1
for ; j < len(removePaths); j++ {
curDir := filepath.Dir(removePaths[j])
if baseDir != curDir {
break
}
}
if i != j-1 {
res = append(res, []int{i, j - 1})
}
i = j
} else {
i++
}
}

return res
}

func reverse(patches []jsonpatch.JsonPatchOperation, interval []int) []jsonpatch.JsonPatchOperation {
res := make([]jsonpatch.JsonPatchOperation, interval[1]-interval[0]+1)
j := 0
for i := interval[1]; i >= interval[0]; i-- {
res[j] = patches[i]
j++
}
return res
}

// filterInvalidPatches filters out patch with the following path:
// - not */metadata/name, */metadata/namespace, */metadata/labels, */metadata/annotations
// - /status
Expand Down
204 changes: 16 additions & 188 deletions pkg/engine/mutate/patch/patchesUtils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,23 @@ import (
"testing"

"github.com/go-logr/logr"
"github.com/mattbaird/jsonpatch"
assertnew "github.com/stretchr/testify/assert"
"gotest.tools/assert"
)

func Test_GeneratePatches(t *testing.T) {
out, err := strategicMergePatch(logr.Discard(), string(baseBytes), string(overlayBytes))
assert.NilError(t, err)

expectedPatches := map[string]bool{
`{"op":"remove","path":"/spec/template/spec/containers/0"}`: true,
`{"op":"add","path":"/spec/template/spec/containers/0","value":{"image":"nginx","name":"nginx"}}`: true,
`{"op":"add","path":"/spec/template/spec/containers/1","value":{"env":[{"name":"WORDPRESS_DB_HOST","value":"$(MYSQL_SERVICE)"},{"name":"WORDPRESS_DB_PASSWORD","valueFrom":{"secretKeyRef":{"key":"password","name":"mysql-pass"}}}],"image":"wordpress:4.8-apache","name":"wordpress","ports":[{"containerPort":80,"name":"wordpress"}],"volumeMounts":[{"mountPath":"/var/www/html","name":"wordpress-persistent-storage"}]}}`: true,
`{"op":"add","path":"/spec/template/spec/initContainers","value":[{"command":["echo $(WORDPRESS_SERVICE)","echo $(MYSQL_SERVICE)"],"image":"debian","name":"init-command"}]}`: true,
`{"op":"add","path":"/spec/template/spec/containers/1","value":{"env":[{"name":"WORDPRESS_DB_HOST","value":"$(MYSQL_SERVICE)"},{"name":"WORDPRESS_DB_PASSWORD","valueFrom":{"secretKeyRef":{"key":"password","name":"mysql-pass"}}}],"image":"wordpress:4.8-apache","name":"wordpress","ports":[{"containerPort":80,"name":"wordpress"}],"volumeMounts":[{"mountPath":"/var/www/html","name":"wordpress-persistent-storage"}]}}`: true,
`{"op":"replace","path":"/spec/template/spec/containers/0/image","value":"nginx"}`: true,
`{"op":"replace","path":"/spec/template/spec/containers/0/name","value":"nginx"}`: true,
`{"op":"remove","path":"/spec/template/spec/containers/0/ports"}`: true,
`{"op":"remove","path":"/spec/template/spec/containers/0/volumeMounts"}`: true,
}
patches, err := generatePatches(baseBytes, out)
assert.NilError(t, err)

for _, p := range patches {
assertnew.Equal(t, expectedPatches[p.Json()], true)
}
Expand Down Expand Up @@ -219,189 +218,18 @@ func Test_ignorePath(t *testing.T) {
func Test_GeneratePatches_sortRemovalPatches(t *testing.T) {
base := []byte(`{"apiVersion": "apps/v1","kind": "Deployment","metadata": {"name": "nginx-deployment","labels": {"app": "nginx"}},"spec": {"selector": {"matchLabels": {"app": "nginx"}},"replicas": 1,"template": {"metadata": {"labels": {"app": "nginx"}},"spec": {"containers": [{"name": "nginx","image": "nginx:1.14.2","ports": [{"containerPort": 80}]}],"tolerations": [{"effect": "NoExecute","key": "node.kubernetes.io/not-ready","operator": "Exists","tolerationSeconds": 300},{"effect": "NoExecute","key": "node.kubernetes.io/unreachable","operator": "Exists","tolerationSeconds": 300}]}}}}`)
patchedResource := []byte(`{"apiVersion": "apps/v1","kind": "Deployment","metadata": {"name": "nginx-deployment","labels": {"app": "nginx"}},"spec": {"selector": {"matchLabels": {"app": "nginx"}},"replicas": 1,"template": {"metadata": {"labels": {"app": "nginx"}},"spec": {"containers": [{"name": "nginx","image": "nginx:1.14.2","ports": [{"containerPort": 80}]}],"tolerations": [{"effect": "NoSchedule","key": "networkzone","operator": "Equal","value": "dmz"}]}}}}`)
expectedPatches := [][]byte{[]byte(`{"op":"remove","path":"/spec/template/spec/tolerations/1"}`), []byte(`{"op":"remove","path":"/spec/template/spec/tolerations/0"}`), []byte(`{"op":"add","path":"/spec/template/spec/tolerations/0","value":{"effect":"NoSchedule","key":"networkzone","operator":"Equal","value":"dmz"}}`)}
expectedPatches := [][]byte{
[]byte(`{"op":"remove","path":"/spec/template/spec/tolerations/1"}`),
[]byte(`{"op":"replace","path":"/spec/template/spec/tolerations/0/effect","value":"NoSchedule"}`),
[]byte(`{"op":"replace","path":"/spec/template/spec/tolerations/0/key","value":"networkzone"}`),
[]byte(`{"op":"replace","path":"/spec/template/spec/tolerations/0/operator","value":"Equal"}`),
[]byte(`{"op":"add","path":"/spec/template/spec/tolerations/0/value","value":"dmz"}`),
[]byte(`{"op":"remove","path":"/spec/template/spec/tolerations/0/tolerationSeconds"}`),
}
patches, err := generatePatches(base, patchedResource)
fmt.Println(patches)
assertnew.Nil(t, err)
assertnew.Equal(t, expectedPatches, ConvertPatches(patches...))
}

func Test_sortRemovalPatches(t *testing.T) {
tests := []struct {
patches []jsonpatch.JsonPatchOperation
expected []jsonpatch.JsonPatchOperation
}{
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "add", Path: "/a"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "add", Path: "/a"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "add", Path: "/a"}, {Operation: "remove", Path: "/a"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "add", Path: "/a"}, {Operation: "remove", Path: "/a"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "add", Path: "/a/0"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "add", Path: "/a/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/a/1"}, {Operation: "remove", Path: "/a/2"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/2"}, {Operation: "remove", Path: "/a/1"}, {Operation: "remove", Path: "/a/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/0"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/0"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/c/0"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/b/0"}, {Operation: "remove", Path: "/c/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "add", Path: "/z"}, {Operation: "remove", Path: "/b/0"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/c/0"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "add", Path: "/z"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/b/0"}, {Operation: "remove", Path: "/c/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/0"}, {Operation: "add", Path: "/b/c/0"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/c/0"}},
expected: []jsonpatch.JsonPatchOperation{{Operation: "remove", Path: "/a/0"}, {Operation: "remove", Path: "/b/0"}, {Operation: "add", Path: "/b/c/0"}, {Operation: "remove", Path: "/b/1"}, {Operation: "remove", Path: "/c/0"}},
},
{
patches: []jsonpatch.JsonPatchOperation{
{Operation: "remove", Path: "/spec/containers/0/args/7"},
{Operation: "remove", Path: "/spec/containers/0/args/6"},
{Operation: "remove", Path: "/spec/containers/0/args/5"},
{Operation: "remove", Path: "/spec/containers/0/args/4"},
{Operation: "remove", Path: "/spec/containers/0/args/3"},
{Operation: "remove", Path: "/spec/containers/0/args/2"},
{Operation: "remove", Path: "/spec/containers/0/args/1"},
{Operation: "remove", Path: "/spec/containers/0/args/0"},
{Operation: "add", Path: "/spec/containers/0/args/0", Value: "--logtostderr"},
{Operation: "add", Path: "/spec/containers/0/args/1", Value: "--secure-listen-address=[$(IP)]:9100"},
{Operation: "add", Path: "/spec/containers/0/args/2", Value: "--tls-cipher-suites=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"},
{Operation: "add", Path: "/spec/containers/0/args/3", Value: "--upstream=http://127.0.0.1:9100/"},
{Operation: "remove", Path: "/spec/containers/1/args/3"},
{Operation: "remove", Path: "/spec/containers/1/args/2"},
{Operation: "remove", Path: "/spec/containers/1/args/1"},
{Operation: "remove", Path: "/spec/containers/1/args/0"},
{Operation: "add", Path: "/spec/containers/1/args/0", Value: "--web.listen-address=127.0.0.1:9100"},
{Operation: "add", Path: "/spec/containers/1/args/1", Value: "--Path.procfs=/host/proc"},
{Operation: "add", Path: "/spec/containers/1/args/2", Value: "--Path.sysfs=/host/sys"},
{Operation: "add", Path: "/spec/containers/1/args/3", Value: "--Path.rootfs=/host/root"},
{Operation: "add", Path: "/spec/containers/1/args/4", Value: "--no-collector.wifi"},
{Operation: "add", Path: "/spec/containers/1/args/5", Value: "--no-collector.hwmon"},
{Operation: "add", Path: "/spec/containers/1/args/6", Value: "--collector.filesystem.ignored-mount-points=^/(dev|proc|sys|var/lib/docker/.+)($|/)"},
{Operation: "add", Path: "/spec/containers/1/args/7", Value: "--collector.filesystem.ignored-fs-types=^(autofs|binfmt_misc|cgroup|tracefs)$"},
},
expected: []jsonpatch.JsonPatchOperation{
{Operation: "remove", Path: "/spec/containers/0/args/0"},
{Operation: "remove", Path: "/spec/containers/0/args/1"},
{Operation: "remove", Path: "/spec/containers/0/args/2"},
{Operation: "remove", Path: "/spec/containers/0/args/3"},
{Operation: "remove", Path: "/spec/containers/0/args/4"},
{Operation: "remove", Path: "/spec/containers/0/args/5"},
{Operation: "remove", Path: "/spec/containers/0/args/6"},
{Operation: "remove", Path: "/spec/containers/0/args/7"},
{Operation: "add", Path: "/spec/containers/0/args/0", Value: "--logtostderr"},
{Operation: "add", Path: "/spec/containers/0/args/1", Value: "--secure-listen-address=[$(IP)]:9100"},
{Operation: "add", Path: "/spec/containers/0/args/2", Value: "--tls-cipher-suites=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256"},
{Operation: "add", Path: "/spec/containers/0/args/3", Value: "--upstream=http://127.0.0.1:9100/"},
{Operation: "remove", Path: "/spec/containers/1/args/0"},
{Operation: "remove", Path: "/spec/containers/1/args/1"},
{Operation: "remove", Path: "/spec/containers/1/args/2"},
{Operation: "remove", Path: "/spec/containers/1/args/3"},
{Operation: "add", Path: "/spec/containers/1/args/0", Value: "--web.listen-address=127.0.0.1:9100"},
{Operation: "add", Path: "/spec/containers/1/args/1", Value: "--Path.procfs=/host/proc"},
{Operation: "add", Path: "/spec/containers/1/args/2", Value: "--Path.sysfs=/host/sys"},
{Operation: "add", Path: "/spec/containers/1/args/3", Value: "--Path.rootfs=/host/root"},
{Operation: "add", Path: "/spec/containers/1/args/4", Value: "--no-collector.wifi"},
{Operation: "add", Path: "/spec/containers/1/args/5", Value: "--no-collector.hwmon"},
{Operation: "add", Path: "/spec/containers/1/args/6", Value: "--collector.filesystem.ignored-mount-points=^/(dev|proc|sys|var/lib/docker/.+)($|/)"},
{Operation: "add", Path: "/spec/containers/1/args/7", Value: "--collector.filesystem.ignored-fs-types=^(autofs|binfmt_misc|cgroup|tracefs)$"},
},
},
}

for i, test := range tests {
sortedPatches := filterAndSortPatches(test.patches)
assertnew.Equal(t, test.expected, sortedPatches, fmt.Sprintf("%dth test fails", i))
}
}

func Test_getRemoveInterval(t *testing.T) {
tests := []struct {
removalPaths []string
expectedIndex [][]int
}{
{
removalPaths: []string{"/a/0", "/b/0", "/b/1", "/c/0"},
expectedIndex: [][]int{{1, 2}},
},
{
removalPaths: []string{},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/0"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/0", "/a/1"},
expectedIndex: [][]int{{0, 1}},
},
{
removalPaths: []string{"/a/0", "/a"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a", "/a/0"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a", "/a"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/0", "/b/0"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/0", "/a/1", "/a/2"},
expectedIndex: [][]int{{0, 2}},
},
{
removalPaths: []string{"/a/0", "/a/1", "/a/b"},
expectedIndex: [][]int{{0, 1}},
},
{
removalPaths: []string{"/a", "/a/0", "/a/1"},
expectedIndex: [][]int{{1, 2}},
},
{
removalPaths: []string{"/", "/a", "/b/0"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/b", "/a/c", "/b/0", "/b/1", "/c"},
expectedIndex: [][]int{{2, 3}},
},
{
removalPaths: []string{"/a/0", "/b/c", "/b/d", "/b/e", "/c/0"},
expectedIndex: [][]int{},
},
{
removalPaths: []string{"/a/0", "/a/1", "/b/z", "/c/0", "/c/1", "/c/2", "/d/z", "/e/0"},
expectedIndex: [][]int{{0, 1}, {3, 5}},
},
{
removalPaths: []string{"/a/0", "/a/1", "/a/2", "/a/3"},
expectedIndex: [][]int{{0, 3}},
},
}

for i, test := range tests {
res := getRemoveInterval(test.removalPaths)
assertnew.Equal(t, test.expectedIndex, res, fmt.Sprintf("%d-th test fails at path %v", i, test.removalPaths))
for _, patch := range patches {
fmt.Println(patch.Json())
}
assertnew.Equal(t, expectedPatches, ConvertPatches(patches...))
}

0 comments on commit 79744f1

Please sign in to comment.