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

fix: improve error handling for tracing policies directory access #3289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arthur-zhang
Copy link
Contributor

improve error display when policy dir do not exist when starting tetragon.

Description

Before the change,the error message is not clear to figure out what happens when custom tracing policy dir do not exist, the error message is

time="2025-01-10T10:28:37+08:00" level=fatal msg="Failed to start tetragon" error="stat .: no such file or directory"

Now we check all directories upfront and provide better error context.

time="2025-01-10T10:38:06+08:00" level=fatal msg="Failed to start tetragon" error="Tracing Policies directory does not exist: /home/arthur/dev/tetragon/my_policy_dir"

Changelog

Optimize directory existence check in loadTpFromDir

improve error display when policy dir do not exist when starting tetragon.

Signed-off-by: arthur-zhang <[email protected]>
@arthur-zhang arthur-zhang requested a review from a team as a code owner January 10, 2025 02:44
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Hey thanks! Not related to your change but the existing code: I'm not a huge fan of this flow as it looks unnecessarily complicated and I don't exactly get why we don't want to fail in a situation where the directory is given but does not exists. @tixxdz do you remember why you added this "return nil" case? Should we keep this?

You could simplify this a bit as suggested, it's a less explicit error msg but the code flow is a bit more understandable.

Comment on lines +581 to +590
if os.IsNotExist(err) {
// If the default directory does not exist then do not fail
// Probably tetragon not fully installed, developers testing, etc
if dir == defaults.DefaultTpDir {
log.WithField("tracing-policy-dir", dir).Info("Loading Tracing Policies from directory ignored, directory does not exist")
return nil
}
return fmt.Errorf("Tracing Policies directory does not exist: %s", dir)
}
return fmt.Errorf("Failed to access tracing policies directory: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if os.IsNotExist(err) {
// If the default directory does not exist then do not fail
// Probably tetragon not fully installed, developers testing, etc
if dir == defaults.DefaultTpDir {
log.WithField("tracing-policy-dir", dir).Info("Loading Tracing Policies from directory ignored, directory does not exist")
return nil
}
return fmt.Errorf("Tracing Policies directory does not exist: %s", dir)
}
return fmt.Errorf("Failed to access tracing policies directory: %w", err)
// Do not fail if the default directory doesn't exist,
// it might because of developer setup or incomplete installation
if os.IsNotExist(err) && dir == defaults.DefaultTpDir {
log.WithField("tracing-policy-dir", dir).Info("Loading Tracing Policies from directory ignored, directory does not exist")
return nil
}
return fmt.Errorf("Failed to access tracing policies dir %s: %w", dir, err)

@mtardy mtardy added the release-note/misc This PR makes changes that have no direct user impact. label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants