Add Config to Disable Hosts File Modification #4938
Conversation
|
Hi @tricktron. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
/retest-required |
|
@praveenkumar Thanks for running the tests. I see many timeout failures unrelated to my change. Is this normal and can I rerun the tests or do only maintainers have the right to rerun tests? |
|
/retest |
tricktron
force-pushed
the
skip-writing-to-hosts-file
branch
from
1244205 to
33f1a93
Compare
WalkthroughAdds a new public config key Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client as Machine Client
participant Config as Config Store
participant Start as Start Flow
participant DNS as DNS Service
participant OS as OS-specific Handler
User->>Client: crc start
Client->>Config: get("modify-hosts-file")
Config-->>Client: bool
Client->>Start: build ServicePostStartConfig(ModifyHostsFile=bool,...)
Start->>DNS: runPostStart(config)
DNS->>OS: runPostStartForOS(config)
alt ModifyHostsFile == true
OS->>OS: addOpenShiftHosts()
OS-->>DNS: success/error
else ModifyHostsFile == false
OS-->>DNS: log "Skipping hosts file modification"
end
DNS-->>Start: return
Start-->>Client: start complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (2)pkg/crc/services/services.go (3)
pkg/crc/config/settings_test.go (1)
🔇 Additional comments (5)
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: 0
🧹 Nitpick comments (3)
pkg/crc/config/settings.go (1)
133-135: Help text: make OS-agnostic to avoid misleading Windows users.Current text mentions “/etc/hosts”, which is Unix-specific. Prefer “system hosts file”.
Apply this diff:
- cfg.AddSetting(ModifyHostsFile, true, ValidateBool, SuccessfullyApplied, - "Allow CRC to modify the /etc/hosts file (true/false, default: true)") + cfg.AddSetting(ModifyHostsFile, true, ValidateBool, SuccessfullyApplied, + "Allow CRC to modify the system hosts file (true/false, default: true)")pkg/crc/services/dns/dns_darwin.go (1)
36-39: Optional: log when skipping hosts modification for visibility.A brief info log helps users correlate behavior with config.
Apply this diff:
- if serviceConfig.ModifyHostsFile { - if err := addOpenShiftHosts(serviceConfig); err != nil { - return err - } - } + if serviceConfig.ModifyHostsFile { + if err := addOpenShiftHosts(serviceConfig); err != nil { + return err + } + } else { + logging.Infof("Skipping hosts file modification on macOS (modify-hosts-file=false)") + }pkg/crc/machine/start.go (1)
506-513: Improve error message when DNS-from-host check fails with modify-hosts-file=false.Make the failure actionable by hinting at manual DNS/hosts setup when the flag is disabled.
You can adjust the message like this (snippet for context):
if err := dns.CheckCRCLocalDNSReachableFromHost(servicePostStartConfig); err != nil { if !client.useVSock() { msg := "Failed to query DNS from host" if !servicePostStartConfig.ModifyHostsFile { msg += " (modify-hosts-file=false). Ensure your system DNS/hosts entries resolve the CRC domains." } return nil, errors.Wrap(err, msg) } logging.Warn(fmt.Sprintf("Failed to query DNS from host: %v", err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/crc/config/settings.go(2 hunks)pkg/crc/config/settings_test.go(2 hunks)pkg/crc/machine/client.go(1 hunks)pkg/crc/machine/start.go(1 hunks)pkg/crc/services/dns/dns_darwin.go(1 hunks)pkg/crc/services/dns/dns_linux.go(1 hunks)pkg/crc/services/dns/dns_windows.go(1 hunks)pkg/crc/services/services.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
pkg/crc/services/services.go (4)
pkg/crc/ssh/ssh.go (1)
Runner(16-18)pkg/crc/machine/bundle/metadata.go (1)
CrcBundleInfo(27-38)pkg/crc/config/settings.go (2)
NetworkMode(23-23)ModifyHostsFile(31-31)pkg/crc/network/types.go (1)
Mode(39-39)
pkg/crc/services/dns/dns_darwin.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/services/dns/dns_windows.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/config/settings_test.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/services/dns/dns_linux.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/machine/client.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/config/settings.go (2)
pkg/crc/config/validations.go (1)
ValidateBool(19-25)pkg/crc/config/callbacks.go (1)
SuccessfullyApplied(36-38)
pkg/crc/machine/start.go (2)
pkg/crc/config/settings.go (2)
NetworkMode(23-23)ModifyHostsFile(31-31)pkg/crc/preflight/labels.go (1)
NetworkMode(13-13)
🔇 Additional comments (8)
pkg/crc/config/settings.go (1)
31-31: LGTM: new setting key introduced cleanly.pkg/crc/services/dns/dns_linux.go (1)
10-14: LGTM: gating host file changes behind the new flag.pkg/crc/machine/client.go (1)
67-69: LGTM: simple accessor for the new setting.pkg/crc/services/dns/dns_windows.go (1)
15-19: LGTM: respects modify-hosts-file on Windows.pkg/crc/config/settings_test.go (2)
254-256: LGTM: default value covered in matrix.
337-339: LGTM: provided (false) value covered in matrix.pkg/crc/machine/start.go (1)
484-487: LGTM: value threaded into ServicePostStartConfig.pkg/crc/services/services.go (1)
10-15: LGTM: config plumbed via ServicePostStartConfig — no unkeyed struct literals found. Call sites in pkg/crc/machine/start.go and pkg/crc/services/dns/dns_test.go use keyed initializers.
gbraad
left a comment
gbraad
left a comment
There was a problem hiding this comment.
Suggest minor changes to ensure this is recorded as part of log
| if serviceConfig.ModifyHostsFile { | ||
| if err := addOpenShiftHosts(serviceConfig); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
- }
+ } else {
+ logging.Infof("Skipping hosts file modification")
+ }There was a problem hiding this comment.
... this of course applies to all the platforms (also dns_linux.go and dns_windows.go).
There was a problem hiding this comment.
@gbraad Thank you for your time and the review. I applied your suggestions and also the other two nitpicks by coderabbitai.
tricktron
force-pushed
the
skip-writing-to-hosts-file
branch
from
33f1a93 to
4bac765
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tricktron
force-pushed
the
skip-writing-to-hosts-file
branch
from
4bac765 to
6b59ec8
Compare
|
@gbraad Kind ping to have another look 😃. |
|
@gbraad Another friendly ping to have look 😃. |
praveenkumar
left a comment
praveenkumar
left a comment
There was a problem hiding this comment.
Changes regarding file permission change should be in separate commit.
tricktron
force-pushed
the
skip-writing-to-hosts-file
branch
from
6b59ec8 to
b810d46
Compare
@praveenkumar I created a separate commit for it 👍. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/crc/services/dns/dns_linux.go (1)
3-17: Config‑driven skip behavior is correct; consider clarifying log messageThe conditional around
addOpenShiftHostscorrectly implements theModifyHostsFiletoggle and preserves previous behavior when the flag is true. When false, returningnilafter logging is appropriate.For debuggability, you might make the log slightly more explicit that the skip is configuration‑driven, e.g.:
- if serviceConfig.ModifyHostsFile { - return addOpenShiftHosts(serviceConfig) - } else { - logging.Infof("Skipping hosts file modification") - } + if serviceConfig.ModifyHostsFile { + return addOpenShiftHosts(serviceConfig) + } + + logging.Infof("Skipping hosts file modification because 'modify-hosts-file' is set to false")pkg/crc/services/dns/dns_windows.go (1)
3-23: Windows hosts‑file toggle logic is correct; mirror clearer log messageThe guard on
NetworkModeremains intact, and the newModifyHostsFilecheck correctly makes hosts‑file updates optional while keeping prior behavior when true. Returningnilon the skip path is reasonable.As with the Linux implementation, you could clarify the log to indicate that the skip is due to configuration, e.g.:
- if serviceConfig.ModifyHostsFile { - return addOpenShiftHosts(serviceConfig) - } else { - logging.Infof("Skipping hosts file modification") - } + if serviceConfig.ModifyHostsFile { + return addOpenShiftHosts(serviceConfig) + } + + logging.Infof("Skipping hosts file modification because 'modify-hosts-file' is set to false")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/crc/config/settings.go(2 hunks)pkg/crc/config/settings_test.go(2 hunks)pkg/crc/machine/client.go(1 hunks)pkg/crc/machine/start.go(7 hunks)pkg/crc/services/dns/dns_darwin.go(1 hunks)pkg/crc/services/dns/dns_linux.go(1 hunks)pkg/crc/services/dns/dns_windows.go(2 hunks)pkg/crc/services/services.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/crc/services/dns/dns_darwin.go
- pkg/crc/config/settings.go
- pkg/crc/config/settings_test.go
- pkg/crc/machine/start.go
- pkg/crc/machine/client.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/crc/services/dns/dns_linux.go (2)
pkg/crc/services/services.go (1)
ServicePostStartConfig(9-16)pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
pkg/crc/services/services.go (5)
pkg/crc/oc/oc.go (1)
SSHRunner(74-76)pkg/crc/ssh/ssh.go (1)
Runner(16-18)pkg/crc/machine/bundle/metadata.go (1)
CrcBundleInfo(27-38)pkg/crc/config/settings.go (2)
NetworkMode(23-23)ModifyHostsFile(31-31)pkg/crc/network/types.go (1)
Mode(39-39)
pkg/crc/services/dns/dns_windows.go (1)
pkg/crc/config/settings.go (1)
ModifyHostsFile(31-31)
🔇 Additional comments (1)
pkg/crc/services/services.go (1)
9-15: ServicePostStartConfig extension withModifyHostsFilelooks consistentAdding
ModifyHostsFile boolis an additive, zero-value-safe change and fits the existing config aggregation pattern. No issues from an API or correctness perspective as long as callers initialize it via the new setting as done elsewhere in the PR.
Convert legacy octal notation (0600, 0644) to modern Go style with explicit 0o prefix (0o600, 0o644) for improved code clarity.
This change introduces a new configuration setting 'modify-hosts-file' that allows users to disable CRC's automatic modification of the /etc/hosts file. The setting defaults to true to maintain backward compatibility with existing installations.
tricktron
force-pushed
the
skip-writing-to-hosts-file
branch
from
b810d46 to
34341be
Compare
|
@tricktron: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Description
This change introduces a new configuration setting
modify-hosts-filethat allows users to disable CRC's automatic modification of the /etc/hosts file. The setting defaults to true to maintain backward compatibility with existing installations.When set to false, CRC will skip adding OpenShift hostnames to the system's hosts file during cluster startup. This is particularly useful for systems like NixOS where the hosts file is managed declaratively and cannot be modified by applications, or for users who prefer to manage DNS resolution through other means.
The implementation adds the configuration option to all platform-specific DNS modules (Linux, macOS, Windows) and includes the setting in the service configuration passed to DNS setup routines.
Type of change
test, version modification, documentation, etc.)
Proposed changes
Testing
I tested this e2e on my NixOS machine by manually setting the needed host file entries:
api.crc.testing, canary-openshift-ingress-canary.apps-crc.testing, ...and with the new configmodify-hosts-file = false.Contribution Checklist
Summary by CodeRabbit
New Features
Documentation / Messaging
Tests
✏️ Tip: You can customize this high-level summary in your review settings.