Skip to content

Allow multiple management planes #1124

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

Jump to bottom
Open
wants to merge 2 commits into
base: add-auxiliary-command-server-proto
Choose a base branch
from
Open

Allow multiple management planes #1124

wants to merge 2 commits into from

Conversation

aphralG
Copy link
Contributor

@aphralG aphralG commented Jun 12, 2025

Proposed changes

Add Auxiliary Command Server block to agent config.
Modified Command plugin to take a command server type so two can be created one for each Management plane
Added CommandServerType to context for management plane sepcific messages "command" or "auxiliary"

auxiliary_command:
    server: 
        host: "second.management.plane"
        port: 9999
    auth: 
        token: "1234"
        tokenpath: "path/to/my_token"
    tls: 
        cert: "some.cert"
        key: "some.key"
        ca: "some.ca"
        skip_verify: false
        server_name: "server-name"

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 12, 2025
@aphralG aphralG requested a review from a team as a code owner June 12, 2025 15:43
@github-actions github-actions bot added the chore Pull requests for routine tasks label Jun 12, 2025
dhurley
dhurley reviewed Jun 16, 2025
View reviewed changes
internal/command/command_plugin.go Outdated
conn grpc.GrpcConnectionInterface
commandService commandService
subscribeChannel chan *mpi.ManagementPlaneRequest
commandServerType string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this be an enum?

internal/command/command_plugin.go Outdated

cp.messagePipe = messagePipe
cp.commandService = NewCommandService(cp.conn.CommandServiceClient(), cp.config, cp.subscribeChannel)

go cp.monitorSubscribeChannel(ctx)
newCtx := context.WithValue(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you create the new context at the start of the function? Just so it can be used for the debug message above as well.

internal/logger/logger.go
@@ -33,6 +34,7 @@ var (
}

CorrelationIDContextKey = contextKey(CorrelationIDKey)
ServerTypeContextKey = contextKey(ServerTypeKey)
)

type (
Copy link
Collaborator

Choose a reason for hiding this comment

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

On line 77 can you add ServerTypeContextKey to the list pf handlers so that the server type gets automatically added to the log messages?

internal/command/command_plugin.go Outdated
slog.DebugContext(ctx, "Command plugin received unknown topic", "topic", msg.Topic)
}
} else {
slog.Info("Sever type is not right ignoring message", "command_server_type",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove this info log as it will spam the log file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya this was supposed to be removed was just using for testing

internal/command/command_plugin.go Outdated
@@ -226,12 +248,24 @@ func (cp *CommandPlugin) monitorSubscribeChannel(ctx context.Context) {
slog.InfoContext(ctx, "Received management plane config upload request")
cp.handleConfigUploadRequest(newCtx, message)
case *mpi.ManagementPlaneRequest_ConfigApplyRequest:
if cp.commandServerType != "command" {
slog.WarnContext(newCtx, "Auxiliary command server can not perform config apply",
"command_server_type", cp.commandServerType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think you need to send a failure response back to the management plane here as well

internal/command/command_plugin.go
slog.InfoContext(ctx, "Received management plane config apply request")
cp.handleConfigApplyRequest(newCtx, message)
case *mpi.ManagementPlaneRequest_HealthRequest:
slog.InfoContext(ctx, "Received management plane health request")
cp.handleHealthRequest(newCtx)
case *mpi.ManagementPlaneRequest_ActionRequest:
if cp.commandServerType != "command" {
slog.WarnContext(newCtx, "Auxiliary command server can not perform api action",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as comment above

internal/command/command_service.go Outdated
@@ -261,6 +261,7 @@ func (cs *CommandService) UpdateClient(ctx context.Context, client mpi.CommandSe
cs.subscribeClientMutex.Unlock()

cs.isConnected.Store(false)
// Will this have the sever type ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove comment?

internal/plugin/plugin_manager.go Outdated
// Followup PR to add read plugin eventually
}
} else {
slog.InfoContext(ctx, "Agent is not connected to an auxiliary management plane. "+
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this log message will cause confusion to the end user. Maybe we should just have a debug message for when an auxiliary command server isn'r configured

sean-breen
sean-breen requested changes Jun 19, 2025
View reviewed changes
internal/command/command_plugin.go
@@ -336,3 +395,7 @@ func (cp *CommandPlugin) createDataPlaneResponse(correlationID string, status mp
},
}
}

func (s ServerType) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to the top near the ServerType declaration?

internal/plugin/plugin_manager.go
@@ -59,6 +60,26 @@ func addCommandAndFilePlugins(ctx context.Context, plugins []bus.Plugin, agentCo
return plugins
}

func addAuxiliaryCommandAndFilePlugins(ctx context.Context, plugins []bus.Plugin,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be addAuxiliaryCommandPlugin?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhurley dhurley dhurley left review comments

@sean-breen sean-breen sean-breen requested changes

Assignees

@aphralG aphralG

Labels
chore Pull requests for routine tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aphralG @dhurley @sean-breen