Skip to content

[release-4.21] OCPBUGS-77371: fix(sync-global-pullsecret): compare content ignoring trailing newlines #7809

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

Closed
openshift-cherrypick-robot wants to merge 1 commit into from
Closed

[release-4.21] OCPBUGS-77371: fix(sync-global-pullsecret): compare content ignoring trailing newlines #7809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions sync-global-pullsecret/sync-global-pullsecret.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 (
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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))
Expand Down
142 changes: 92 additions & 50 deletions sync-global-pullsecret/sync-global-pullsecret_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -87,72 +125,69 @@ 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,
},
}

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-*")
Expand All @@ -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
Expand Down