Add interval and Infoblox maxResults to ExternalDNS CR#483
Add interval and Infoblox maxResults to ExternalDNS CR#483sanjaytripathi97 wants to merge 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughTwo optional fields are added to the Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (12 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.
We don't need updates in alpha version, only beta update is enough.
There was a problem hiding this comment.
Agreed, I've reverted all v1alpha1 changes. The new fields (spec.interval and spec.provider.infoblox.maxResults) are only added to v1beta1, and the CRD schema is regenerated for the storage version accordingly.
| // Interval specifies the interval between two consecutive synchronizations | ||
| // performed by ExternalDNS. When omitted, ExternalDNS uses its default | ||
| // interval of 1 minute. | ||
| // | ||
| // +kubebuilder:validation:Optional | ||
| // +optional |
There was a problem hiding this comment.
Default value as long as minimum and maximum values should be set the CRD. This will enable early validation without saving the state in the server. The default value should be the same at the current default to not change the existing behavior.
There was a problem hiding this comment.
Done. spec.interval now has CRD-level validation:
- default:
1m(matches external-dns default) - minimum:
10s - maximum:
24h
The operator passes --interval only when the value is greater than zero. With the CRD default of 1m, new resources get the same behavior as external-dns today without requiring users to set the field explicitly.
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +optional |
There was a problem hiding this comment.
Similar comment, we need to set the default which is the same as the current used one and we need to think of what the maximum value can be reasonable for the max results.
Overall, I had small reservations about exposing max_results because the problems related to the need to customize this parameter are supposed to be fixed with pagination which is built-in in the webhook implementation. However I can see that the webhook implementation does suppose max_results parameter too, so this field will make sense in the future too (when we'll migrate to the webhook provider for Infoblox).
There was a problem hiding this comment.
Done. spec.provider.infoblox.maxResults now has CRD-level validation:
- default:
0(current behavior_max_resultsis not sent to WAPI) - minimum:
0 - maximum:
10000
The operator only adds --infoblox-max-results when the value is greater than zero, so existing Infoblox deployments are unchanged unless the user opts in.
I agree this field will remain useful even after a potential move to the Infoblox webhook provider, since that implementation also supports max_results. This gives customers a supported way to configure it today via the ExternalDNS CR.
| r.validateInterval(), | ||
| r.validateInfobloxMaxResults(), |
There was a problem hiding this comment.
We prefer CRD validation over webhook because it's earlier in the life cycle of the CR.
There was a problem hiding this comment.
Agreed I've removed validateInterval() and validateInfobloxMaxResults() from the webhook. Validation for both fields is now handled entirely via CRD schema defaults and bounds, so invalid values are rejected earlier without relying on the admission webhook.
There was a problem hiding this comment.
Unit test is a good start but we will need some e2e test scenarios to back up this change. You can find the e2e tests in test/e2e/ (from the root of repository).
There was a problem hiding this comment.
Added test/e2e/infoblox_sync_options_test.go with TestExternalDNSInfobloxSyncOptions. The test:
- Creates an Infoblox
ExternalDNSCR withintervalandmaxResultsset - Waits for the operand deployment to become available
- Verifies the external-dns container args include
--interval=...and--infoblox-max-results=...
The test is skipped unless the e2e suite runs against the Infoblox provider. Unit tests in pod_interval_test.go continue to cover the deployment arg builder logic, let me know if this is fine.
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
| defer func() { | ||
| _ = common.KubeClient.Delete(context.TODO(), &extDNS) | ||
| }() |
There was a problem hiding this comment.
Handle cleanup delete failures instead of discarding them.
Line 58 drops the delete error, which can hide leaked CRs and make later e2e runs flaky.
Suggested fix
import (
"context"
"fmt"
"strings"
"testing"
"time"
"github.com/openshift/external-dns-operator/test/common"
appsv1 "k8s.io/api/apps/v1"
+ apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
)
@@
defer func() {
- _ = common.KubeClient.Delete(context.TODO(), &extDNS)
+ if err := common.KubeClient.Delete(context.TODO(), &extDNS); err != nil && !apierrors.IsNotFound(err) {
+ t.Errorf("failed to delete external DNS %q: %v", testInfobloxSyncExtDNSName, err)
+ }
}()As per coding guidelines, **/*.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` around lines 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.
Source: Coding guidelines
| for _, container := range deployment.Spec.Template.Spec.Containers { | ||
| if !strings.HasPrefix(container.Name, "external-dns") { | ||
| continue | ||
| } | ||
| if !containsArg(container.Args, intervalArg) { | ||
| t.Logf("container %s is missing %q in args: %v", container.Name, intervalArg, container.Args) | ||
| return false, nil | ||
| } | ||
| if !containsArg(container.Args, maxResultsArg) { | ||
| t.Logf("container %s is missing %q in args: %v", container.Name, maxResultsArg, container.Args) | ||
| return false, nil | ||
| } | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
Fail when no external-dns container is found to avoid false-positive passes.
Line 94 returns success even if the loop never validates any container args (e.g., name mismatch), so this test can pass without checking the new flags.
Suggested fix
intervalArg := fmt.Sprintf("--interval=%s", syncInterval.Duration.String())
maxResultsArg := fmt.Sprintf("--infoblox-max-results=%d", testInfobloxMaxResults)
+ foundExternalDNSContainer := false
for _, container := range deployment.Spec.Template.Spec.Containers {
if !strings.HasPrefix(container.Name, "external-dns") {
continue
}
+ foundExternalDNSContainer = true
if !containsArg(container.Args, intervalArg) {
t.Logf("container %s is missing %q in args: %v", container.Name, intervalArg, container.Args)
return false, nil
}
if !containsArg(container.Args, maxResultsArg) {
t.Logf("container %s is missing %q in args: %v", container.Name, maxResultsArg, container.Args)
return false, nil
}
}
+ if !foundExternalDNSContainer {
+ t.Log("external-dns container not found in deployment")
+ return false, nil
+ }
return true, nil🤖 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` around lines 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.
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.
sanjaytripathi97
force-pushed
the
add-infoblox-max-results-and-interval
branch
from
fc8963e to
ecd0951
Compare
3 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).