-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add token file watcher #979
base: v3
Are you sure you want to change the base?
Conversation
* modify alpine package name: nginx-agent-3.0.0_1234 -> nginx-agent-3.0.0.1234 * protoc-gen update
* update config defaults and format
func (w *Watcher) monitorWatchers(ctx context.Context) { | ||
for { | ||
select { | ||
case <-ctx.Done(): | ||
return | ||
case message := <-w.credentialUpdatesChannel: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this watcher also need to be stopped during a config apply ? Or check if a config apply is in progress ?
Could the token path be added to the full config test internal/config/config_test.go |
func (cp *CommandPlugin) processConnectionReset(ctx context.Context, msg *bus.Message) { | ||
slog.DebugContext(ctx, "Command plugin received connection reset") | ||
if newConnection, ok := msg.Data.(grpc.GrpcConnectionInterface); ok { | ||
if !cp.commandService.IsConnected() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If its not connected should we still update the command service with the new connection & client?
@@ -212,7 +212,7 @@ func TestGrpc_Reconnection(t *testing.T) { | |||
require.NoError(t, err) | |||
mockManagementPlaneAPIAddress = net.JoinHostPort(ipAddress, ports["9093/tcp"][0].HostPort) | |||
|
|||
time.Sleep(5 * time.Second) | |||
time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a sleep could we increase the retry max wait here instead? https://github.com/nginx/agent/blob/v3/test/integration/grpc_management_plane_api_test.go#L565
) | ||
|
||
const ( | ||
Create = fsnotify.Create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these constants used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they're used in isEventSkippable
to check the type of the incoming FSNotify event
func isEventSkippable(event fsnotify.Event) bool { |
Proposed changes
CredentialWatcherService
which will initially monitor the file pointed to bytokenpath
option in agent conf.Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTING
documentmake install-tools
and have attached any dependency changes to this pull requestREADME.md
)