fix: set container after modification

fengxsong commented 2 years ago

Signed-off-by: fengxsong fengxsong@outlook.com

Should set container after modification, otherwise it won't be affected.

ghost commented 2 years ago

👇 Click on the image for a new way to code review

- Make big changes easier — review code in small groups of related files - Know where to start — see the whole change at a glance - Take a code tour — explore the change with an interactive tour - Make comments and review — all fully sync’ed with github [Try it now!](https://app.codesee.io/r/reviews?pr=398&src=https%3A%2F%2Fgithub.com%2Fdocker-slim%2Fdocker-slim)

Legend

CodeSee Map Legend

kcq commented 2 years ago

@iximiuz can you take a look at this enhancement

iximiuz commented 2 years ago

I'll need to run this code, but at first sight, this change is not needed (since the modified container is a pointer and we were modifying it in place).

fengxsong commented 2 years ago

I'll need to run this code, but at first sight, this change is not needed (since the modified container is a pointer and we were modifying it in place).

~~It's needed actually. cause type of .spec.containers is []Container but not []*Container~~

I've check line https://github.com/docker-slim/docker-slim/blob/master/pkg/app/master/kubernetes/workload.go#L67, maybe not this reason. It's odd. LoL

iximiuz commented 2 years ago

@fengxsong easily can be! :) That's why I said I'll have to run this code before actually deciding on the action. And in any case, thanks a lot for your contribution!

iximiuz commented 2 years ago

@fengxsong you are totally right, PodTemplateSpec.Spec.Containers has a type []Container. You can see it here. However, as you discovered above, I return a pointer to an element in this list. So, modifying it in place should be fine.

Here is what I tried to validate this reasoning (yep, I should have written a proper test, but there is no time for that at the moment, unfortunately):

And here is what I got:

TEMPLATE (BEFORE) "{\"metadata\":{\"creationTimestamp\":null,\"labels\":{\"app\":\"my-k8s-app\"}},\"spec\":{\"containers\":[{\"name\":\"my-k8s-app\",\"image\":\"dslimexamples/my-k8s-app\",\"ports\":[{\"containerPort\":1300,\"protocol\":\"TCP\"}],\"resources\":{},\"terminationMessagePath\":\"/dev/termination-log\",\"terminationMessagePolicy\":\"File\",\"imagePullPolicy\":\"IfNotPresent\"}],\"restartPolicy\":\"Always\",\"terminationGracePeriodSeconds\":30,\"dnsPolicy\":\"ClusterFirst\",\"securityContext\":{},\"schedulerName\":\"default-scheduler\"}}"

TEMPLATE (AFTER) "{\"metadata\":{\"creationTimestamp\":null,\"labels\":{\"app\":\"my-k8s-app\",\"dockersl.im/target-pod\":\"dockerslimk_564136_20221003195156\"}},\"spec\":{\"volumes\":[{\"name\":\"dockerslim-sensor\",\"emptyDir\":{}},{\"name\":\"dockerslim-artifacts\",\"emptyDir\":{}}],\"initContainers\":[{\"name\":\"dockerslim-sensor-loader\",\"image\":\"alpine\",\"command\":[\"sh\",\"-c\",\"until [ -f /opt/dockerslim/bin/docker-slim-sensor ]; do echo \\\"Waiting for sensor to appear...\\\"; sleep 1; done; echo \\\"Sensor found! Exiting...\\\"\"],\"resources\":{},\"volumeMounts\":[{\"name\":\"dockerslim-sensor\",\"mountPath\":\"/opt/dockerslim/bin\"}]}],\"containers\":[{\"name\":\"my-k8s-app\",\"image\":\"dslimexamples/my-k8s-app\",\"command\":[\"/opt/dockerslim/bin/docker-slim-sensor\"],\"ports\":[{\"containerPort\":1300,\"protocol\":\"TCP\"}],\"resources\":{},\"volumeMounts\":[{\"name\":\"dockerslim-sensor\",\"mountPath\":\"/opt/dockerslim/bin\"},{\"name\":\"dockerslim-artifacts\",\"mountPath\":\"/opt/dockerslim/artifacts\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"terminationMessagePolicy\":\"File\",\"imagePullPolicy\":\"IfNotPresent\",\"securityContext\":{\"capabilities\":{\"add\":[\"SYS_ADMIN\"]},\"privileged\":true}}],\"restartPolicy\":\"Always\",\"terminationGracePeriodSeconds\":30,\"dnsPolicy\":\"ClusterFirst\",\"securityContext\":{},\"schedulerName\":\"default-scheduler\"}}"

CONTAINER "{\"name\":\"my-k8s-app\",\"image\":\"dslimexamples/my-k8s-app\",\"command\":[\"/opt/dockerslim/bin/docker-slim-sensor\"],\"ports\":[{\"containerPort\":1300,\"protocol\":\"TCP\"}],\"resources\":{},\"volumeMounts\":[{\"name\":\"dockerslim-sensor\",\"mountPath\":\"/opt/dockerslim/bin\"},{\"name\":\"dockerslim-artifacts\",\"mountPath\":\"/opt/dockerslim/artifacts\"}],\"terminationMessagePath\":\"/dev/termination-log\",\"terminationMessagePolicy\":\"File\",\"imagePullPolicy\":\"IfNotPresent\",\"securityContext\":{\"capabilities\":{\"add\":[\"SYS_ADMIN\"]},\"privileged\":true}}"

In other words, the template after the modification has the changes (look for SYS_ADMIN string literal, for instance). So, it's a pity, but I'm inclined to close this PR instead of merging.

fengxsong commented 2 years ago

@iximiuz Hi there, from what I observed, the template doesn't have that change. here's my test case

$ git diff
diff --git a/pkg/app/master/inspectors/pod/pod_inspector.go b/pkg/app/master/inspectors/pod/pod_inspector.go
index 5b7744fa..f61b95ca 100644
--- a/pkg/app/master/inspectors/pod/pod_inspector.go
+++ b/pkg/app/master/inspectors/pod/pod_inspector.go
@@ -2,6 +2,7 @@ package pod

 import (
    "context"
+   "encoding/json"
    "errors"
    "fmt"
    "os"
@@ -168,6 +169,7 @@ func (i *Inspector) TargetHost() string {
 }

 func (i *Inspector) RunPod() error {
+   fmt.Println("BEFORE", toJSON(i.workload.Template()))
    if err := i.prepareWorkload(); err != nil {
        return err
    }
@@ -175,6 +177,8 @@ func (i *Inspector) RunPod() error {
    if err := i.applyWorkload(); err != nil {
        return err
    }
+   fmt.Println("AFTER", toJSON(i.workload.Template()))
+   panic("here")

    if err := waitForContainer(i.ctx, i.kubeClient, i.pod.Namespace, i.pod.Name, sensorLoaderContainer, true); err != nil {
        return err
@@ -365,11 +369,20 @@ func (i *Inspector) prepareWorkload() error {
        targetCont.SecurityContext.Capabilities.Add,
        "SYS_ADMIN",
    )
-   i.workload.SetContainer(targetCont)
+   // i.workload.SetContainer(targetCont)
+   fmt.Println("INNER", toJSON(i.workload.Template()))

    return nil
 }

+func toJSON(v interface{}) string {
+   b, err := json.Marshal(v)
+   if err != nil {
+       fmt.Println(err)
+   }
+   return string(b)
+}
+
 func (i *Inspector) applyWorkload() error {
    if err := i.kubeClient.CreateOrUpdate(i.ctx, i.workload.Info()); err != nil {
        return err
$ make build_dev
$ sudo cp bin/* /usr/local/bin/
$ # testing
$ docker-slim build --target-kube-workload deployment/test --target-kube-workload-namespace=default --target-kube-workload-container=test
app='docker-slim' message='Join the Gitter channel to ask questions or to share your feedback' info='https://gitter.im/docker-slim/community'
app='docker-slim' message='Join the Discord server to ask questions or to share your feedback' info='https://discord.gg/9tDyxYS'
app='docker-slim' message='GitHub Discussions' info='https://github.com/docker-slim/docker-slim/discussions'
cmd=build info=param.http.probe message='using default probe'
cmd=build state=started
cmd=build info=params target.namespace='default' target.container='test' target.image='' continue.mode='probe' target.type='kubernetes.workload' target='deployment/test'
cmd=build info=kubernetes.workload target='{"name":"test","image":"dockerhub.tencentcloudcr.com/library/nginx:1.21","resources":{},"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}' namespace='default' name='test' template='{"metadata":{"creationTimestamp":null,"labels":{"app":"test"}},"spec":{"containers":[{"name":"test","image":"dockerhub.tencentcloudcr.com/library/nginx:1.21","resources":{},"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","securityContext":{},"schedulerName":"default-scheduler","tolerations":[{"operator":"Exists"}]}}'
cmd=build state=image.inspection.start
cmd=build info=image id='sha256:0e901e68141fd02f237cf63eb842529f8a9500636a9419e3cf4fb986b8fe3d5d' size.bytes='141526528' size.human='142 MB'
cmd=build info=image.stack index='0' name='nginx:1.21' id='sha256:0e901e68141fd02f237cf63eb842529f8a9500636a9419e3cf4fb986b8fe3d5d'
cmd=build info=image.exposed_ports list='80'
cmd=build state=image.inspection.done
BEFORE {"metadata":{"creationTimestamp":null,"labels":{"app":"test"}},"spec":{"containers":[{"name":"test","image":"dockerhub.tencentcloudcr.com/library/nginx:1.21","resources":{},"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","securityContext":{},"schedulerName":"default-scheduler","tolerations":[{"operator":"Exists"}]}}
INNER {"metadata":{"creationTimestamp":null,"labels":{"app":"test","dockersl.im/target-pod":"dockerslimk_568948_20221004131756"}},"spec":{"volumes":[{"name":"dockerslim-sensor","emptyDir":{}},{"name":"dockerslim-artifacts","emptyDir":{}}],"initContainers":[{"name":"dockerslim-sensor-loader","image":"alpine","command":["sh","-c","until [ -f /opt/dockerslim/bin/docker-slim-sensor ]; do echo \"Waiting for sensor to appear...\"; sleep 1; done; echo \"Sensor found! Exiting...\""],"resources":{},"volumeMounts":[{"name":"dockerslim-sensor","mountPath":"/opt/dockerslim/bin"}]}],"containers":[{"name":"test","image":"dockerhub.tencentcloudcr.com/library/nginx:1.21","resources":{},"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","securityContext":{},"schedulerName":"default-scheduler","tolerations":[{"operator":"Exists"}]}}
cmd=build info=pod status='created' namespace='default' name='test-75864846cc-pmfp8'
AFTER {"metadata":{"creationTimestamp":null,"labels":{"app":"test","dockersl.im/target-pod":"dockerslimk_568948_20221004131756"}},"spec":{"volumes":[{"name":"dockerslim-sensor","emptyDir":{}},{"name":"dockerslim-artifacts","emptyDir":{}}],"initContainers":[{"name":"dockerslim-sensor-loader","image":"alpine","command":["sh","-c","until [ -f /opt/dockerslim/bin/docker-slim-sensor ]; do echo \"Waiting for sensor to appear...\"; sleep 1; done; echo \"Sensor found! Exiting...\""],"resources":{},"volumeMounts":[{"name":"dockerslim-sensor","mountPath":"/opt/dockerslim/bin"}],"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}],"containers":[{"name":"test","image":"dockerhub.tencentcloudcr.com/library/nginx:1.21","resources":{},"terminationMessagePath":"/dev/termination-log","terminationMessagePolicy":"File","imagePullPolicy":"Always"}],"restartPolicy":"Always","terminationGracePeriodSeconds":30,"dnsPolicy":"ClusterFirst","securityContext":{},"schedulerName":"default-scheduler","tolerations":[{"operator":"Exists"}]}}
app='docker-slim' message='Join the Gitter channel to ask questions or to share your feedback' info='https://gitter.im/docker-slim/community'
app='docker-slim' message='Join the Discord server to ask questions or to share your feedback' info='https://discord.gg/9tDyxYS'
app='docker-slim' message='GitHub Discussions' info='https://github.com/docker-slim/docker-slim/discussions'
panic: here
$ go env GOVERSION
go1.19

kcq commented 1 year ago

@CodiumAI-Agent /review

CodiumAI-Agent commented 1 year ago

PR Analysis

🎯 Main theme: Fixing container setting after modification
📌 Type of PR: Bug fix
🧪 Relevant tests added: No
✨ Focused PR: Yes, the PR has a clear title and description that matches the changes made in the code. The PR is focused on fixing the issue of setting the container after modification.
🔒 Security concerns: No, the changes made in this PR do not seem to introduce any potential security issues.

PR Feedback
💡 General PR suggestions: The changes in the PR seem to be addressing the issue described in the PR description. However, it would be beneficial to add tests to verify the correctness of the changes. This would help in ensuring that the changes are working as expected and prevent potential regressions in the future.

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands: /review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option. /describe: Modify the PR title and description based on the contents of the PR. /improve: Suggest improvements to the code in the PR. /ask \<QUESTION>: Pose a question about the PR.

To edit any configuration parameter from 'configuration.toml', add --config_path=new_value For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."

kcq commented 1 year ago

@CodiumAI-Agent /describe

slimtoolkit / slim