Add interval and Infoblox maxResults to ExternalDNS CR - #483
Add interval and Infoblox maxResults to ExternalDNS CR#483sanjaytripathi97 wants to merge 11 commits into
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds optional Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ 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 |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @sanjaytripathi97. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/usage.md (1)
213-214: 💤 Low valueConsider clarifying that
intervalapplies to all providers.The
intervalfield is provider-agnostic and can be used with AWS, GCP, Azure, BlueCat, and Infoblox. Currently it's only documented in the Infoblox example, which may give the impression it's Infoblox-specific. Consider adding a brief note such as "optional, applies to all providers" or showing it in another provider example.🤖 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 `@docs/usage.md` around lines 213 - 214, Clarify that the interval field is provider-agnostic by updating the docs: add a short note next to the existing "interval: 5m" line (or in a shared config section) stating "optional, applies to all providers (AWS, GCP, Azure, BlueCat, Infoblox)" and/or show the same interval comment in at least one other provider example so readers don't assume it's Infoblox-specific; update the text around the example to reference the global nature of the interval setting and ensure the symbol "interval" and provider names AWS, GCP, Azure, BlueCat, Infoblox are mentioned.
🤖 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.
Nitpick comments:
In `@docs/usage.md`:
- Around line 213-214: Clarify that the interval field is provider-agnostic by
updating the docs: add a short note next to the existing "interval: 5m" line (or
in a shared config section) stating "optional, applies to all providers (AWS,
GCP, Azure, BlueCat, Infoblox)" and/or show the same interval comment in at
least one other provider example so readers don't assume it's Infoblox-specific;
update the text around the example to reference the global nature of the
interval setting and ensure the symbol "interval" and provider names AWS, GCP,
Azure, BlueCat, Infoblox are mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2b2c06bf-7b29-4061-b5b4-9062bd77b002
⛔ Files ignored due to path filters (2)
api/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*api/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (8)
api/v1alpha1/externaldns_types.goapi/v1beta1/externaldns_types.goapi/v1beta1/externaldns_webhook.goconfig/crd/bases/externaldns.olm.openshift.io_externaldnses.yamlconfig/samples/infoblox/operator_v1beta1_infoblox_openshift.yamldocs/usage.mdpkg/operator/controller/externaldns/pod.gopkg/operator/controller/externaldns/pod_interval_test.go
|
@alebedev87 @grzpiotrowski Please review when you can. We have some developers looking to use CNAME records in our root domain and need the max results added to resolve their plight. |
|
@theiratenate, @sanjaytripathi97 : https://redhat.atlassian.net/browse/RFE-6781 is not accepted yet. But I think it would be easier to do so if the PR will be in a ready state. Let me have a look. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/e2e/infoblox_sync_options_test.go`:
- Around line 81-94: The code at the end of the container validation loop
returns true, nil unconditionally at line 94, even if no external-dns container
was found and validated. To prevent false-positive test passes, introduce a
boolean flag before the loop to track whether an external-dns container was
found. Set this flag to true after successfully validating a container that has
a name starting with "external-dns" and contains both the intervalArg and
maxResultsArg arguments. At the end of the function, check this flag and return
false if no matching external-dns container was ever found and validated,
ensuring the test fails when expected containers are missing.
- Around line 57-59: The defer cleanup block in the test is ignoring the error
returned from common.KubeClient.Delete(context.TODO(), &extDNS) by using the
underscore operator, which violates the coding guideline of never ignoring error
returns and can hide leaked Custom Resources. Replace the underscore with a
proper error variable and handle the error appropriately by either logging it or
asserting that the deletion should succeed, rather than discarding the error.
🪄 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: 914281a1-313e-4627-bee0-d63885c658f7
⛔ Files ignored due to path filters (1)
api/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
api/v1beta1/externaldns_types.goconfig/crd/bases/externaldns.olm.openshift.io_externaldnses.yamlpkg/operator/controller/externaldns/pod.gopkg/operator/controller/externaldns/pod_interval_test.gotest/e2e/infoblox.gotest/e2e/infoblox_sync_options_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/operator/controller/externaldns/pod.go
- pkg/operator/controller/externaldns/pod_interval_test.go
- config/crd/bases/externaldns.olm.openshift.io_externaldnses.yaml
Expose sync interval and Infoblox WAPI pagination settings in the ExternalDNS API so operator-managed deployments can integrate with large Infoblox grids without manual deployment patches.
Revert v1alpha1 API changes, move validation to CRD defaults and bounds, remove webhook validators, and add an Infoblox e2e test for operand args.
fc8963e to
ecd0951
Compare
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
Thanks a lot for the follow-up! I think we are closer to the good implementation. I re-reviewed the OpenShift API conventions meanwhile and had to reconsider the defaulting strategy PTAL.
Rename interval to intervalSeconds as int32 with CRD min/max and no default. Drop maxResults CRD default. Update e2e to verify sync spacing from operand logs instead of only checking deployment args.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/e2e/infoblox_sync_options_test.go`:
- Line 194: Update the resource cleanup around readCloser in the
timestamp-parsing flow so its Close error is propagated when parsing otherwise
succeeds. Preserve any existing timestamp parsing error as the returned error,
and ensure the defer no longer discards readCloser.Close()’s result.
- Around line 102-108: Update the failure logging in the container argument
checks within the e2e test to omit the full container.Args list, which may
expose internal hostnames. Keep logging the container name and the missing
expected flag (intervalArg or maxResultsArg) only, while preserving the existing
failure returns.
- Around line 49-53: Register cleanup for the independently created credentials
Secret in the test immediately after the successful common.KubeClient.Create
call, using the test cleanup mechanism to delete credSecret and prevent stale
resources or name collisions. Keep the existing creation failure handling
unchanged.
🪄 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: Pro Plus
Run ID: 3d887fcc-ceb1-45d6-9ea5-a21b73640b5d
📒 Files selected for processing (8)
api/v1beta1/externaldns_types.goconfig/crd/bases/externaldns.olm.openshift.io_externaldnses.yamlconfig/samples/infoblox/operator_v1beta1_infoblox_openshift.yamldocs/usage.mdpkg/operator/controller/externaldns/pod.gopkg/operator/controller/externaldns/pod_interval_test.gotest/e2e/infoblox.gotest/e2e/infoblox_sync_options_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/samples/infoblox/operator_v1beta1_infoblox_openshift.yaml
- docs/usage.md
- test/e2e/infoblox.go
- pkg/operator/controller/externaldns/pod.go
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer readCloser.Close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the stream close error.
Line 194 discards Close()’s error. Propagate it when timestamp parsing otherwise succeeds.
As per path instructions, **/*.go: “Never ignore error returns”.
🤖 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 `@test/e2e/infoblox_sync_options_test.go` at line 194, Update the resource
cleanup around readCloser in the timestamp-parsing flow so its Close error is
propagated when parsing otherwise succeeds. Preserve any existing timestamp
parsing error as the returned error, and ensure the defer no longer discards
readCloser.Close()’s result.
Source: Path instructions
|
I just checked out and found that https://redhat.atlassian.net/browse/RFE-8890 and https://redhat.atlassian.net/browse/RFE-8888 got approved in April. This PR addresses both of these RFEs. Let's reference them in the description. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
The operand Deployment is created in the operator namespace unless E2E_SEPARATE_OPERAND_NAMESPACE is set; the test was looking in external-dns. Also commit the regenerated bundle CRD so verify and e2e install the intervalSeconds/maxResults schema.
make bundle regenerates CSV examples from config/samples; commit the intervalSeconds and maxResults values so verify stays clean.
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
Another review with a deeper look into unit and e2e tests.
There was a problem hiding this comment.
I think that we should add both interval and maxresults tests in the existing TestDesiredExternalDNSDeployment test function in deployment_test.go. A new Infoblox test case which specifies both of the new fields. The omitted values are supposed to be covered by the existing Infloblox test case(s).
There was a problem hiding this comment.
Done. Added an Infoblox case in TestDesiredExternalDNSDeployment that sets both intervalSeconds and maxResults and asserts --interval=120s and --infoblox-max-results=2000. Omitted values remain covered by the existing Nominal Infoblox case. Removed the standalone pod_interval_test.go.
| if helper.platform() != infobloxDNSProvider { | ||
| t.Skip("test only runs against Infoblox provider") | ||
| } |
There was a problem hiding this comment.
We try to keep all test provider agnostic and add them to operator_test.go. The intervalSeconds is exactly this case. Let's add it there and not skip for any provider.
There was a problem hiding this comment.
Done. Moved intervalSeconds coverage into TestExternalDNSSyncInterval in operator_test.go. It runs for all providers (no Infoblox-only skip) and verifies both the operand arg and sync spacing from logs. Removed infoblox_sync_options_test.go.
Update intervalSeconds/maxResults godoc for oc explain, cover both fields in TestDesiredExternalDNSDeployment, and move intervalSeconds e2e into provider-agnostic operator_test.go. Rely on unit tests for maxResults instead of a heavy Infoblox e2e scenario.
alebedev87
left a comment
alebedev87
left a comment
There was a problem hiding this comment.
The API LGTM, some remarks about the e2e test though.
| } | ||
| } | ||
|
|
||
| func TestExternalDNSSyncInterval(t *testing.T) { |
There was a problem hiding this comment.
We prefer to let the file grow by appending to the end. Can you please add this new test after the last TestXXX function?
There was a problem hiding this comment.
Done. Moved TestExternalDNSSyncInterval to the end of operator_test.go, after the other TestXXX functions.
| expected := []appsv1.DeploymentCondition{ | ||
| {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue}, | ||
| } | ||
| if err := waitForOperatorDeploymentStatusCondition(context.TODO(), t, common.KubeClient, expected...); err != nil { | ||
| t.Fatalf("operator was not available: %v", err) | ||
| } |
There was a problem hiding this comment.
This is redundant TestOperatorAvailable should validate this before TestExternalDNSSyncInterval kicks in.
There was a problem hiding this comment.
Done. Removed the extra operator availability wait; TestOperatorAvailable covers that earlier.
| testSyncIntervalExtDNSName = "test-extdns-interval" | ||
| testSyncIntervalSeconds int32 = 120 | ||
| syncLogMessage = "All records are already up to date" | ||
| minObservedSyncGap = 90 * time.Second | ||
| maxObservedSyncGap = 150 * time.Second |
There was a problem hiding this comment.
Those are variables which make sense only for TestExternalDNSSyncInterval, so should be moved in there.
There was a problem hiding this comment.
Done. Moved the sync-interval-only constants/vars into TestExternalDNSSyncInterval.
| curr := timestamps[len(timestamps)-1] | ||
| gap := curr.Sub(prev) | ||
| t.Logf("observed sync gap between %s and %s: %s", prev.Format(time.RFC3339), curr.Format(time.RFC3339), gap) | ||
| if gap < minObservedSyncGap || gap > maxObservedSyncGap { |
There was a problem hiding this comment.
Why the range is so vast? The sync interval is 120s and we are expecting it to be between 90s and 150s. I could understand +/- 5s but +/- 30s seems like a little too much.
There was a problem hiding this comment.
Done. Tightened the expected gap to [115s, 125s] (±5s around the configured 120s interval), I hope this is fine now.
| func containsContainerArg(args []string, expected string) bool { | ||
| for _, arg := range args { | ||
| if arg == expected { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func collectSyncTimestamps(ctx context.Context, namespace, podName, containerName string) ([]time.Time, error) { | ||
| opts := &corev1.PodLogOptions{ | ||
| Container: containerName, | ||
| Follow: false, | ||
| } | ||
| readCloser, err := common.KubeClientSet.CoreV1().Pods(namespace).GetLogs(podName, opts).Stream(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { | ||
| _ = readCloser.Close() | ||
| }() | ||
|
|
||
| return parseSyncTimestamps(readCloser) | ||
| } | ||
|
|
||
| func parseSyncTimestamps(r io.Reader) ([]time.Time, error) { | ||
| var timestamps []time.Time | ||
| scanner := bufio.NewScanner(r) | ||
| buf := make([]byte, 0, 64*1024) | ||
| scanner.Buffer(buf, 1024*1024) | ||
|
|
||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| if !strings.Contains(line, syncLogMessage) { | ||
| continue | ||
| } | ||
| matches := logTimestampPattern.FindStringSubmatch(line) | ||
| if len(matches) != 2 { | ||
| continue | ||
| } | ||
| ts, err := time.Parse(time.RFC3339Nano, matches[1]) | ||
| if err != nil { | ||
| ts, err = time.Parse(time.RFC3339, matches[1]) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| } | ||
| timestamps = append(timestamps, ts) | ||
| } | ||
| if err := scanner.Err(); err != nil { | ||
| return nil, err | ||
| } | ||
| return timestamps, nil | ||
| } |
There was a problem hiding this comment.
Can you please move all the helper functions to the bottom of the file? There is a dedicated comment marker there.
There was a problem hiding this comment.
Done. Moved the helper functions under the existing // HELPER FUNCTIONS marker.
| // OperandNamespace returns the namespace where ExternalDNS operand Deployments are created. | ||
| // By default the operator deploys operands into its own namespace; when | ||
| // E2E_SEPARATE_OPERAND_NAMESPACE=true it uses the dedicated "external-dns" namespace. | ||
| func OperandNamespace() string { | ||
| if os.Getenv(e2eSeparateOperandNsEnvVar) == "true" { | ||
| return operandNamespace | ||
| } | ||
| return OperatorNamespace | ||
| } |
There was a problem hiding this comment.
Deploying operands in external-dns-operator is defacto standard now. No need to count for the other case, you can drop this function and use common.OperatorNamespace wherever the operand namespace is needed.
There was a problem hiding this comment.
Done. Removed OperandNamespace() and now use common.OperatorNamespace for operand lookups.
Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Co-authored-by: Andrey Lebedev <alebedev87@gmail.com>
Move TestExternalDNSSyncInterval to the end of operator_test.go, drop the redundant operator readiness wait, tighten the observed sync gap tolerance, hard-fail when interval args are missing, and always use OperatorNamespace for operand lookups.
|
@sanjaytripathi97: The following tests failed, say
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. |
4 participants
Related issue
Customers integrating OpenShift External DNS Operator with Infoblox require two external-dns flags that were not exposed through the
ExternalDNSCR:--interval— controls how often external-dns synchronizes DNS records (reduces load on Infoblox)--infoblox-max-results— sets the_max_resultsWAPI query parameter, required for Infoblox grids with thousands of DNS records.Without these, operator-managed deployments only received a fixed set of hardcoded args. Manual patches to the Deployment were reverted on reconcile because the operator owns and reconciles container args.
This blocked successful Infoblox integration in enterprise environments and forced workarounds such as removing
ownerReferencefrom the Deployment (operator-unmanaged), which is unsupported and fragile.With this PR, Expose both settings declaratively via the
ExternalDNSCR and wire them through the operator deployment builder.New CR fields
spec.interval--intervalspec.provider.infoblox.maxResults--infoblox-max-results_max_resultson WAPI requests).