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

Try to resolve container.name from the injected agent args #1319

Merged
merged 11 commits into from
Dec 1, 2020
Merged

Try to resolve container.name from the injected agent args #1319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
65a8d62
add testcase to confirm the issue
lujiajing1126 Nov 27, 2020
fd27c42
find container.name in the old container args to resolve the issue
lujiajing1126 Nov 27, 2020
4324e78
refactor tag generation
lujiajing1126 Nov 28, 2020
4aab7d1
revert back go.sum and go.mod
lujiajing1126 Nov 30, 2020
6fbe2ec
use assert.Equal instead of reflect.DeepEqual
lujiajing1126 Nov 30, 2020
436bcc5
add a test to cover an unreached branch
lujiajing1126 Nov 30, 2020
aef839e
refactor to use getContainerName to extract container name if possible
lujiajing1126 Nov 30, 2020
980af69
add tests for getContainerName
lujiajing1126 Dec 1, 2020
fa80037
remove redundent Contains assertion
lujiajing1126 Dec 1, 2020
de6d80d
remove redundent Contains assertion in other two places
lujiajing1126 Dec 1, 2020
5a14178
do not care about agent container in the controller level
lujiajing1126 Dec 1, 2020
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
38 changes: 12 additions & 26 deletions pkg/controller/deployment/deployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,33 +157,19 @@ func (r *ReconcileDeployment) Reconcile(request reconcile.Request) (reconcile.Re
}
}

depHasAgent, depAgentIndex := inject.HasJaegerAgent(dep)
patch := client.MergeFrom(dep.DeepCopy())
if !depHasAgent {
// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
// Verified that jaeger instance was found and is not marked for deletion.
log.WithFields(log.Fields{
"deployment": dep.Name,
"namespace": dep.Namespace,
"jaeger": jaeger.Name,
"jaeger-namespace": jaeger.Namespace,
}).Info("Injecting Jaeger Agent sidecar")
injectedDep := inject.Sidecar(jaeger, dep)
if err := r.client.Patch(ctx, injectedDep, patch); err != nil {
log.WithField("deployment", injectedDep).WithError(err).Error("failed to update")
return reconcile.Result{}, tracing.HandleError(err, span)
}
} else {
// This could be an update, re-inject the sidecar in case the parameters changed.
// if any change is detected the cluster will create a new version of the deployment
// otherwise it will do nothing.
containers := dep.Spec.Template.Spec.Containers
dep.Spec.Template.Spec.Containers = append(containers[:depAgentIndex], containers[depAgentIndex+1:]...)
injectedDep := inject.Sidecar(jaeger, dep)
if err := r.client.Patch(ctx, injectedDep, patch); err != nil {
log.WithField("deployment", injectedDep).WithError(err).Error("failed to update")
return reconcile.Result{}, tracing.HandleError(err, span)
}
// a suitable jaeger instance was found! let's inject a sidecar pointing to it then
// Verified that jaeger instance was found and is not marked for deletion.
log.WithFields(log.Fields{
"deployment": dep.Name,
"namespace": dep.Namespace,
"jaeger": jaeger.Name,
"jaeger-namespace": jaeger.Namespace,
}).Info("Injecting Jaeger Agent sidecar")
injectedDep := inject.Sidecar(jaeger, dep)
if err := r.client.Patch(ctx, injectedDep, patch); err != nil {
log.WithField("deployment", injectedDep).WithError(err).Error("failed to update")
return reconcile.Result{}, tracing.HandleError(err, span)
}
} else {
log.WithField("deployment", dep.Name).Info("No suitable Jaeger instances found to inject a sidecar")
Expand Down
80 changes: 63 additions & 17 deletions pkg/inject/sidecar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ func Sidecar(jaeger *v1.Jaeger, dep *appsv1.Deployment) *appsv1.Deployment {
hasAgent, agentContainerIndex := HasJaegerAgent(dep)
logFields.Debug("injecting sidecar")
if hasAgent { // This is an update
dep.Spec.Template.Spec.Containers[agentContainerIndex] = container(jaeger, dep)
dep.Spec.Template.Spec.Containers[agentContainerIndex] = container(jaeger, dep, agentContainerIndex)
} else {
dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger, dep))

dep.Spec.Template.Spec.Containers = append(dep.Spec.Template.Spec.Containers, container(jaeger, dep, -1))
}

jaegerName := util.Truncate(jaeger.Name, 63)
Expand Down Expand Up @@ -164,7 +163,7 @@ func getJaeger(name string, jaegers *v1.JaegerList) *v1.Jaeger {
return nil
}

func container(jaeger *v1.Jaeger, dep *appsv1.Deployment) corev1.Container {
func container(jaeger *v1.Jaeger, dep *appsv1.Deployment, agentIdx int) corev1.Container {
args := append(jaeger.Spec.Agent.Options.ToArgs())

// we only add the grpc host if we are adding the reporter type and there's no explicit value yet
Expand All @@ -187,21 +186,31 @@ func container(jaeger *v1.Jaeger, dep *appsv1.Deployment) corev1.Container {
adminPort := util.GetAdminPort(args, 14271)

if len(util.FindItem("--jaeger.tags=", args)) == 0 {
agentTags := fmt.Sprintf("%s=%s,%s=%s,%s=%s,%s=%s,%s=%s",
"cluster", "undefined", // this value isn't currently available
"deployment.name", dep.Name,
"pod.namespace", dep.Namespace,
"pod.name", fmt.Sprintf("${%s:}", envVarPodName),
"host.ip", fmt.Sprintf("${%s:}", envVarHostIP),
)

if len(dep.Spec.Template.Spec.Containers) == 1 {
agentTags = fmt.Sprintf("%s,%s=%s", agentTags,
"container.name", dep.Spec.Template.Spec.Containers[0].Name,
)
defaultAgentTagsMap := make(map[string]string)
defaultAgentTagsMap["cluster"] = "undefined" // this value isn't currently available
defaultAgentTagsMap["deployment.name"] = dep.Name
defaultAgentTagsMap["pod.namespace"] = dep.Namespace
defaultAgentTagsMap["pod.name"] = fmt.Sprintf("${%s:}", envVarPodName)
defaultAgentTagsMap["host.ip"] = fmt.Sprintf("${%s:}", envVarHostIP)

defaultContainerName := getContainerName(dep.Spec.Template.Spec.Containers, agentIdx)

// if we can deduce the container name from the PodSpec
if defaultContainerName != "" {
defaultAgentTagsMap["container.name"] = defaultContainerName
}

if agentIdx > -1 {
existingAgentTags := parseAgentTags(dep.Spec.Template.Spec.Containers[agentIdx].Args)
// merge two maps
for key, value := range defaultAgentTagsMap {
existingAgentTags[key] = value
}
args = append(args, fmt.Sprintf(`--jaeger.tags=%s`, joinTags(existingAgentTags)))
} else {
args = append(args, fmt.Sprintf(`--jaeger.tags=%s`, joinTags(defaultAgentTagsMap)))
}

args = append(args, fmt.Sprintf(`--jaeger.tags=%s`, agentTags))
}

commonSpec := util.Merge([]v1.JaegerCommonSpec{jaeger.Spec.Agent.JaegerCommonSpec, jaeger.Spec.JaegerCommonSpec})
Expand Down Expand Up @@ -378,3 +387,40 @@ func EqualSidecar(dep, oldDep *appsv1.Deployment) bool {
oldDepContainer := oldDep.Spec.Template.Spec.Containers[oldDepIndex]
return reflect.DeepEqual(depContainer, oldDepContainer)
}

func parseAgentTags(args []string) map[string]string {
tagsArg := util.FindItem("--jaeger.tags=", args)
if tagsArg == "" {
return map[string]string{}
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
}
tagsParam := strings.SplitN(tagsArg, "=", 2)[1]
tagsMap := make(map[string]string)
tagsArr := strings.Split(tagsParam, ",")
for _, tagsPairStr := range tagsArr {
tagsPair := strings.SplitN(tagsPairStr, "=", 2)
tagsMap[tagsPair[0]] = tagsPair[1]
}
return tagsMap
}

func joinTags(tags map[string]string) string {
tagsSlice := make([]string, 0)
for key, value := range tags {
tagsSlice = append(tagsSlice, fmt.Sprintf("%s=%s", key, value))
}
sort.Strings(tagsSlice)
return strings.Join(tagsSlice, ",")
}

func getContainerName(containers []corev1.Container, agentIdx int) string {
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
if agentIdx == -1 && len(containers) == 1 { // we only have one single container and it is not the agent
return containers[0].Name
} else if agentIdx > -1 && len(containers)-1 == 1 { // we have one single container besides the agent
// agent: 0, app: 1
// agent: 1, app: 0
return containers[1-agentIdx].Name
} else {
// otherwise, we cannot determine `container.name`
return ""
}
}
115 changes: 98 additions & 17 deletions pkg/inject/sidecar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,8 +508,8 @@ func TestSidecarOrderOfArguments(t *testing.T) {
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[1].Args, "--c-option")
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[1].Args, "--jaeger.tags")
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[1].Args, "--reporter.grpc.host-port")
agentTags := agentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Contains(t, agentTags, "container.name=only_container")
agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Equal(t, agentTagsMap["container.name"], "only_container")
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are changing this PR for some other reason, you might want to remove the Contains, as the Equals assertion already covers it.

Copy link
Contributor Author

@lujiajing1126 lujiajing1126 Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you're right. Unnecessary Contains Removed. Also fix the same issue in two other places.

}

func TestSidecarExplicitTags(t *testing.T) {
Expand All @@ -523,8 +523,8 @@ func TestSidecarExplicitTags(t *testing.T) {

// verify
assert.Len(t, dep.Spec.Template.Spec.Containers, 2)
agentTags := agentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Equal(t, []string{"key=val"}, agentTags)
agentTags := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Equal(t, agentTags, map[string]string{"key": "val"})
}

func TestSidecarCustomReporterPort(t *testing.T) {
Expand Down Expand Up @@ -671,8 +671,29 @@ func TestSidecarAgentTagsWithMultipleContainers(t *testing.T) {
assert.Len(t, dep.Spec.Template.Spec.Containers, 3, "Expected 3 containers")
assert.Equal(t, "jaeger-agent", dep.Spec.Template.Spec.Containers[2].Name)
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[2].Args, "--jaeger.tags")
agentTags := agentTags(dep.Spec.Template.Spec.Containers[2].Args)
assert.Equal(t, "", util.FindItem("container.name=", agentTags))
agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[2].Args)
assert.NotContains(t, agentTagsMap, "container.name")
}

func TestSidecarAgentContainerNameTagWithDoubleInjectedContainer(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})
dep := Sidecar(jaeger, dep(map[string]string{}, map[string]string{}))

// inject - 1st time
assert.Equal(t, dep.Labels[Label], jaeger.Name)
assert.Len(t, dep.Spec.Template.Spec.Containers, 2, "Expected 2 containers")
assert.Equal(t, "jaeger-agent", dep.Spec.Template.Spec.Containers[1].Name)
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[1].Args, "--jaeger.tags")
agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Equal(t, agentTagsMap["container.name"], "only_container")

// inject - 2nd time due to deployment/namespace reconciliation
dep = Sidecar(jaeger, dep)
assert.Len(t, dep.Spec.Template.Spec.Containers, 2, "Expected 2 containers")
assert.Equal(t, "jaeger-agent", dep.Spec.Template.Spec.Containers[1].Name)
containsOptionWithPrefix(t, dep.Spec.Template.Spec.Containers[1].Args, "--jaeger.tags")
agentTagsMap = parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Equal(t, agentTagsMap["container.name"], "only_container")
}

func ns(annotations map[string]string) *corev1.Namespace {
Expand Down Expand Up @@ -734,15 +755,6 @@ func containsOptionWithPrefix(t *testing.T, args []string, prefix string) bool {
return false
}

func agentTags(args []string) []string {
tagsArg := util.FindItem("--jaeger.tags=", args)
if tagsArg == "" {
return []string{}
}
tagsParam := strings.SplitN(tagsArg, "=", 2)[1]
return strings.Split(tagsParam, ",")
}

func TestSidecarArgumentsOpenshiftTLS(t *testing.T) {
viper.Set("platform", v1.FlagPlatformOpenShift)
defer viper.Reset()
Expand All @@ -765,8 +777,9 @@ func TestSidecarArgumentsOpenshiftTLS(t *testing.T) {
assert.Greater(t, len(util.FindItem("--reporter.grpc.host-port=dns:///my-instance-collector-headless.test.svc:14250", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.enabled=true", dep.Spec.Template.Spec.Containers[1].Args)), 0)
assert.Greater(t, len(util.FindItem("--reporter.grpc.tls.ca="+ca.ServiceCAPath, dep.Spec.Template.Spec.Containers[1].Args)), 0)
agentTags := agentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Contains(t, agentTags, "container.name=only_container")
agentTagsMap := parseAgentTags(dep.Spec.Template.Spec.Containers[1].Args)
assert.Contains(t, agentTagsMap, "container.name")
assert.Equal(t, agentTagsMap["container.name"], "only_container")
}

func TestEqualSidecar(t *testing.T) {
Expand Down Expand Up @@ -863,3 +876,71 @@ func TestSidecarWithSecurityContext(t *testing.T) {
assert.Len(t, dep.Spec.Template.Spec.Containers, 2)
assert.Equal(t, dep.Spec.Template.Spec.Containers[1].SecurityContext, expectedSecurityContext)
}

func TestSortedTags(t *testing.T) {
defaultAgentTagsMap := make(map[string]string)
defaultAgentTagsMap["cluster"] = "undefined" // this value isn't currently available
defaultAgentTagsMap["deployment.name"] = "deploy"
defaultAgentTagsMap["pod.namespace"] = "ns"
defaultAgentTagsMap["pod.name"] = "pod_name"
defaultAgentTagsMap["host.ip"] = "0.0.0.0"
assert.Equal(t, joinTags(defaultAgentTagsMap), fmt.Sprintf("%s=%s,%s=%s,%s=%s,%s=%s,%s=%s",
"cluster", "undefined", // this value isn't currently available
"deployment.name", "deploy",
"host.ip", "0.0.0.0",
"pod.name", "pod_name",
"pod.namespace", "ns",
))
}

func TestSortedTagsWithContainer(t *testing.T) {
defaultAgentTagsMap := make(map[string]string)
defaultAgentTagsMap["cluster"] = "undefined" // this value isn't currently available
defaultAgentTagsMap["deployment.name"] = "deploy"
defaultAgentTagsMap["pod.namespace"] = "ns"
defaultAgentTagsMap["pod.name"] = "pod_name"
defaultAgentTagsMap["host.ip"] = "0.0.0.0"
defaultAgentTagsMap["container.name"] = "only_container"
assert.Equal(t, joinTags(defaultAgentTagsMap), fmt.Sprintf("%s=%s,%s=%s,%s=%s,%s=%s,%s=%s,%s=%s",
"cluster", "undefined", // this value isn't currently available
"container.name", "only_container",
"deployment.name", "deploy",
"host.ip", "0.0.0.0",
"pod.name", "pod_name",
"pod.namespace", "ns",
))
}

func TestParseEmptyAgentTags(t *testing.T) {
tags := parseAgentTags([]string{})
assert.Equal(t, tags, map[string]string{})
}

func TestGetContainerNameWithOneAppContainer(t *testing.T) {
deploy := dep(map[string]string{}, map[string]string{})
containerName := getContainerName(deploy.Spec.Template.Spec.Containers, -1)
assert.Equal(t, "only_container", containerName)
}

func TestGetContainerNameWithTwoAppContainers(t *testing.T) {
deploy := depWithTwoContainers(map[string]string{}, map[string]string{})
containerName := getContainerName(deploy.Spec.Template.Spec.Containers, -1)
assert.Equal(t, "", containerName)
}

func TestGetContainerNameWithAppContainerAndJaegerAgent(t *testing.T) {
nsn := types.NamespacedName{
Name: "my-instance",
Namespace: "Test",
}
jaeger := v1.NewJaeger(nsn)
deploy := dep(map[string]string{}, map[string]string{})
deploy = Sidecar(jaeger, deploy)

assert.Len(t, deploy.Spec.Template.Spec.Containers, 2)
hasAgent, agentIdx := HasJaegerAgent(deploy)
assert.True(t, hasAgent)
assert.Greater(t, agentIdx, -1)
containerName := getContainerName(deploy.Spec.Template.Spec.Containers, agentIdx)
assert.Equal(t, "only_container", containerName)
}