Skip to content

OCPBUGS-78195: fix(sync-global-pullsecret): compare content ignoring trailing newlines #7638

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

Merged
openshift-merge-bot merged 1 commit into from
Feb 26, 2026
Merged

OCPBUGS-78195: fix(sync-global-pullsecret): compare content ignoring trailing newlines #7638

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 {

@jparrill jparrill Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this interface necessary at all? I mean:

  • The interfaces usually are there to expose a function in a contract for a generic behavior. In this case this is pretty specific
  • The problem statement is the content comparison between files, could we change the code in that sense?

@devguyio devguyio Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The interface is necessary to allow for mocking Kubelet restarting. What alternative are you proposing?

@jparrill jparrill Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not at all, the issue this PR is trying to address (IMHO) is the comparison between the content processed in the HCCO vs the file content in the Kubelet. Alternative for kubelet restart, we can do the same we have already, something like:

// Global variable for testing
var kubeletRestartFunc = signalKubeletToRestartProcess

// In tests
func TestCheckAndFixFile(t *testing.T) {
  originalFunc := kubeletRestartFunc
  defer func() { kubeletRestartFunc = originalFunc }()

  kubeletRestartFunc = func() error {
     return nil // or whatever error you want to test
  }
}

@devguyio devguyio Feb 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the issue this PR is trying to address (IMHO)

agreed, and that includes improving the unit tests of the function where that logic no?

the existing unit tests were confusing because even in the happy path cases, it expected errors:

{
	name:               "file exists with different content",
	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,
	expectedErrorContains: []string{
		"failed to restart kubelet after 3 attempts",
		"rolled back changes",
	},
	expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`,
	expectError:          true,
},

if I'm reading that unit test, I would think that if the file exists with different content, the sync function should succeed and and there should be no errors, so why is expectError set to `true?

I understand this was done because kubelet restart wasn't mocked. So here's my thought process:

There are two ways that are the convention in this repo:

  1. Use a hand-made mock or a toggles (as you mentioned)
type kubeletRestarter = func() error
  1. Use mocking library for generating mocks.

My understanding is that we started recently adopting the gomock approach. For gomock, you need an interface. In both cases we need an extra type, whether it's an interface or an extra function type it depends IMHO on what you're trying to achieve with that extra type.

I'm fine switching to a hand-made mock and drop the gomock if that'll unblock this PR

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
Comment on lines +21 to 24

@coderabbitai coderabbitai Bot Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, explore the directory structure
find . -type f -name "*sync-global-pullsecret*" | head -20

Repository: openshift/hypershift

Length of output: 170

🏁 Script executed:

# Check if MockKubeletRestarter is defined anywhere in the codebase
rg "MockKubeletRestarter" --type go

Repository: openshift/hypershift

Length of output: 1236

🏁 Script executed:

# Look for mock generation directives
rg "go:generate" sync-global-pullsecret/ --type go

Repository: openshift/hypershift

Length of output: 254

🏁 Script executed:

# Check if the generated mock file exists
ls -la sync-global-pullsecret/sync-global-pullsecret_mock.go

Repository: openshift/hypershift

Length of output: 164

🏁 Script executed:

# If it exists, check if MockKubeletRestarter is defined in it
if [ -f sync-global-pullsecret/sync-global-pullsecret_mock.go ]; then
  rg "type MockKubeletRestarter" sync-global-pullsecret/sync-global-pullsecret_mock.go
  echo "---"
  rg "func NewMockKubeletRestarter" sync-global-pullsecret/sync-global-pullsecret_mock.go
fi

Repository: openshift/hypershift

Length of output: 46

🏁 Script executed:

# Also read the lines in the test file to see actual context
sed -n '21,24p' sync-global-pullsecret/sync-global-pullsecret_test.go
echo "---"
sed -n '213,224p' sync-global-pullsecret/sync-global-pullsecret_test.go

Repository: openshift/hypershift

Length of output: 589

MockKubeletRestarter is undefined; tests won't compile.

Static analysis reports MockKubeletRestarter missing. The generated mock file does not exist. Please regenerate and commit the mock (run go:generate or mockgen directly) so the type exists in package syncglobalpullsecret.

mockgen -destination=sync-global-pullsecret/sync-global-pullsecret_mock.go -package=syncglobalpullsecret ./sync-global-pullsecret . dbusConn,KubeletRestarter

Also applies to: 213-224

🧰 Tools
🪛 golangci-lint (2.5.0)

[error] 21-21: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:21:31: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:32:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:45:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:60:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:73:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:100:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:116:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:142:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:154:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:166:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:166:30: too many errors

(typecheck)

🤖 Prompt for AI Agents 
In `@sync-global-pullsecret/sync-global-pullsecret_test.go` around lines 21 - 24,
The tests reference MockKubeletRestarter which is missing; regenerate and commit
the generated mock for the syncglobalpullsecret package so the type exists. Run
the provided mockgen command (or the go:generate target) to produce
sync-global-pullsecret_mock.go in package syncglobalpullsecret exposing dbusConn
and KubeletRestarter, then add/commit that file so symbols like
MockKubeletRestarter used by the tests compile.

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