Skip to content

File Settings should return error in case the file itself does not exist #437

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

Open
fmoehler wants to merge 2 commits into
base: main
Choose a base branch
from
Open

File Settings should return error in case the file itself does not exist #437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b2bbfb
Update file_settings_source.go
fmoehler May 28, 2026
f98de31
Add file path to error message and logging for SSH key failures
fmoehler May 29, 2026
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
3 changes: 3 additions & 0 deletions infrastructure/file_settings_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ func NewFileSettingsSource(
}

func (s *FileSettingsSource) PublicSSHKeyForUsername(string) (string, error) {
if !s.fs.FileExists(s.settingsFilePath) {
return "", bosherr.Errorf("File for settings source does not exist: '%s'", s.settingsFilePath)
}
Comment thread
fmoehler marked this conversation as resolved.
return "", nil
}
Comment on lines 37 to 42
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical logic error: must always return an error to enable fallback.

The current implementation only returns an error when the settings file is missing (lines 38-40), but returns ("", nil) when the file exists (line 41). Based on the fallback logic in infrastructure/multi_settings_source.go (lines 38-40), the agent stops at the first source that returns err == nil without trying remaining sources. When the settings file exists, this method will return success with an empty SSH key, preventing the agent from falling back to other SSH key sources—defeating the entire purpose of this PR.

Per the PR description, the file settings source "does not return an SSH key" regardless of whether its settings file exists. It should always return an error when an SSH key is requested.

🐛 Proposed fix to always return an error 
 func (s *FileSettingsSource) PublicSSHKeyForUsername(string) (string, error) {
-	if !s.fs.FileExists(s.settingsFilePath) {
-		return "", bosherr.Error("File for settings source does not exist")
-	}
-	return "", nil
+	return "", bosherr.Error("File settings source does not provide SSH keys")
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s *FileSettingsSource) PublicSSHKeyForUsername(string) (string, error) {
if !s.fs.FileExists(s.settingsFilePath) {
return "", bosherr.Error("File for settings source does not exist")
}
return "", nil
}
func (s *FileSettingsSource) PublicSSHKeyForUsername(string) (string, error) {
return "", bosherr.Error("File settings source does not provide SSH keys")
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@infrastructure/file_settings_source.go` around lines 37 - 42, The
PublicSSHKeyForUsername method on FileSettingsSource currently returns ("", nil)
when the settings file exists which stops fallback; change it so it always
returns a non-nil error when asked for an SSH key (even if
s.fs.FileExists(s.settingsFilePath) is true). In other words, keep the existing
missing-file error branch, and replace the final return ("", nil) in
FileSettingsSource.PublicSSHKeyForUsername with a return of ("",
bosherr.Error(...)) that clearly states this source does not provide SSH keys so
multi-source fallback can continue.

Copy link
Copy Markdown
Contributor Author

@fmoehler fmoehler May 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Valid Point. However I don't know about the history of this and therefore I tried to keep the existing logic in case the file exists

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 28, 2026

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check multi_settings_source.go for the fallback/iteration logic
cat infrastructure/multi_settings_source.go

echo "---"
# Check git log for PublicSSHKeyForUsername history in file_settings_source.go
git log --oneline -10 -- infrastructure/file_settings_source.go

echo "---"
# Check what interface PublicSSHKeyForUsername belongs to
rg -n "PublicSSHKeyForUsername" --type=go -C3

echo "---"
# Check how PublicSSHKeyForUsername is called in the agent
rg -n "PublicSSHKeyForUsername" --type=go -B5 -A10

Length of output: 59807

@fmoehler, thanks for the explanation — that context is helpful. Your reasoning does hold for the primary scenario described in the PR:

  • File doesn't exist → return error → multi-source continues to the next source (e.g. config drive) that can provide actual SSH keys ✓
  • File exists → return ("", nil) → bootstrap.go (lines 57-65) handles an empty key gracefully by skipping SSH setup ✓

I also notice that several other sources (CDROMSettingsSource, VsphereGuestInfoSettingsSource, InstanceMetadataSettingsSource) follow the same "always return ("", nil)" pattern, so this is consistent with how those sources behave.

The one remaining edge case worth calling out: if a stemcell config ever places FileSettingsSource before a source that provides actual SSH keys (e.g. config drive), and the file exists, the multi-source would stop at FileSettingsSource and the SSH key from the later source would never be found. But based on what you've described, that doesn't appear to be the intended deployment topology — file-based settings are for environments that don't rely on IaaS SSH key injection.

Given that context, the current implementation looks reasonable as a conservative fix. The PR achieves its stated goal.

🧠 Learnings used 
Learnt from: julian-hj
Repo: cloudfoundry/bosh-agent PR: 431
File: platform/net/firewall_provider_windows.go:77-80
Timestamp: 2026-05-20T18:36:44.653Z
Learning: When handling/parsing NATS URLs anywhere in this bosh-agent codebase, do not add implicit/default port fallback behavior (e.g., defaulting to port 4222 when the URL omits a port). NATS URLs are expected to be machine-generated and should always include an explicit port; if the port is missing, treat it as an invalid/unsupported input rather than introducing an extra defaulting code path.


Expand Down
30 changes: 26 additions & 4 deletions infrastructure/file_settings_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,32 @@ var _ = Describe("FileSettingsSource", func() {
})

Describe("PublicSSHKeyForUsername", func() {
It("returns an empty string", func() {
publicKey, err := source.PublicSSHKeyForUsername("fake-username")
Expect(err).ToNot(HaveOccurred())
Expect(publicKey).To(Equal(""))
Context("when the settings file exists", func() {
BeforeEach(func() {
settingsFileName := "/fake-settings-file-path"
source = infrastructure.NewFileSettingsSource(settingsFileName, fs, logger)
err := fs.WriteFileString(settingsFileName, "{}")
Expect(err).NotTo(HaveOccurred())
})

It("returns an empty string", func() {
publicKey, err := source.PublicSSHKeyForUsername("fake-username")
Expect(err).ToNot(HaveOccurred())
Expect(publicKey).To(Equal(""))
})
})

Context("when the settings file does not exist", func() {
BeforeEach(func() {
source = infrastructure.NewFileSettingsSource("/missing-settings-file-path", fs, logger)
})

It("returns an error with the file path", func() {
_, err := source.PublicSSHKeyForUsername("fake-username")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("File for settings source does not exist"))
Expect(err.Error()).To(ContainSubstring("/missing-settings-file-path"))
})
})
})

Expand Down
1 change: 1 addition & 0 deletions infrastructure/multi_settings_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ func (s *MultiSettingsSource) PublicSSHKeyForUsername(username string) (string,
s.selectedSSHKeySource = source
return publicSSHKey, nil
}
s.logger.Warn("multi-settings-source", "Failed to get public SSH key from source: %v", err)
}

return "", bosherr.WrapErrorf(err, "Getting public SSH key for '%s'", username)
Expand Down
10 changes: 10 additions & 0 deletions infrastructure/multi_settings_source_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ var _ = Describe("MultiSettingsSource", func() {
Expect(err).ToNot(HaveOccurred())
Expect(publicKey).To(Equal("fake-public-key-2"))
})

It("logs a warning for the first source", func() {
_, _ = source.PublicSSHKeyForUsername("fake-username") //nolint:errcheck
Expect(logBuffer.String()).To(ContainSubstring("fake-public-key-err-1"))
})
})

Context("when both sources fail to get ssh key", func() {
Expand All @@ -93,6 +98,11 @@ var _ = Describe("MultiSettingsSource", func() {
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("fake-public-key-err-2"))
})

It("logs a warning for the first source", func() {
_, _ = source.PublicSSHKeyForUsername("fake-username") //nolint:errcheck
Expect(logBuffer.String()).To(ContainSubstring("fake-public-key-err-1"))
})
})
})

Expand Down
Loading