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.

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

Address naming rules with CronJobControllerV2 for k8s v1.21 #345

Merged
merged 43 commits into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
a67ac55
add regex for checking isUnixTime
Jan 27, 2022
74978c2
add test case for cron job
Jan 27, 2022
af5ef66
fix cron job in minutes and reduce time complex when checking for run…
Jan 27, 2022
2df3528
add final test on parse development and fix issue
Jan 27, 2022
7c911b2
add comments for test case
Jan 27, 2022
d71a495
reformat the structure
Jan 27, 2022
6696947
reformat the structure
Jan 27, 2022
9da9e4f
add back container insight common
Jan 27, 2022
59ad81d
fix regex for deployment
Jan 28, 2022
bb4eed9
fix string input for containUnexpectedRuneSet
Jan 28, 2022
79b7a02
fixed format for utils go
Jan 28, 2022
36ba09a
add error checking when parsing int
Jan 28, 2022
8eed962
change must compile to compile
Jan 28, 2022
1e48188
add test case and fix some test
Jan 28, 2022
56f4fd4
define var as a compiler and new type for regexp
Jan 28, 2022
df6456c
delete function
Jan 28, 2022
8a0556c
change back to use function for unexpected reg exp
Jan 28, 2022
5ae96fa
delete using function
khanhntd Jan 28, 2022
a29dbe2
change naming convention and some comments
khanhntd Jan 28, 2022
c0cca61
add test case for k8s processor
khanhntd Jan 28, 2022
3a3dfdf
change to use test table driven and add some test case to cover coverage
khanhntd Jan 28, 2022
f3891a5
delete coverage
khanhntd Jan 28, 2022
3445b91
reduce some test case
khanhntd Jan 28, 2022
2918c24
delete some assertion
khanhntd Jan 28, 2022
cf7f246
delete some assertion
khanhntd Jan 28, 2022
f6d5ce1
fix test
khanhntd Jan 28, 2022
d2285fd
fix test again
khanhntd Jan 28, 2022
62f7c14
fix some test case
khanhntd Jan 29, 2022
7368314
fix some test case
khanhntd Jan 29, 2022
dbbeabc
change condition when getting back
khanhntd Jan 29, 2022
e75dec1
add error condition
khanhntd Jan 29, 2022
a20b21a
add some comments
khanhntd Jan 29, 2022
86b9011
add comments for test case
khanhntd Jan 29, 2022
ee72ea7
fix some test case
khanhntd Jan 29, 2022
0f1401b
change condition for deployment from replicaset name
khanhntd Jan 29, 2022
466452d
add back mounts
khanhntd Jan 29, 2022
bce3233
change back to use condition
khanhntd Jan 29, 2022
3df097c
fix regex
khanhntd Jan 31, 2022
b74e23f
fix further regex
khanhntd Jan 31, 2022
f27baa1
fix regex issues
khanhntd Jan 31, 2022
4ebd373
change to use anchor
khanhntd Jan 31, 2022
e2a7300
define variables to use instead of defining everytime
khanhntd Jan 31, 2022
90d1a57
Add test for smaller than 3 characters
khanhntd Feb 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 31 additions & 30 deletions plugins/processors/k8sdecorator/stores/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package stores

import (
"regexp"
"strconv"
"strings"

. "github.com/aws/amazon-cloudwatch-agent/internal/containerinsightscommon"
Expand All @@ -30,11 +32,12 @@ func createContainerKeyFromMetric(tags map[string]string) string {
return k8sutil.CreateContainerKey(namespace, podName, containerName)
}

const (
// kubeAllowedStringAlphaNums holds the characters allowed in replicaset names from as parent deployment
var (
// deploymentUnallowedRegExp holds the characters allowed in replicaset names from as parent deployment
// https://github.com/kubernetes/apimachinery/blob/master/pkg/util/rand/rand.go#L83
kubeAllowedStringAlphaNums = "bcdfghjklmnpqrstvwxz2456789"
cronJobAllowedString = "0123456789"
deploymentUnallowedRegExp = regexp.MustCompile(`[^b-hj-np-tv-xz-z2-24-9]+`)
SaxyPandaBear marked this conversation as resolved.
Show resolved Hide resolved
khanhntd marked this conversation as resolved.
Show resolved Hide resolved
// cronJobUnallowedRegexp ensures the characters in cron job name are only numbers.
SaxyPandaBear marked this conversation as resolved.
Show resolved Hide resolved
cronJobUnallowedRegexp = regexp.MustCompile(`\D+`)
)

// get the deployment name by stripping the last dash following some rules
Expand All @@ -46,47 +49,45 @@ func parseDeploymentFromReplicaSet(name string) string {
return ""
}
suffix := name[lastDash+1:]
if len(suffix) < 3 {
if len(suffix) >= 3 && !deploymentUnallowedRegExp.MatchString(suffix) {
// Invalid suffix if it is less than 3
return ""
}

if !stringInRuneset(suffix, kubeAllowedStringAlphaNums) {
// Invalid suffix
return ""
return name[:lastDash]
}

return name[:lastDash]
return ""
}

// get the cronJob name by stripping the last dash following some rules
// return empty if it is not following the rule
// Get the cronJob name by stripping the last dash following by the naming convention: JobName-UnixTime
// based on https://github.com/kubernetes/kubernetes/blob/c4d752765b3bbac2237bf87cf0b1c2e307844666/pkg/controller/cronjob/cronjob_controllerv2.go#L594-L596.
// Before v1.21 CronJob in Kubernetes has used Unix Time in second; after v1.21 is a Unix Time in Minutes.

func parseCronJobFromJob(name string) string {
lastDash := strings.LastIndexAny(name, "-")

//Return empty since the naming convention is: JobName-UnixTime, if it does not have the "-", meanings the job name is empty
if lastDash == -1 {
// No dash
return ""
}

suffix := name[lastDash+1:]
if len(suffix) != 10 {
// Invalid suffix if it is not 10 rune
return ""
}
suffixInt, err := strconv.ParseInt(suffix, 10, 64)

if !stringInRuneset(suffix, cronJobAllowedString) {
// Invalid suffix
if err != nil {
return ""
}

return name[:lastDash]
}
//Convert Unix Time In Minutes to Unix Time
suffixStringMultiply := strconv.FormatInt(suffixInt*60, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, we only need this if the first if-statement evaluates to false.

//Checking if the suffix is a unix time by checking: the length and contains character
//Checking for the length: CronJobControllerV2 is Unix Time in Minutes (7-9 characters) while CronJob is Unix Time (10 characters).
//However, multiply by 60 to convert the Unix Time In Minutes back to Unix Time in order to have the same condition as Unix Time
if len(suffix) == 10 && !cronJobUnallowedRegexp.MatchString(suffix) { //Condition for CronJob before k8s v1.21
return name[:lastDash]
}

func stringInRuneset(name, subset string) bool {
for _, r := range name {
if !strings.ContainsRune(subset, r) {
// Found an unexpected rune in suffix
return false
}
if len(suffixStringMultiply) == 10 && !cronJobUnallowedRegexp.MatchString(suffixStringMultiply) { //Condition for CronJobControllerV2 after k8s v1.21
return name[:lastDash]
}
return true

return ""
}
88 changes: 83 additions & 5 deletions plugins/processors/k8sdecorator/stores/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,94 @@ package stores

import (
"github.com/docker/docker/pkg/testutil/assert"
"strconv"
"testing"
"time"
)

func TestUtils_parseDeploymentFromReplicaSet(t *testing.T) {
assert.Equal(t, "", parseDeploymentFromReplicaSet("cloudwatch-agent"))
assert.Equal(t, "cloudwatch-agent", parseDeploymentFromReplicaSet("cloudwatch-agent-42kcz"))
testcases := []struct {
name string
inputString string
expected string
}{
{
name: "Get ReplicaSet Name with unallowed characters",
inputString: "cloudwatch-ag",
expected: "",
},
{
name: "Get ReplicaSet Name with allowed characters",
inputString: "cloudwatch-agent-42kcz",
expected: "cloudwatch-agent",
},
{
name: "Get ReplicaSet Name with string smaller than 3 characters",
inputString: "cloudwatch-agent-sd",
expected: "",
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, parseDeploymentFromReplicaSet(tc.inputString), tc.expected, )
})
}
}

func TestUtils_parseCronJobFromJob(t *testing.T) {
assert.Equal(t, "", parseCronJobFromJob("hello-123"))
assert.Equal(t, "hello", parseCronJobFromJob("hello-1234567890"))
assert.Equal(t, "", parseCronJobFromJob("hello-123456789a"))
testcases := []struct {
name string
inputString string
expected string
}{
{
name: "Get CronJobControllerV2 or CronJob's Name with alphabet characters",
inputString: "hello-name",
expected: "",
},
{
name: "Get CronJobControllerV2 or CronJob's Name with special characters and exact 10 characters",
inputString: "hello-1678995&64",
expected: "",
},
{
name: "Get CronJobControllerV2 or CronJob's Name with Unix Time not equal to 10 letters",
inputString: "hello-238",
expected: "",
},
{
name: "Get CronJobControllerV2's Name after k8s v1.21 with correct Unix Time",
inputString: "hello-"+strconv.FormatInt(time.Now().Unix()/60, 10),
SaxyPandaBear marked this conversation as resolved.
Show resolved Hide resolved
expected: "hello",
},
{
name: "Get CronJobControllerV2's Name after k8s v1.21 with alphabet Unix Time",
inputString: "hello-"+strconv.FormatInt(time.Now().Unix()/60, 10)+"a28bc",
expected: "",
},

{
name: "Get CronJobControllerV2's Name after k8s v1.21 with Unix Time not equal to 10 letters",
inputString: "hello"+strconv.FormatInt(time.Now().Unix()/60, 10)+"523",
expected: "",
},
{
name: "Get CronJob's Name before k8s v1.21 with correct Unix Time",
inputString: "hello-"+strconv.FormatInt(time.Now().Unix(), 10),
expected: "hello",
},
{
name: "Get CronJob's Name before k8s v1.21 with special characters",
inputString: "hello-"+strconv.FormatInt(time.Now().Unix(), 10)+"&#64",
expected: "",
},

}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t,parseCronJobFromJob(tc.inputString), tc.expected)
})
}
}