File Settings should return error in case the file itself does not exist #437
File Settings should return error in case the file itself does not exist #437fmoehler wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughPublicSSHKeyForUsername now checks whether the settings file exists via s.fs.FileExists and returns an error "Settings file '' does not exist" if missing; otherwise it continues to return the previous result. Tests were split into contexts covering the settings-file-present and settings-file-missing cases. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@infrastructure/file_settings_source.go`:
- Line 38: Update the spec in infrastructure/file_settings_source_test.go to
expect an error from the File Setting Source call that previously returned an
SSH key: change the assertion that currently expects err == nil to assert err !=
nil (and optionally assert the error message contains "does not provide ssh key"
or matches bosherr.Error output). Locate the test that invokes the File Setting
Source method (the call that previously returned the SSH key) and replace the
nil-error expectation with a non-nil assertion and corresponding message check
so the test verifies the new error contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c99ca351-1668-46b8-ae45-62925b316c43
📒 Files selected for processing (1)
infrastructure/file_settings_source.go
fmoehler
force-pushed
the
file-setting-should-return-error-for-ssh-key
branch
from
a2f5286 to
0b2bbfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
infrastructure/file_settings_source_test.go (1)
29-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest expects incorrect behavior.
This test expects
PublicSSHKeyForUsernameto return no error when the settings file exists (line 39), but that contradicts the PR objective. The file settings source should always return an error when an SSH key is requested (because it doesn't provide SSH keys), allowing the agent to fall back to other sources. The past review comment flagged this same issue and was marked as addressed, yet the test still validates the wrong contract.Once the implementation is corrected to always return an error, this test case should be removed or replaced with a single test that verifies an error is always returned.
🧪 Proposed fix to test the correct behavior
Replace the two context blocks with a single test that verifies an error is always returned:
Describe("PublicSSHKeyForUsername", func() { - 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", func() { - _, err := source.PublicSSHKeyForUsername("fake-username") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("File for settings source does not exist")) - }) + BeforeEach(func() { + source = infrastructure.NewFileSettingsSource("/fake-settings-file-path", fs, logger) + }) + + It("always returns an error because file settings source does not provide SSH keys", func() { + _, err := source.PublicSSHKeyForUsername("fake-username") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("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_test.go` around lines 29 - 42, The test is asserting the wrong contract: FileSettingsSource should always return an error for SSH key requests. Update the spec around infrastructure.NewFileSettingsSource / PublicSSHKeyForUsername so it no longer expects a successful call; remove the existing "when the settings file exists" block and replace it with a single test that calls source.PublicSSHKeyForUsername("fake-username") and asserts that an error is returned (and optionally that the returned key is empty), ensuring the test matches the intended behavior that FileSettingsSource never supplies SSH keys.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@infrastructure/file_settings_source.go`:
- Around line 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.
---
Duplicate comments:
In `@infrastructure/file_settings_source_test.go`:
- Around line 29-42: The test is asserting the wrong contract:
FileSettingsSource should always return an error for SSH key requests. Update
the spec around infrastructure.NewFileSettingsSource / PublicSSHKeyForUsername
so it no longer expects a successful call; remove the existing "when the
settings file exists" block and replace it with a single test that calls
source.PublicSSHKeyForUsername("fake-username") and asserts that an error is
returned (and optionally that the returned key is empty), ensuring the test
matches the intended behavior that FileSettingsSource never supplies SSH keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 744de521-026a-4922-9f7e-a576da679b61
📒 Files selected for processing (2)
infrastructure/file_settings_source.goinfrastructure/file_settings_source_test.go
| 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 | ||
| } |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
🧩 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 -A10Length 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.
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
fmoehler
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the FileSettingsSource SSH key lookup behavior so that it returns an error when its settings file is missing, allowing MultiSettingsSource to fall back to subsequent sources instead of prematurely “succeeding” with an empty SSH key.
Changes:
- Add a settings-file existence check in
FileSettingsSource.PublicSSHKeyForUsernameand return an error when missing. - Extend
FileSettingsSourceunit tests to cover both “file exists” and “file missing” cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
infrastructure/file_settings_source.go |
Return an error from SSH key lookup when the settings file path doesn’t exist, enabling fallback to other sources. |
infrastructure/file_settings_source_test.go |
Add test coverage for the new “missing settings file returns error” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rkoster
requested review from
a team,
s4heid and
selzoc
and removed request for
a team
|
Seems reasonable to me - probably worth addressing some of the ai-generated comments. Do any other Iaases use the |
fmoehler
force-pushed
the
file-setting-should-return-error-for-ssh-key
branch
from
d07f176 to
0b2bbfb
Compare
- Include settings file path in PublicSSHKeyForUsername error message for easier debugging - Add warning logs in MultiSettingsSource when SSH key retrieval fails from a source - Update tests to verify error message includes file path and logging behavior
|
Hi @selzoc , |
|
Does this PR change the behavior for the Azure case? |
|
Hi @selzoc , |
The agent loops over the settings sources defined in the stemcell. E.g. here for openstack when trying to retrieve the default ssh key here. In case of an error the agent tries the next source. The file setting source, however, does not return an SSH Key at all, but also does not error out, therefore blocking the agent from trying other sources for the SSH key retrieval. Therefore, the file setting source should return an error when an ssh key is trying to be fetched, so that the agent can fall back to other settings sources for the ssh key retrieval.