Add interval and Infoblox maxResults to ExternalDNS CR - #483
Add interval and Infoblox maxResults to ExternalDNS CR#483sanjaytripathi97 wants to merge 1 commit into
Conversation
|
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:
📝 WalkthroughWalkthroughThe API and CRD schemas add bounded Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ 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 |
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
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.
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
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.
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.
Expose sync interval and Infoblox WAPI max results through the ExternalDNS API so operator-managed deployments can configure --interval and --infoblox-max-results without manual Deployment patches. intervalSeconds is provider-agnostic (int32, min 60 / max 3600) with no CRD default; when unset the operator omits --interval and external-dns keeps its own default. maxResults is Infoblox-only (min 0 / max 10000) and is omitted when unset/0. Includes unit coverage in TestDesiredExternalDNSDeployment, a provider-agnostic e2e that verifies real sync spacing from operand logs, and regenerated CRD/bundle manifests.
13f0516 to
c2dd31b
Compare
|
/ok-to-test |
|
@sanjaytripathi97: 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. |
|
All CI is green. /approve Holding to let NI&D team have a look. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87 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
4 participants
Related issue
Customers integrating OpenShift External DNS Operator with Infoblox need two external-dns flags that were not exposed through the
ExternalDNSCR:--interval— controls how often external-dns synchronizes DNS records--infoblox-max-results— sets the maximum number of DNS records Infoblox returns per requestWithout these, operator-managed deployments only received a fixed set of hardcoded args. Manual Deployment patches were reverted on reconcile because the operator owns container args.
This PR exposes both settings declaratively via the
ExternalDNSCR and wires them through the operator deployment builder.New CR fields
spec.intervalSeconds--intervalspec.provider.infoblox.maxResults--infoblox-max-resultsBoth fields are optional (v1beta1 only). When unset/
0, the operator omits the corresponding flag and external-dns keeps its own defaults. CRD validation:intervalSecondsmin 60 / max 3600;maxResultsmin 0 / max 10000 (no CRD defaults).