diff --git a/sync-global-pullsecret/sync-global-pullsecret.go b/sync-global-pullsecret/sync-global-pullsecret.go index 57a178a9a75f..279e09dfd696 100644 --- a/sync-global-pullsecret/sync-global-pullsecret.go +++ b/sync-global-pullsecret/sync-global-pullsecret.go @@ -3,6 +3,7 @@ package syncglobalpullsecret // sync-global-pullsecret syncs the pull secret from the user provided pull secret in DataPlane and appends it to the HostedCluster PullSecret to be deployed in the nodes of the HostedCluster. import ( + "bytes" "context" "encoding/json" "fmt" @@ -25,16 +26,30 @@ type syncGlobalPullSecretOptions struct { kubeletConfigJsonPath string } -//go:generate ../hack/tools/bin/mockgen -destination=sync-global-pullsecret_mock.go -package=syncglobalpullsecret . dbusConn +//go:generate ../hack/tools/bin/mockgen -destination=sync-global-pullsecret_mock.go -package=syncglobalpullsecret . dbusConn,KubeletRestarter type dbusConn interface { RestartUnit(name string, mode string, ch chan<- string) (int, error) Close() } +// KubeletRestarter is an interface for restarting the kubelet service. +// This allows tests to inject a mock implementation. +type KubeletRestarter interface { + Restart() error +} + +// realKubeletRestarter implements KubeletRestarter using systemd dbus. +type realKubeletRestarter struct{} + +func (r *realKubeletRestarter) Restart() error { + return signalKubeletToRestartProcess() +} + // GlobalPullSecretSyncer handles the synchronization of pull secrets type GlobalPullSecretSyncer struct { kubeletConfigJsonPath string log logr.Logger + kubeletRestarter KubeletRestarter } const ( @@ -110,6 +125,7 @@ func (o *syncGlobalPullSecretOptions) run(ctx context.Context) error { syncer := &GlobalPullSecretSyncer{ kubeletConfigJsonPath: o.kubeletConfigJsonPath, log: logger, + kubeletRestarter: &realKubeletRestarter{}, } // Start the sync loop @@ -195,16 +211,15 @@ func (s *GlobalPullSecretSyncer) checkAndFixFile(pullSecretBytes []byte) error { return fmt.Errorf("failed to read existing file: %w", err) } - // Preserve trailing newline if it exists in the original file contentToWrite := pullSecretBytes - if len(existingContent) > 0 && existingContent[len(existingContent)-1] == '\n' { - if len(pullSecretBytes) == 0 || pullSecretBytes[len(pullSecretBytes)-1] != '\n' { - contentToWrite = append(pullSecretBytes, '\n') - } - } - // If file content is different, write the desired content - if string(existingContent) != string(contentToWrite) { + // Compare content ignoring trailing newlines to avoid unnecessary restarts + // when only the newline format differs + existingTrimmed := bytes.TrimRight(existingContent, "\n") + newTrimmed := bytes.TrimRight(contentToWrite, "\n") + + // If actual content differs (ignoring trailing newlines), update the file + if !bytes.Equal(existingTrimmed, newTrimmed) { s.log.Info("file content is different, updating it") // Save original content for potential rollback originalContent := existingContent @@ -219,7 +234,7 @@ func (s *GlobalPullSecretSyncer) checkAndFixFile(pullSecretBytes []byte) error { maxRetries := 3 var lastErr error for attempt := 1; attempt <= maxRetries; attempt++ { - if err := signalKubeletToRestartProcess(); err != nil { + if err := s.kubeletRestarter.Restart(); err != nil { lastErr = err if attempt < maxRetries { s.log.Info(fmt.Sprintf("Attempt %d failed, retrying...: %v", attempt, err)) diff --git a/sync-global-pullsecret/sync-global-pullsecret_test.go b/sync-global-pullsecret/sync-global-pullsecret_test.go index 62f7f86d501c..7655a0f6096c 100644 --- a/sync-global-pullsecret/sync-global-pullsecret_test.go +++ b/sync-global-pullsecret/sync-global-pullsecret_test.go @@ -18,17 +18,33 @@ func TestCheckAndFixFile(t *testing.T) { initialContent string secretContent string rollbackShouldFail bool + setupKubeletMock func(*MockKubeletRestarter) expectedErrorContains []string expectedFinalContent string expectError bool description string }{ { - name: "file does not exist", + name: "When file does not exist and kubelet restart succeeds it should create file with new content", + description: "file does not exist, kubelet restart succeeds, file is created", + initialContent: "", + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(nil) + }, + expectedErrorContains: []string{}, + expectedFinalContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + expectError: false, + }, + { + name: "When file does not exist and kubelet restart fails it should rollback", description: "file does not exist, kubelet restart fails, rollback succeeds", initialContent: "", secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: false, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(fmt.Errorf("dbus error")).Times(3) + }, expectedErrorContains: []string{ "failed to restart kubelet after 3 attempts", "rolled back changes", @@ -37,11 +53,26 @@ func TestCheckAndFixFile(t *testing.T) { expectError: true, }, { - name: "file exists with different content", + name: "When file exists with different content and kubelet restart succeeds it should update file", + description: "file exists with different content, kubelet restart succeeds", + initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(nil) + }, + expectedErrorContains: []string{}, + expectedFinalContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + expectError: false, + }, + { + name: "When file exists with different content and kubelet restart fails it should rollback", description: "file exists with different content, kubelet restart fails, rollback succeeds", initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: false, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(fmt.Errorf("dbus error")).Times(3) + }, expectedErrorContains: []string{ "failed to restart kubelet after 3 attempts", "rolled back changes", @@ -50,21 +81,25 @@ func TestCheckAndFixFile(t *testing.T) { expectError: true, }, { - name: "file exists with same content", + name: "When file exists with same content it should not restart kubelet", description: "file exists with same content, no changes needed", initialContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: false, + setupKubeletMock: nil, // No restart expected expectedErrorContains: []string{}, expectedFinalContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, expectError: false, }, { - name: "rollback succeeds", + name: "When kubelet restart fails it should rollback to original content", description: "kubelet restart fails but rollback succeeds, file should be restored to original content", initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: false, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(fmt.Errorf("dbus error")).Times(3) + }, expectedErrorContains: []string{ "failed to restart kubelet after 3 attempts", "rolled back changes", @@ -73,11 +108,14 @@ func TestCheckAndFixFile(t *testing.T) { expectError: true, }, { - name: "rollback fails", + name: "When both kubelet restart and rollback fail it should return combined error", description: "both kubelet restart and rollback fail, file should remain with new content", initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: true, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(fmt.Errorf("dbus error")).Times(3) + }, expectedErrorContains: []string{ "2 errors happened", "the kubelet restart failed after 3 attempts", @@ -87,65 +125,60 @@ func TestCheckAndFixFile(t *testing.T) { expectError: true, }, { - name: "preserve trailing newline when original file has one", - description: "file has trailing newline, new content doesn't, should preserve newline", - initialContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", - secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, - rollbackShouldFail: false, - expectedErrorContains: []string{ - "failed to restart kubelet after 3 attempts", - "rolled back changes", - }, - expectedFinalContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", - expectError: true, + name: "When only trailing newline differs it should not restart kubelet", + description: "file has trailing newline, new content doesn't, should not trigger restart", + initialContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + setupKubeletMock: nil, // No restart expected - content is same ignoring newline + expectedErrorContains: []string{}, + expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", // File unchanged + expectError: false, }, { - name: "preserve single newline when both have newlines", - description: "both original file and new content have trailing newlines, should preserve single newline", - initialContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", - secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", - rollbackShouldFail: false, - expectedErrorContains: []string{ - "failed to restart kubelet after 3 attempts", - "rolled back changes", + name: "When content differs and both have newlines it should update and restart", + description: "both original file and new content have trailing newlines, different content", + initialContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", + secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(nil) }, - expectedFinalContent: "{\"auths\":{\"old.registry.com\":{\"auth\":\"b2xkOnRlc3Q=\"}}}\n", - expectError: true, + expectedErrorContains: []string{}, + expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + expectError: false, }, { - name: "no newline when original file has none", - description: "original file has no newline, new content has newline, should preserve new content format", - initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, - secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", - rollbackShouldFail: false, - expectedErrorContains: []string{ - "failed to restart kubelet after 3 attempts", - "rolled back changes", + name: "When content differs with newline in secret it should write exact secret content", + description: "original file has no newline, new content has newline, should write new content exactly", + initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + secretContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(nil) }, - expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, - expectError: true, + expectedErrorContains: []string{}, + expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + expectError: false, }, { - name: "no newlines preserved", - description: "neither original file nor new content have newlines, should preserve format", - initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, - secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, - rollbackShouldFail: false, - expectedErrorContains: []string{ - "failed to restart kubelet after 3 attempts", - "rolled back changes", + name: "When content differs without newlines it should update and restart", + description: "neither original file nor new content have newlines, should update", + initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, + secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + setupKubeletMock: func(m *MockKubeletRestarter) { + m.EXPECT().Restart().Return(nil) }, - expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`, - expectError: true, + expectedErrorContains: []string{}, + expectedFinalContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, + expectError: false, }, { - name: "same content with newline - no change needed", - description: "file content is identical including newline, no restart should be attempted", + name: "When file has newline and secret does not but content is same it should not restart", + description: "file content is identical ignoring newline, no restart should be attempted", initialContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`, rollbackShouldFail: false, + setupKubeletMock: nil, // No restart expected expectedErrorContains: []string{}, - expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", + expectedFinalContent: "{\"auths\":{\"test.registry.com\":{\"auth\":\"dGVzdDp0ZXN0\"}}}\n", // File unchanged expectError: false, }, } @@ -153,6 +186,8 @@ func TestCheckAndFixFile(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() // Create a temporary directory for test files tempDir, err := os.MkdirTemp("", "sync-pullsecret-test-*") @@ -175,10 +210,17 @@ func TestCheckAndFixFile(t *testing.T) { g.Expect(string(content)).To(Equal(tt.initialContent)) } - // Create syncer for testing + // Create mock kubelet restarter + mockRestarter := NewMockKubeletRestarter(ctrl) + if tt.setupKubeletMock != nil { + tt.setupKubeletMock(mockRestarter) + } + + // Create syncer for testing with mock syncer := &GlobalPullSecretSyncer{ kubeletConfigJsonPath: testFilePath, log: logr.Discard(), + kubeletRestarter: mockRestarter, } // Save original write function and restore it after test