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

odiglet: Initialize clientset at startup and do OpenShift selinux changes last #2405

Merged
merged 5 commits into from
Feb 9, 2025

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Feb 7, 2025

This does 2 things in the Odiglet:

  • Make SELinux calls for openshift last in the init phase. These functions chroot to the host directory to run selinux commands that update the agent permissions, so they are readable by pods. This chroot was preventing the k8s client from initializing with open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory. Moved last so the chroot doesn't mess up future changes.
  • Initializes the k8s clientset in the main odiglet function. This allows the same clientset to be passed through to the init phase (which uses it to apply labels to nodes) and the odiglet controller. Doing so makes clientset an argument to functions like odiglet.New and k8snode.AddLabelToNode, the latter of which is just a helper which should not be initializing its own clientset every call anyway.

Technically, either one of these changes would have fixed the issue. But doing both is even better to help prevent future issues like this.

@damemi damemi requested review from RonFed and tamirdavid1 February 7, 2025 14:54
@damemi damemi force-pushed the openshift-chroot-fix branch from f9e3e90 to b2900fb Compare February 7, 2025 14:58
@damemi damemi added the bug Something isn't working label Feb 7, 2025
@damemi damemi enabled auto-merge (squash) February 7, 2025 15:12
@RonFed
Copy link
Collaborator

RonFed commented Feb 7, 2025

I understand the first change and not sure about the second.
The clientset in the init container is used once anyway so we can create it in init main function not in the odiglet main one.
In addition, the Odiglet.New is called by the enterprise repo and this change will break it. We could of course do the change there as well, but I prefer to share as much code as possible.

BTW, could/should we undo the chroot after we don't longer need it?

@damemi
Copy link
Contributor Author

damemi commented Feb 7, 2025

@RonFed it's messy to be initializing a new k8s client in different spots in the same code, especially in a helper method like AddLabelToNode. I can get this fix without it, but I would recommend we make this change in enterprise too.

Initializing early, especially in code that modifies host files like this, helps ensure the k8s client actually gets set up and would have prevented this issue.

@esara
Copy link
Contributor

esara commented Feb 8, 2025

I only got to check this today and just confirmed the same thing - I just did a simple fix

$ git diff
diff --git a/odiglet/odiglet.go b/odiglet/odiglet.go
index 287f0a9e..f3a7c313 100644
--- a/odiglet/odiglet.go
+++ b/odiglet/odiglet.go
@@ -184,11 +184,6 @@ func OdigletInitPhase() {
        if err := log.Init(); err != nil {
                panic(err)
        }
-       err := fs.CopyAgentsDirectoryToHost()
-       if err != nil {
-               log.Logger.Error(err, "Failed to copy agents directory to host")
-               os.Exit(-1)
-       }

        nn, ok := os.LookupEnv(k8sconsts.NodeNameEnvVar)
        if !ok {
@@ -205,5 +200,11 @@ func OdigletInitPhase() {
                os.Exit(-1)
        }

+       err := fs.CopyAgentsDirectoryToHost()
+       if err != nil {
+               log.Logger.Error(err, "Failed to copy agents directory to host")
+               os.Exit(-1)
+       }
+
        os.Exit(0)
 }

doing the copy at the end, since we are exiting at the end of the init, so the chroot goes away

@esara
Copy link
Contributor

esara commented Feb 8, 2025

Is there a place where the odiglet runs outside of k8s?
Your code assumes that you can always get the client, the previous code only called it from "/root/odiglet init"

@damemi damemi merged commit fcab2a9 into odigos-io:main Feb 9, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants