USHIFT-6902 USHIFT-6903: Custom DNS configuration#6592
Open
pacevedom wants to merge 6 commits intoopenshift:mainfrom
Open
USHIFT-6902 USHIFT-6903: Custom DNS configuration#6592pacevedom wants to merge 6 commits intoopenshift:mainfrom
pacevedom wants to merge 6 commits intoopenshift:mainfrom
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for a user-supplied CoreDNS Corefile, validates the config, updates DNS controller startup to accept a custom Corefile, introduces a DNSConfigurationWatcherManager that watches the Corefile on disk and keeps the Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as User/Admin
participant FS as Filesystem
participant Watcher as DNSConfigurationWatcherManager
participant K8s as Kubernetes API
participant CM as ConfigMap (openshift-dns/dns-default)
participant CoreDNS as CoreDNS Pod
Admin->>FS: Write or update Corefile
FS-->>Watcher: fsnotify event (Create/Write/Rename/Remove)
Watcher->>FS: Read Corefile contents
Watcher->>Watcher: Compute SHA-256 of contents
alt hash changed
Watcher->>K8s: Create/Update ConfigMap with Corefile, labels, annotations
K8s->>CM: Apply ConfigMap
CM->>CoreDNS: ConfigMap change triggers reload
CoreDNS->>CoreDNS: Reload Corefile
else hash unchanged
Watcher->>Watcher: Skip update (no-op)
end
Note right of Watcher: Loop until context cancellation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
Contributor
Author
|
/test ? |
Contributor
Author
|
/test e2e-aws-tests |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
test/suites/standard1/dns-custom-config.robot (1)
134-137:Remove Fake Hosts Entryis a no-op.The keyword only logs. The runtime-reload test calls it for
${updated_hostname}in teardown, but the actual cleanup happens viaTeardown Custom Corefile. Either inline the comment in the test teardown or remove the keyword to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` around lines 134 - 137, The keyword Remove Fake Hosts Entry is a no-op that only logs and duplicates cleanup already performed by Teardown Custom Corefile; remove the Remove Fake Hosts Entry keyword from the suite (or replace its usage) and update the test teardown that calls Remove Fake Hosts Entry for ${updated_hostname} to either call Teardown Custom Corefile directly or inline a short comment in the test teardown explaining the cleanup is handled by Teardown Custom Corefile so there is no separate hostname removal step.pkg/controllers/dnsconfigurationwatcher.go (3)
41-48: Disabled-mode shutdown semantics.When
s.file == ""youclose(ready)andreturn ctx.Err().ctx.Err()isniluntil cancellation, soRunwill block indefinitely without a<-ctx.Done()here. Looks intentional (so the service stays "running"), but worth a<-ctx.Done()to make the wait explicit and to avoidreturn nilbeing mistaken for "completed immediately" by future readers.♻️ Suggested clarification
if s.file == "" { klog.Infof("%s is disabled (not configured)", s.Name()) close(ready) - return ctx.Err() + <-ctx.Done() + return ctx.Err() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 41 - 48, When s.file == "" in DNSConfigurationWatcherManager.Run you currently close(ready) and return ctx.Err() immediately, but ctx.Err() is nil until cancellation; change the disabled-path to explicitly wait for context cancellation by performing a <-ctx.Done() (or select on ctx.Done()) after close(ready) and before returning, then return ctx.Err(); this makes the disabled-mode shutdown semantics explicit and avoids a misleading immediate return value.
175-182: No conflict-retry onUpdate.
assets.ApplyConfigMapWithData(used bypkg/components/controllers.go) wraps writes inretry.RetryOnConflict. The watcher writes the same object without retry, so a benign 409 just fails this iteration. Wrapping the Get+Update inretry.RetryOnConflict(retry.DefaultBackoff, …)would match the existing convention in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 175 - 182, The updateConfigMap flow can fail on benign 409 conflicts because the Get+Update path in createOrUpdateConfigMap isn't wrapped in a conflict retry; wrap the update logic used by updateConfigMap/createOrUpdateConfigMap in retry.RetryOnConflict(retry.DefaultBackoff, func() error { ... }) (from k8s.io/apimachinery/pkg/util/retry) so transient resourceVersion conflicts are retried and the final error is returned; locate the Get+Update sequence in createOrUpdateConfigMap and wrap it with RetryOnConflict, returning its error from updateConfigMap.
109-111: Doublewatcher.Close()on shutdown.
deferat lines 66–70 already closes the watcher; returningwatcher.Close()here invokes it again. fsnotify tolerates it (returns nil on the second call), butreturn nilis clearer and avoids logging a misleading close error if behavior ever changes upstream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 109 - 111, The shutdown branch in the select that triggers on ctx.Done() calls watcher.Close() again causing a double close; replace the current return watcher.Close() with return nil in the ctx.Done() case so the deferred close at the top of the function handles cleanup and the method simply returns after logging via klog.Infof("%s stopping", s.Name()); keep references to the watcher variable, the select case listening on ctx.Done(), and s.Name() so you modify the correct block.pkg/config/dns.go (3)
99-105: Use a named constant (or1<<20) for the 1 MiB size limit.
1048576is repeated in bothvalidateConfigFileandvalidateHosts. A singleconst dnsConfigMapMaxSize = 1 << 20would document intent and avoid drift if the limit ever changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns.go` around lines 99 - 105, Replace the repeated literal 1048576 with a named constant to document intent and prevent drift: declare a const dnsConfigMapMaxSize = 1 << 20 (or equivalent) near the top of the file and use it in the size checks inside validateConfigFile and validateHosts where the current comparisons use 1048576; update the error messages to reference the constant value if needed so both functions consistently use dnsConfigMapMaxSize.
87-90: UsecleanPathconsistently in errors.You compute
cleanPathand validate against it, but the error messages echo back the originalt.ConfigFile. If the user supplies/etc/microshift/../microshift/dns/Corefile, the message will keep the unclean form. Minor — pick one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns.go` around lines 87 - 90, The validation builds cleanPath with filepath.Clean but the error returns the original t.ConfigFile; update the error (and any related messages) to reference cleanPath instead so the normalized path is shown to the user—look for the filepath.Clean(t.ConfigFile) usage and the subsequent IsAbs check in pkg/config/dns.go and change occurrences of t.ConfigFile in the error string to cleanPath.
107-111: Open-then-close as a readability probe.Returning
file.Close()directly means a transient close error (rare) becomes a startup-blocking validation error. Functionally fine, butos.OpenFile(cleanPath, os.O_RDONLY, 0)followed by an explicit deferred close +return nilwould be a touch cleaner. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns.go` around lines 107 - 111, Change the one-shot open-and-return-close pattern to open the file for read-only with os.OpenFile(cleanPath, os.O_RDONLY, 0), check the error as currently done, then defer file.Close() and return nil so a rare transient Close() error won't be treated as a validation failure; update the block that uses variables file, cleanPath and t.ConfigFile accordingly (open, error check, defer close, return nil).pkg/config/dns_test.go (2)
12-110: Missing test for the readability branch.
validateConfigFilehas a dedicatedos.Openstep (lines 107–111) for permission errors, but no test exercises it. A quickos.Chmod(tmpFile, 0o000)case (skipped when running as root) would close that gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns_test.go` around lines 12 - 110, Add a test that exercises the permission-readability branch of validateConfigFile by creating a temp Corefile (use createTempCorefile or tmp file), os.Chmod the file to 0o000 to remove read permissions, skip the test when running as root, instantiate DNS with ConfigFile set to that path and Hosts.Status disabled, call dns.validate() and assert it returns an error containing a permission/readability message; ensure you clean up/reset file mode after the test. Reference the validateConfigFile path exercised by DNS.validate / validateConfigFile so the test hits the os.Open permission branch.
59-68: Hardcoded/tmppath is a very minor flake risk.
/tmp/nonexistent-corefile-test-12345could (in theory) exist on a shared host. Preferfilepath.Join(t.TempDir(), "nonexistent")so the test is hermetic.♻️ Suggested change
func TestDNS_ValidateConfigFile_NonExistentFile(t *testing.T) { dns := DNS{ - ConfigFile: "/tmp/nonexistent-corefile-test-12345", + ConfigFile: filepath.Join(t.TempDir(), "nonexistent-corefile"), Hosts: HostsConfig{ Status: HostsStatusDisabled, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns_test.go` around lines 59 - 68, TestDNS_ValidateConfigFile_NonExistentFile uses a hardcoded "/tmp/..." path which can flake; update the test to create a unique temp directory with t.TempDir() and set DNS.ConfigFile to filepath.Join(t.TempDir(), "nonexistent-corefile-test-12345") (or similar) before calling dns.validate() so the file is guaranteed not to exist and the test is hermetic; keep assertions the same and import "path/filepath" if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/components/controllers.go`:
- Around line 337-346: Before calling os.ReadFile on cfg.DNS.ConfigFile, stat
the file and re-validate its size against the ConfigMap 1MiB limit (1048576
bytes) to guard against growth since dns.validate(); if the stat shows the file
is too large, return a clear error instead of reading (and avoid passing
oversized data to assets.ApplyConfigMapWithData), and consider factoring this
check into a shared helper used by both the watcher and the code path that reads
cfg.DNS.ConfigFile; reference cfg.DNS.ConfigFile, dns.validate(), os.ReadFile,
and assets.ApplyConfigMapWithData when locating where to add the stat+size check
and error return.
- Around line 337-352: The startup apply in controllers.go (the branch using
assets.ApplyConfigMapWithData / cm) and the DNSConfigurationWatcherManager's
createOrUpdateConfigMap are clobbering each other's annotations/labels; fix by
consolidating ownership: either skip the initial apply when cfg.DNS.ConfigFile
!= "" and let DNSConfigurationWatcherManager own the openshift-dns/dns-default
ConfigMap, or (preferred minimal change) change createOrUpdateConfigMap to merge
labels and annotations into the existing ConfigMap instead of doing
existing.Annotations = configMap.Annotations / replacing Labels — i.e., read
existing := get, set existing.Labels/Annotations = merge(existing, configMap)
(preserve keys from both, allow watcher to add/overwrite its keys) and then
update; reference functions: ApplyConfigMapWithData, assets.ApplyConfigMaps,
DNSConfigurationWatcherManager, createOrUpdateConfigMap, and cfg.DNS.ConfigFile
when making the change.
In `@pkg/controllers/dnsconfigurationwatcher.go`:
- Around line 56-59: The TOCTOU arises because s.updateConfigMap runs before
hashing, so a file change between updateConfigMap and getFileHash can make
lastHash reflect new content while the ConfigMap contains old content; fix by
calling s.getFileHash(ctx) first and assign s.lastHash (or record it
appropriately), then call s.updateConfigMap(ctx, kubeClient), then
s.setupWatches(...)/close ready — i.e., reorder to getFileHash -> set lastHash
-> updateConfigMap -> setupWatches (or alternatively ensure the first watch
event always forces a reconcile if reordering is undesirable).
- Around line 137-142: The current isRelevantEvent (in
DNSConfigurationWatcherManager) only accepts Write/Create so it loses the
per-file watch after editors do atomic rename/remove; update isRelevantEvent to
also treat fsnotify.Rename and fsnotify.Remove as relevant, and in the eventLoop
handler for events on s.file, when you observe Rename or Remove call
watcher.Remove if needed and mark the per-file watcher as stale, and on
subsequent Create (or immediately after Rename) call watcher.Add(s.file) to
re-add the file watcher before reconciling; reference the
DNSConfigurationWatcherManager.isRelevantEvent and the eventLoop code where
watcher.Add(s.file) / watcher.Remove should be invoked.
- Around line 204-210: The current code treats any error from
configMapsClient.Get as "not found" and always calls configMapsClient.Create;
instead check the error using apierrors.IsNotFound(err) inside the block that
handles Get errors: if apierrors.IsNotFound(err) then attempt
configMapsClient.Create(ctx, configMap, metav1.CreateOptions{}) and log using
s.Name(), dnsConfigMapNamespace, dnsConfigMapName on success, otherwise return
the original err immediately (do not fall through to Create) so
RBAC/throttling/API errors are propagated; keep handling of create errors (wrap
with fmt.Errorf) as before.
In `@test/suites/standard1/dns-custom-config.robot`:
- Around line 40-50: The test "Runtime Reload Without MicroShift Restart" is
flaky due to slow propagation (file → fsnotify → ConfigMap → kubelet sync →
CoreDNS reload); add a fast intermediate assertion to verify the Corefile
ConfigMap has been updated before calling Resolve Host From Pod. Specifically,
after Update Corefile With New Host and before Resolve Host From Pod use a
keyword to fetch and assert the ConfigMap content (the ConfigMap updated by
Setup Custom Corefile With Hosts Entry / Update Corefile With New Host) or poll
the ConfigMap until it contains the new hostname, or alternatively lower the
kubelet projected-volume sync period for the test run so the kubelet sync
(default ~60s) won’t cause flakiness.
- Around line 113-117: In the "Router Should Resolve Hostname" test, the Should
Contain line splits the expected substring into two positional args so
${hostname} becomes the failure message; update the assertion to pass the
expected substring as a single argument (e.g., combine "Name:" and ${hostname}
into one item) so the call reads something like Should Contain ${output}
Name: ${hostname}; edit the Should Contain line in that test (near the Oc Exec
call) to ensure the item is a single parameter rather than two separate tokens.
---
Nitpick comments:
In `@pkg/config/dns_test.go`:
- Around line 12-110: Add a test that exercises the permission-readability
branch of validateConfigFile by creating a temp Corefile (use createTempCorefile
or tmp file), os.Chmod the file to 0o000 to remove read permissions, skip the
test when running as root, instantiate DNS with ConfigFile set to that path and
Hosts.Status disabled, call dns.validate() and assert it returns an error
containing a permission/readability message; ensure you clean up/reset file mode
after the test. Reference the validateConfigFile path exercised by DNS.validate
/ validateConfigFile so the test hits the os.Open permission branch.
- Around line 59-68: TestDNS_ValidateConfigFile_NonExistentFile uses a hardcoded
"/tmp/..." path which can flake; update the test to create a unique temp
directory with t.TempDir() and set DNS.ConfigFile to filepath.Join(t.TempDir(),
"nonexistent-corefile-test-12345") (or similar) before calling dns.validate() so
the file is guaranteed not to exist and the test is hermetic; keep assertions
the same and import "path/filepath" if needed.
In `@pkg/config/dns.go`:
- Around line 99-105: Replace the repeated literal 1048576 with a named constant
to document intent and prevent drift: declare a const dnsConfigMapMaxSize = 1 <<
20 (or equivalent) near the top of the file and use it in the size checks inside
validateConfigFile and validateHosts where the current comparisons use 1048576;
update the error messages to reference the constant value if needed so both
functions consistently use dnsConfigMapMaxSize.
- Around line 87-90: The validation builds cleanPath with filepath.Clean but the
error returns the original t.ConfigFile; update the error (and any related
messages) to reference cleanPath instead so the normalized path is shown to the
user—look for the filepath.Clean(t.ConfigFile) usage and the subsequent IsAbs
check in pkg/config/dns.go and change occurrences of t.ConfigFile in the error
string to cleanPath.
- Around line 107-111: Change the one-shot open-and-return-close pattern to open
the file for read-only with os.OpenFile(cleanPath, os.O_RDONLY, 0), check the
error as currently done, then defer file.Close() and return nil so a rare
transient Close() error won't be treated as a validation failure; update the
block that uses variables file, cleanPath and t.ConfigFile accordingly (open,
error check, defer close, return nil).
In `@pkg/controllers/dnsconfigurationwatcher.go`:
- Around line 41-48: When s.file == "" in DNSConfigurationWatcherManager.Run you
currently close(ready) and return ctx.Err() immediately, but ctx.Err() is nil
until cancellation; change the disabled-path to explicitly wait for context
cancellation by performing a <-ctx.Done() (or select on ctx.Done()) after
close(ready) and before returning, then return ctx.Err(); this makes the
disabled-mode shutdown semantics explicit and avoids a misleading immediate
return value.
- Around line 175-182: The updateConfigMap flow can fail on benign 409 conflicts
because the Get+Update path in createOrUpdateConfigMap isn't wrapped in a
conflict retry; wrap the update logic used by
updateConfigMap/createOrUpdateConfigMap in
retry.RetryOnConflict(retry.DefaultBackoff, func() error { ... }) (from
k8s.io/apimachinery/pkg/util/retry) so transient resourceVersion conflicts are
retried and the final error is returned; locate the Get+Update sequence in
createOrUpdateConfigMap and wrap it with RetryOnConflict, returning its error
from updateConfigMap.
- Around line 109-111: The shutdown branch in the select that triggers on
ctx.Done() calls watcher.Close() again causing a double close; replace the
current return watcher.Close() with return nil in the ctx.Done() case so the
deferred close at the top of the function handles cleanup and the method simply
returns after logging via klog.Infof("%s stopping", s.Name()); keep references
to the watcher variable, the select case listening on ctx.Done(), and s.Name()
so you modify the correct block.
In `@test/suites/standard1/dns-custom-config.robot`:
- Around line 134-137: The keyword Remove Fake Hosts Entry is a no-op that only
logs and duplicates cleanup already performed by Teardown Custom Corefile;
remove the Remove Fake Hosts Entry keyword from the suite (or replace its usage)
and update the test teardown that calls Remove Fake Hosts Entry for
${updated_hostname} to either call Teardown Custom Corefile directly or inline a
short comment in the test teardown explaining the cleanup is handled by Teardown
Custom Corefile so there is no separate hostname removal step.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9a68241a-4489-42f2-8ce4-1c42a7a1bf76
📒 Files selected for processing (7)
pkg/cmd/run.gopkg/components/controllers.gopkg/config/dns.gopkg/config/dns_test.gopkg/controllers/dnsconfigurationwatcher.gotest/assets/dns/Corefile.templatetest/suites/standard1/dns-custom-config.robot
Contributor
Author
|
/test e2e-aws-tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/suites/standard1/dns-custom-config.robot (1)
144-149:rm -frc check is effectively a no-op.
rm -fswallows missing-file errors and returns 0 in virtually all real cases, so the assertion adds noise without catching anything. Either drop it or switch torm(without-f) so a genuine permission/path issue actually surfaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` around lines 144 - 149, The test step "Remove Custom Corefile" uses "rm -f ${CUSTOM_COREFILE_PATH}" which makes the subsequent rc assertion meaningless; either remove the rc/assertion entirely from that test or change the command to "rm ${CUSTOM_COREFILE_PATH}" so failures bubble up; update the Execute Command invocation (the Robot keyword call that sets ${stdout} ${stderr} ${rc}= Execute Command ...) and the Should Be Equal As Integers assertion accordingly (referencing ${rc}), and keep ${CUSTOM_COREFILE_PATH} as the target variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/standard1/dns-custom-config.robot`:
- Line 41: The test "Verify ConfigMap Uses Custom Corefile" runs immediately
after "Restart MicroShift" and can read a stale/default Corefile because
DNSConfigurationWatcherManager reconciles the dns-default ConfigMap
asynchronously; update the test to wrap the ConfigMap assertion in Robot's "Wait
Until Keyword Succeeds" (same pattern used by "Resolve Host From Pod") so it
will retry until the custom Corefile appears, referencing the "Verify ConfigMap
Uses Custom Corefile" keyword and using the retry/interval values consistent
with other waits in the suite.
---
Nitpick comments:
In `@test/suites/standard1/dns-custom-config.robot`:
- Around line 144-149: The test step "Remove Custom Corefile" uses "rm -f
${CUSTOM_COREFILE_PATH}" which makes the subsequent rc assertion meaningless;
either remove the rc/assertion entirely from that test or change the command to
"rm ${CUSTOM_COREFILE_PATH}" so failures bubble up; update the Execute Command
invocation (the Robot keyword call that sets ${stdout} ${stderr} ${rc}= Execute
Command ...) and the Should Be Equal As Integers assertion accordingly
(referencing ${rc}), and keep ${CUSTOM_COREFILE_PATH} as the target variable.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 681fff44-1e28-4b73-a2d9-f7e7f8b020e0
📒 Files selected for processing (2)
test/assets/dns/Corefile.templatetest/suites/standard1/dns-custom-config.robot
✅ Files skipped from review due to trivial changes (1)
- test/assets/dns/Corefile.template
Contributor
Author
|
/test e2e-aws-tests |
Contributor
Author
|
/test e2e-aws-tests |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/suites/standard1/dns-custom-config.robot`:
- Line 20: The Corefile template path is working-directory dependent; update the
assignment of ${COREFILE_TEMPLATE} to use the suite-relative path
"test/assets/dns/Corefile.template" instead of "./assets/dns/Corefile.template"
so the test consistently finds the template (locate where ${COREFILE_TEMPLATE}
is set in dns-custom-config.robot and replace the path).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1c7b7cd2-f47c-4400-b6f7-d69ac30925c6
📒 Files selected for processing (2)
test/assets/dns/Corefile.templatetest/suites/standard1/dns-custom-config.robot
✅ Files skipped from review due to trivial changes (1)
- test/assets/dns/Corefile.template
Contributor
Author
|
/test e2e-aws-tests |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
test/suites/standard1/dns-custom-config.robot (4)
44-45:⚠️ Potential issue | 🟡 MinorAssert the ConfigMap update before the nslookup retry.
Right after
Update Corefile With New Host, failures are ambiguous: the watcher may not have synced yet, or CoreDNS may not have reloaded yet. Pollingdns-default.data.Corefilefor${updated_hostname}here makes this test much less flaky.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` around lines 44 - 45, After the "Update Corefile With New Host" step, add a poll/assert that the ConfigMap key dns-default.data.Corefile contains ${updated_hostname} before running "Resolve Host From Pod"; implement this by polling the ConfigMap (e.g., a retry loop or Robot's Wait Until Keyword Succeeds) until the Corefile text includes ${updated_hostname} and fail if the timeout is reached so the test only proceeds once the watcher/CoreDNS reload has actually observed the change.
87-92:⚠️ Potential issue | 🟡 MinorMake the ConfigMap check retryable.
This keyword reads
dns-defaultonce, but the watcher updates it asynchronously afterRestart MicroShift. Wrapping the assertion inWait Until Keyword Succeedswill avoid reading the stale/default Corefile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` around lines 87 - 92, The test reads the dns-default ConfigMap once with the keyword Oc Get JsonPath and asserts via Should Contain, but this can race with asynchronous updates after Restart MicroShift; update the "Verify ConfigMap Uses Custom Corefile" case to retry the read/assert by wrapping the Oc Get JsonPath + Should Contain sequence inside a Wait Until Keyword Succeeds (or equivalent retry) block so it re-reads ${corefile_data} until the ${HOSTNAME} appears, keeping the existing keywords names (Oc Get JsonPath, Should Contain, Wait Until Keyword Succeeds) and the test name intact.
103-104:⚠️ Potential issue | 🟡 MinorFix the
Should Containitem argument.
${hostname}is currently the failure message, not part of the expected substring, so this passes for any successful lookup.Proposed fix
- Should Contain ${output} Name: ${hostname} + Should Contain ${output} Name: ${hostname}What is the Robot Framework BuiltIn `Should Contain` keyword signature, and how are positional arguments interpreted?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` around lines 103 - 104, The test calls BuiltIn's Should Contain with ${output}, "Name:" and ${hostname} which makes ${hostname} the custom failure message instead of part of the expected substring; update the Should Contain invocation (after the Oc Exec that sets ${output}) to pass the full expected substring as the second positional argument — e.g. combine the tokens into one expected string like "Name: ${hostname}" (so call Should Contain ${output} Name: ${hostname}) so Robot treats the whole "Name: <hostname>" as the substring to look for.
20-20:⚠️ Potential issue | 🟠 MajorUse a suite-relative Corefile template path.
./assets/dns/Corefile.templateis working-directory dependent. This suite lives undertest/suites/standard1, so the current path will misstest/assets/dns/Corefile.templatein many runners.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/standard1/dns-custom-config.robot` at line 20, The path assigned to ${COREFILE_TEMPLATE} is relative to the working directory and should be suite-relative; update the value in dns-custom-config.robot (the ${COREFILE_TEMPLATE} assignment) from ./assets/dns/Corefile.template to the suite-relative path ../../assets/dns/Corefile.template (or use a suite-dir variable if preferred) so it resolves to test/assets/dns/Corefile.template when the suite runs.pkg/components/controllers.go (1)
337-343:⚠️ Potential issue | 🟡 MinorRe-check the Corefile size right before reading it.
dns.validate()runs earlier, but the file can grow before thisos.ReadFile. If it exceeds the 1 MiB ConfigMap limit here, the failure moves to the API write path and becomes much less clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/components/controllers.go` around lines 337 - 343, Before reading the DNS Corefile, re-check its size to ensure it does not exceed the 1 MiB ConfigMap limit; specifically, in the branch handling cfg.DNS.ConfigFile (the block that currently calls os.ReadFile and then assets.ApplyConfigMapWithData with cm and kubeconfigPath), stat the file (os.Stat) and return a clear error if the size is > 1*1024*1024 so the check catches growth that occurred after dns.validate() and prevents a confusing API write failure when calling assets.ApplyConfigMapWithData.pkg/controllers/dnsconfigurationwatcher.go (4)
56-59:⚠️ Potential issue | 🟡 MinorHash/write ordering still has a missed-update window.
If the file changes after the initial
updateConfigMap()but beforegetFileHash(),lastHashcan reflect new content while the ConfigMap still has old content, and the next event gets skipped as “unchanged”.Also applies to: 77-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 56 - 59, The race comes from calling updateConfigMap before computing/setting the hash, allowing the file to change between update and getFileHash and causing missed updates; fix by computing the file hash with getFileHash() first, use that hash to decide whether to update, pass or apply that same computed hash when calling updateConfigMap (or store it as the ConfigMap annotation) and then set lastHash = computedHash only after a successful update; apply the same change in both places that currently update the ConfigMap (the initial creation path using updateConfigMap(...) and the subsequent update path around the existing getFileHash/lastHash checks).
204-210:⚠️ Potential issue | 🔴 CriticalDon’t treat every
Getfailure as “not found”.Transient API/RBAC/throttling errors will fall into
Create()here and mask the real problem. This needs anapierrors.IsNotFound(err)split, with all otherGeterrors returned directly.In Kubernetes client-go, should errors from `ConfigMaps().Get(...)` be distinguished with `apierrors.IsNotFound(err)` instead of treating any error as a create path?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 204 - 210, The Get error must not be assumed to mean "not found"; update the logic around configMapsClient.Get(ctx, dnsConfigMapName, metav1.GetOptions{}) to check apierrors.IsNotFound(err) and only call configMapsClient.Create(...) in that branch; for any other error from Get (e.g. RBAC/throttling), return the error immediately (wrap with fmt.Errorf as appropriate). Ensure you reference the existing variables and functions (configMapsClient, dnsConfigMapName, configMap, s.Name()) and keep the success log (klog.Infof) only after a successful Create.
212-214:⚠️ Potential issue | 🟠 MajorMerge annotations instead of replacing them.
existing.Annotations = configMap.Annotationsdrops any metadata already present onopenshift-dns/dns-default, including annotations coming from the startup asset apply path. That leaves the final shape dependent on whichever writer ran last.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 212 - 214, The code currently replaces annotations via existing.Annotations = configMap.Annotations which drops preexisting metadata; instead merge configMap.Annotations into existing.Annotations before calling configMapsClient.Update: ensure existing.Annotations is non-nil (create map if nil), iterate over configMap.Annotations and copy/overwrite keys into existing.Annotations, and only then call configMapsClient.Update(ctx, existing, metav1.UpdateOptions{}); handle nil configMap.Annotations by leaving existing annotations untouched.
137-142:⚠️ Potential issue | 🟠 MajorHandle atomic replace events and re-add the file watch.
A lot of editors/config tools update files via temp-file + rename. With only
Write|Create, the watcher can miss future changes after aRename/Removeon the original inode.With fsnotify on Linux, do atomic file replacements emit `Rename`/`Remove` on the old watched file, and does the new inode require re-adding the file watch?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 137 - 142, The watcher currently only treats Write/Create as relevant; change isRelevantEvent to also treat fsnotify.Rename and fsnotify.Remove as relevant, and ensure the watcher logic (where events from isRelevantEvent are handled) attempts to remove and re-add the watch for s.file when a Rename or Remove is seen so the new inode is watched; update the handling around DNSConfigurationWatcherManager methods (isRelevantEvent and the event processing path that manages the fsnotify watcher) to detect Rename/Remove, call the code that removes the old watch and then re-adds a watch for s.file (or call a helper like addWatch/reAddWatch if present) so atomic replace (temp-file + rename) doesn’t lose future events.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/dns.go`:
- Around line 92-111: The validateConfigFile() path checks (os.Stat on cleanPath
producing fi) currently allow non-regular files (directories, FIFOs, device
nodes); update the validation after calling os.Stat to reject any non-regular
file by checking fi.Mode().IsRegular() and returning a clear error referencing
t.ConfigFile if it is not regular, before proceeding to size checks and os.Open;
keep existing size and readability checks (fi.Size(), os.Open) intact so only
regular files are accepted upfront.
---
Duplicate comments:
In `@pkg/components/controllers.go`:
- Around line 337-343: Before reading the DNS Corefile, re-check its size to
ensure it does not exceed the 1 MiB ConfigMap limit; specifically, in the branch
handling cfg.DNS.ConfigFile (the block that currently calls os.ReadFile and then
assets.ApplyConfigMapWithData with cm and kubeconfigPath), stat the file
(os.Stat) and return a clear error if the size is > 1*1024*1024 so the check
catches growth that occurred after dns.validate() and prevents a confusing API
write failure when calling assets.ApplyConfigMapWithData.
In `@pkg/controllers/dnsconfigurationwatcher.go`:
- Around line 56-59: The race comes from calling updateConfigMap before
computing/setting the hash, allowing the file to change between update and
getFileHash and causing missed updates; fix by computing the file hash with
getFileHash() first, use that hash to decide whether to update, pass or apply
that same computed hash when calling updateConfigMap (or store it as the
ConfigMap annotation) and then set lastHash = computedHash only after a
successful update; apply the same change in both places that currently update
the ConfigMap (the initial creation path using updateConfigMap(...) and the
subsequent update path around the existing getFileHash/lastHash checks).
- Around line 204-210: The Get error must not be assumed to mean "not found";
update the logic around configMapsClient.Get(ctx, dnsConfigMapName,
metav1.GetOptions{}) to check apierrors.IsNotFound(err) and only call
configMapsClient.Create(...) in that branch; for any other error from Get (e.g.
RBAC/throttling), return the error immediately (wrap with fmt.Errorf as
appropriate). Ensure you reference the existing variables and functions
(configMapsClient, dnsConfigMapName, configMap, s.Name()) and keep the success
log (klog.Infof) only after a successful Create.
- Around line 212-214: The code currently replaces annotations via
existing.Annotations = configMap.Annotations which drops preexisting metadata;
instead merge configMap.Annotations into existing.Annotations before calling
configMapsClient.Update: ensure existing.Annotations is non-nil (create map if
nil), iterate over configMap.Annotations and copy/overwrite keys into
existing.Annotations, and only then call configMapsClient.Update(ctx, existing,
metav1.UpdateOptions{}); handle nil configMap.Annotations by leaving existing
annotations untouched.
- Around line 137-142: The watcher currently only treats Write/Create as
relevant; change isRelevantEvent to also treat fsnotify.Rename and
fsnotify.Remove as relevant, and ensure the watcher logic (where events from
isRelevantEvent are handled) attempts to remove and re-add the watch for s.file
when a Rename or Remove is seen so the new inode is watched; update the handling
around DNSConfigurationWatcherManager methods (isRelevantEvent and the event
processing path that manages the fsnotify watcher) to detect Rename/Remove, call
the code that removes the old watch and then re-adds a watch for s.file (or call
a helper like addWatch/reAddWatch if present) so atomic replace (temp-file +
rename) doesn’t lose future events.
In `@test/suites/standard1/dns-custom-config.robot`:
- Around line 44-45: After the "Update Corefile With New Host" step, add a
poll/assert that the ConfigMap key dns-default.data.Corefile contains
${updated_hostname} before running "Resolve Host From Pod"; implement this by
polling the ConfigMap (e.g., a retry loop or Robot's Wait Until Keyword
Succeeds) until the Corefile text includes ${updated_hostname} and fail if the
timeout is reached so the test only proceeds once the watcher/CoreDNS reload has
actually observed the change.
- Around line 87-92: The test reads the dns-default ConfigMap once with the
keyword Oc Get JsonPath and asserts via Should Contain, but this can race with
asynchronous updates after Restart MicroShift; update the "Verify ConfigMap Uses
Custom Corefile" case to retry the read/assert by wrapping the Oc Get JsonPath +
Should Contain sequence inside a Wait Until Keyword Succeeds (or equivalent
retry) block so it re-reads ${corefile_data} until the ${HOSTNAME} appears,
keeping the existing keywords names (Oc Get JsonPath, Should Contain, Wait Until
Keyword Succeeds) and the test name intact.
- Around line 103-104: The test calls BuiltIn's Should Contain with ${output},
"Name:" and ${hostname} which makes ${hostname} the custom failure message
instead of part of the expected substring; update the Should Contain invocation
(after the Oc Exec that sets ${output}) to pass the full expected substring as
the second positional argument — e.g. combine the tokens into one expected
string like "Name: ${hostname}" (so call Should Contain ${output} Name:
${hostname}) so Robot treats the whole "Name: <hostname>" as the substring to
look for.
- Line 20: The path assigned to ${COREFILE_TEMPLATE} is relative to the working
directory and should be suite-relative; update the value in
dns-custom-config.robot (the ${COREFILE_TEMPLATE} assignment) from
./assets/dns/Corefile.template to the suite-relative path
../../assets/dns/Corefile.template (or use a suite-dir variable if preferred) so
it resolves to test/assets/dns/Corefile.template when the suite runs.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 13e9a4b4-69a8-44a2-a75c-4b33d2dee538
📒 Files selected for processing (8)
pkg/cmd/run.gopkg/components/controllers.gopkg/config/config.gopkg/config/dns.gopkg/config/dns_test.gopkg/controllers/dnsconfigurationwatcher.gotest/assets/dns/Corefile.templatetest/suites/standard1/dns-custom-config.robot
✅ Files skipped from review due to trivial changes (2)
- pkg/config/config.go
- test/assets/dns/Corefile.template
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cmd/run.go
- pkg/config/dns_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/config/dns.go (1)
124-139: Redundantos.Statcalls.
os.Stat(cleanPath)is called twice (lines 126 and 134). The second call is unnecessary since the result from line 126 can be reused.♻️ Proposed fix
cleanPath := filepath.Clean(t.Hosts.File) fi, err := os.Stat(cleanPath) - if err == nil && fi.Size() > 1048576 { - return fmt.Errorf("hosts file %s exceeds 1MiB ConfigMap (and internal buffer) size limit (got %d bytes)", t.Hosts.File, fi.Size()) - } - if !filepath.IsAbs(cleanPath) { - return fmt.Errorf("hosts file path must be absolute: got %s", t.Hosts.File) - } - - _, err = os.Stat(cleanPath) if os.IsNotExist(err) { return fmt.Errorf("hosts file %s does not exist", t.Hosts.File) } else if err != nil { return fmt.Errorf("error checking hosts file %s: %v", t.Hosts.File, err) } + if !filepath.IsAbs(cleanPath) { + return fmt.Errorf("hosts file path must be absolute: got %s", t.Hosts.File) + } + if fi.Size() > 1048576 { + return fmt.Errorf("hosts file %s exceeds 1MiB ConfigMap (and internal buffer) size limit (got %d bytes)", t.Hosts.File, fi.Size()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/dns.go` around lines 124 - 139, The code redundantly calls os.Stat(cleanPath) twice; reuse the initial os.Stat result (fi, err) instead of calling os.Stat again: after cleaning path (cleanPath) call os.Stat once, check err for IsNotExist and other errors, then if err == nil use fi.Size() to enforce the 1MiB limit and also validate filepath.IsAbs(cleanPath) and return errors referencing t.Hosts.File as currently done; update the logic around the existing fi and err variables to remove the second os.Stat call and preserve the same error messages.pkg/controllers/dnsconfigurationwatcher.go (1)
110-112: Doublewatcher.Close()on context cancellation.
watcher.Close()is called here onctx.Done(), but it's also deferred at line 67. Calling Close twice on fsnotify.Watcher is safe (returns nil on second call), but returningwatcher.Close()error here is misleading since the deferred close will also run.♻️ Proposed fix
case <-ctx.Done(): klog.Infof("%s stopping", s.Name()) - return watcher.Close() + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 110 - 112, The ctx.Done() branch logs stopping with s.Name() then calls and returns watcher.Close(), but watcher.Close() is already deferred (defer watcher.Close()), so return watcher.Close() is redundant/misleading; change the ctx.Done() case to just log and return (or return nil) so the deferred watcher.Close() handles closing and no Close() error is propagated from this branch—locate the ctx.Done() case that references s.Name() and watcher.Close() in the same scope as the deferred watcher.Close() and replace the return watcher.Close() with a plain return (or return nil).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/dnsconfigurationwatcher.go`:
- Around line 160-162: When s.updateConfigMap(ctx, kubeClient) fails, the code
currently returns currentHash which can cause future identical-file changes to
be skipped; change the error return to return an empty/zero hash (e.g. "" )
instead of currentHash so the watcher will not treat the failed update as
already-applied; update the error branch in the function containing
s.updateConfigMap to return false, "" , err (or another sentinel “no-hash” value
used elsewhere) so the next change with the same computed hash will trigger an
update.
---
Nitpick comments:
In `@pkg/config/dns.go`:
- Around line 124-139: The code redundantly calls os.Stat(cleanPath) twice;
reuse the initial os.Stat result (fi, err) instead of calling os.Stat again:
after cleaning path (cleanPath) call os.Stat once, check err for IsNotExist and
other errors, then if err == nil use fi.Size() to enforce the 1MiB limit and
also validate filepath.IsAbs(cleanPath) and return errors referencing
t.Hosts.File as currently done; update the logic around the existing fi and err
variables to remove the second os.Stat call and preserve the same error
messages.
In `@pkg/controllers/dnsconfigurationwatcher.go`:
- Around line 110-112: The ctx.Done() branch logs stopping with s.Name() then
calls and returns watcher.Close(), but watcher.Close() is already deferred
(defer watcher.Close()), so return watcher.Close() is redundant/misleading;
change the ctx.Done() case to just log and return (or return nil) so the
deferred watcher.Close() handles closing and no Close() error is propagated from
this branch—locate the ctx.Done() case that references s.Name() and
watcher.Close() in the same scope as the deferred watcher.Close() and replace
the return watcher.Close() with a plain return (or return nil).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ecc76575-50bf-4dfc-9c46-c9beec447f22
📒 Files selected for processing (5)
pkg/config/dns.gopkg/config/dns_test.gopkg/controllers/dnsconfigurationwatcher.gotest/assets/dns/Corefile.templatetest/suites/standard1/dns-custom-config.robot
✅ Files skipped from review due to trivial changes (1)
- test/assets/dns/Corefile.template
🚧 Files skipped from review as they are similar to previous changes (1)
- test/suites/standard1/dns-custom-config.robot
Comment on lines
+160
to
+162
| if err := s.updateConfigMap(ctx, kubeClient); err != nil { | ||
| klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) | ||
| return false, currentHash, err |
There was a problem hiding this comment.
Hash returned on error may cause missed updates.
When updateConfigMap fails, currentHash is returned. If the next file change produces the same hash, the update will be skipped even though the ConfigMap was never successfully updated.
🛡️ Proposed fix
if err := s.updateConfigMap(ctx, kubeClient); err != nil {
klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err)
- return false, currentHash, err
+ return false, lastHash, err
}📝 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
| if err := s.updateConfigMap(ctx, kubeClient); err != nil { | |
| klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) | |
| return false, currentHash, err | |
| if err := s.updateConfigMap(ctx, kubeClient); err != nil { | |
| klog.Errorf("%s failed to update ConfigMap: %v", s.Name(), err) | |
| return false, lastHash, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/dnsconfigurationwatcher.go` around lines 160 - 162, When
s.updateConfigMap(ctx, kubeClient) fails, the code currently returns currentHash
which can cause future identical-file changes to be skipped; change the error
return to return an empty/zero hash (e.g. "" ) instead of currentHash so the
watcher will not treat the failed update as already-applied; update the error
branch in the function containing s.updateConfigMap to return false, "" , err
(or another sentinel “no-hash” value used elsewhere) so the next change with the
same computed hash will trigger an update.
Add a new ConfigFile field to the DNS config struct that allows users to specify a custom CoreDNS Corefile path. Validation ensures mutual exclusivity with dns.hosts, and checks that the file is absolute, exists, is readable, non-empty, and within the 1MiB ConfigMap limit.
When dns.configFile is set, read the file and apply its contents directly as the Corefile key in the dns-default ConfigMap, bypassing the default template rendering. When unset, existing behavior is preserved.
Add DNSConfigurationWatcherManager that watches the custom Corefile specified by dns.configFile for changes at runtime, updating the dns-default ConfigMap when the file is modified. Follows the same fsnotify + SHA256 hashing pattern as the existing hosts watcher.
Cover mutual exclusivity with dns.hosts, non-absolute path, missing file, empty file, file exceeding 1MiB limit, valid config, and default behavior when configFile is unset.
pacevedom
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@pacevedom: This pull request references USHIFT-6902 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. This pull request references USHIFT-6903 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
RobotFramework tests covering: - Custom Corefile is used by CoreDNS instead of the default - Runtime reload propagates changes without MicroShift restart - Cluster-local resolution works with custom Corefile
- Use apierrors.IsNotFound for ConfigMap Get errors instead of treating any error as not-found - Merge annotations into existing ConfigMap instead of replacing them to avoid clobbering annotations set by the asset apply - Handle atomic-rename file updates (Rename/Remove events) and re-add the file watch on Create so the watcher survives vim/sed -i edits - Reject non-regular files (directories, FIFOs) in validateConfigFile - Fix Should Contain assertion in robot test that treated the hostname as a failure message instead of a second assertion - Add retry wrapper to ConfigMap verification after restart
Contributor
Author
|
/retest |
2 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
|
@pacevedom: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Validation
Tests