Skip to content

Add read only file plugin #1128

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

Open
wants to merge 12 commits into
base: add-auxiliary-command-server-to-agent-config
Choose a base branch
from

Conversation

aphralG
Copy link
Contributor

@aphralG aphralG commented Jun 16, 2025

Proposed changes

File plugin subscribes to different topics depending on the server type.
Moved all handling of grpc to file service operator
Refactored file manager service, file plugin and file operator.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING document
  • I have run make install-tools and have attached any dependency changes to this pull request
  • If applicable, I have added tests that prove my fix is effective or that my feature works
  • If applicable, I have checked that any relevant tests pass after adding my changes
  • If applicable, I have updated any relevant documentation (README.md)
  • If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

@aphralG aphralG self-assigned this Jun 16, 2025
@aphralG aphralG requested a review from a team as a code owner June 16, 2025 15:07
@github-actions github-actions bot added the chore Pull requests for routine tasks label Jun 16, 2025

var _ bus.Plugin = (*ReadFilePlugin)(nil)

// The file plugin only writes, deletes and checks hashes of files
Copy link
Contributor

@sean-breen sean-breen Jun 19, 2025

Choose a reason for hiding this comment

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

Is this comment accurate for a 'read only' plugin?

@dhurley dhurley added the v3.x Issues and Pull Requests related to the major version v3 label Jun 19, 2025
}

// nolint: dupl
Copy link
Collaborator

Choose a reason for hiding this comment

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

To remove duplicate code between the file plugin and the read only file plugin, you could use embedding to take all the functionally from the read only file plugin and override whatever functions you need to change.

This can be done by updateing the FilePlugin struct to the following:

type FilePlugin struct {
	*ReadFilePlugin
}

// The file plugin only writes, deletes and checks hashes of files
// the file plugin does not care about the instance type

type ReadFilePlugin struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type ReadFilePlugin struct {
type ReadOnlyFilePlugin struct {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also can we change the name of the file to read_only_file_plugin.go?


func (rp *ReadFilePlugin) Info() *bus.Info {
return &bus.Info{
Name: "read",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Name: "read",
Name: "read-only-file",

}

func (rp *ReadFilePlugin) Init(ctx context.Context, messagePipe bus.MessagePipeInterface) error {
slog.DebugContext(ctx, "Starting read file plugin")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
slog.DebugContext(ctx, "Starting read file plugin")
slog.DebugContext(ctx, "Starting read only file plugin")

Should be consistent in the plugin name for the rest of the log statements in this file

"google.golang.org/protobuf/types/known/timestamppb"
)

type FileServiceOperator struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a comment here exampling the purpose of this operator

@aphralG aphralG requested review from dhurley and sean-breen June 23, 2025 13:47
Features: config.DefaultFeatures(),
},
expected: []bus.Plugin{
&resource.Resource{},
&command.CommandPlugin{},
&file.FilePlugin{},
&command.CommandPlugin{},
Copy link
Contributor

Choose a reason for hiding this comment

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

&command.CommandPlugin{} is here twice

Copy link
Contributor Author

@aphralG aphralG Jun 24, 2025

Choose a reason for hiding this comment

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

Ya thats right, when an auxiliary command server is configured there will be two command plugins and two file plugins, one for the command sever and one for the auxiliary sever

@aphralG aphralG requested a review from sean-breen June 26, 2025 11:21
conn: grpcConnection,
config: agentConfig,
conn: grpcConnection,
serverType: serverType,
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Init & Close functions can you add the serverType to the context?

fp.handleConfigApplyFailedRequest(ctx, msg)
default:
slog.DebugContext(ctx, "File plugin received unknown topic", "topic", msg.Topic)
if logger.ServerType(ctx) == fp.serverType.String() || logger.ServerType(ctx) == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If logger.ServerType(ctx) == "" then we should create a new context with the serverType.

@dhurley
Copy link
Collaborator

dhurley commented Jun 27, 2025

The FilePlugin Info function needs to be updated as well to reflect if the file plugin is for command or auxiliary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks v3.x Issues and Pull Requests related to the major version v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants