NE-2411: Add Template field to DNS operator#2765
NE-2411: Add Template field to DNS operator#2765grzpiotrowski wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @grzpiotrowski! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughAdds a new exported feature gate variable 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
/retitle [WIP] NE-2411: Add templates field to DNS operator |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@grzpiotrowski: This pull request references NE-2411 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 "4.22.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@operator/v1/types_dns.go`:
- Around line 528-535: The Zones slice lacks per-item format validation; update
the Zones field in operator/v1/types_dns.go to add a kubebuilder validation
Pattern that enforces RFC1123 DNS names or the literal ".". Specifically, add a
comment like //
+kubebuilder:validation:Pattern="^(\\.|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)$"
immediately above the Zones []string `json:"zones"` declaration so each entry is
validated as either "." or an RFC1123-compliant name. Ensure the annotation is
placed alongside the existing Required/MinItems tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 964b4669-8aa2-4d3f-bc4d-79f2a6924c53
📒 Files selected for processing (2)
features/features.gooperator/v1/types_dns.go
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
added
size/XXL
openshift-ci
Bot
removed
the
needs-rebase
grzpiotrowski
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
grzpiotrowski
changed the title
|
@grzpiotrowski: This pull request references NE-2411 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 "4.22.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. |
| // actions defines a list of actions to apply to matching queries. | ||
| // | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +required | ||
| Actions []TemplateAction `json:"actions"` |
There was a problem hiding this comment.
Just now I realized that with a slice we may have some duplicates which may be contradictory. Especially when the API will be enhanced to other actions (answer, authority, additional):
spec:
template:
actions:
- returnEmpty:
rcode: NOERRROR
- returnEmpty:
rcode: NXDOMAIN
There was a problem hiding this comment.
Changed the action to a struct.
So now the spec example would be:
spec:
template:
zones: ["example.com", "abc.com"]
queryType: AAAA
queryClass: IN
action:
rcode: NOERROR
|
@grzpiotrowski: This pull request references NE-2411 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 "4.22.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. |
grzpiotrowski
force-pushed
the
NE-2411
branch
2 times, most recently
from
f3b5fcd to
cb219ec
Compare
|
@grzpiotrowski: This pull request references NE-2411 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 "4.22.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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
operator/v1/types_dns.go (1)
513-531:⚠️ Potential issue | 🟠 MajorValidate each
zonesentry against the documented DNS format.Right now the schema only checks count and length. Values like
bad_domainorexample.com.will be accepted here and only fail later when the CoreDNS config is rendered.Suggested schema guard
// +listType=set // +kubebuilder:validation:MinItems=1 // +kubebuilder:validation:MaxItems=15 // +kubebuilder:validation:items:MinLength=1 // +kubebuilder:validation:items:MaxLength=253 + // +kubebuilder:validation:items:Pattern=`^(\.|([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*)$` // +required Zones []string `json:"zones,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/types_dns.go` around lines 513 - 531, The Zones slice currently only validates count and length; add an items-level pattern validation so each entry must be either the single root string "." or a RFC1123-compliant DNS name (no trailing dot, labels 1–63 chars, letters/numbers/hyphen, labels cannot start or end with hyphen, full name max 253). Modify the Zones declaration in operator/v1/types_dns.go (the Zones []string field) to include a +kubebuilder:validation:items:Pattern tag that enforces "either '.' OR a valid DNS name" and update the field comment to note the stricter validation; this prevents invalid values like "bad_domain" or names with a trailing dot from being accepted.
🧹 Nitpick comments (1)
operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml (1)
4-5: Add a companion disabled-gate suite forspec.template.The suite-level
featureGatesfilter means this file only exercises CRDs whereDNSTemplatePluginis enabled (tests/types.go,tests/crd_filter.go). That leaves the disabled variants untested, so an accidental ungate/pruning regression would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml` around lines 4 - 5, Add a companion test suite that covers the DNSTemplatePlugin-disabled variant of spec.template so the CRD behavior when DNSTemplatePlugin is off is exercised; specifically, create a copy of the current DNSTemplatePlugin suite YAML but remove or clear the featureGates entry (so it does not list DNSTemplatePlugin) and save it as a separate suite (e.g., DNSTemplatePlugin.disabled.yaml), ensuring the new suite targets the same spec.template tests; this will let the existing tests/crd_filter.go suite-level featureGates filter run the disabled-case and catch gating/pruning regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@operator/v1/types_dns.go`:
- Around line 128-130: The CRD is missing RFC1123 pattern validation for the
zones slice; update the struct that defines the Zones field (Zones []string) to
include a kubebuilder validation Pattern annotation using the same DNS name
regex used elsewhere in this file (the regex around line 243) so each item is
RFC1123-compliant; keep the existing MinItems/MaxItems/MaxLength annotations and
add the kubebuilder:validation:Pattern line above the Zones field declaration to
enforce the per-item DNS name format.
---
Duplicate comments:
In `@operator/v1/types_dns.go`:
- Around line 513-531: The Zones slice currently only validates count and
length; add an items-level pattern validation so each entry must be either the
single root string "." or a RFC1123-compliant DNS name (no trailing dot, labels
1–63 chars, letters/numbers/hyphen, labels cannot start or end with hyphen, full
name max 253). Modify the Zones declaration in operator/v1/types_dns.go (the
Zones []string field) to include a +kubebuilder:validation:items:Pattern tag
that enforces "either '.' OR a valid DNS name" and update the field comment to
note the stricter validation; this prevents invalid values like "bad_domain" or
names with a trailing dot from being accepted.
---
Nitpick comments:
In `@operator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yaml`:
- Around line 4-5: Add a companion test suite that covers the
DNSTemplatePlugin-disabled variant of spec.template so the CRD behavior when
DNSTemplatePlugin is off is exercised; specifically, create a copy of the
current DNSTemplatePlugin suite YAML but remove or clear the featureGates entry
(so it does not list DNSTemplatePlugin) and save it as a separate suite (e.g.,
DNSTemplatePlugin.disabled.yaml), ensuring the new suite targets the same
spec.template tests; this will let the existing tests/crd_filter.go suite-level
featureGates filter run the disabled-case and catch gating/pruning regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 95a71bcf-4a81-42a3-a7bb-55f27a83bf9b
⛔ Files ignored due to path filters (7)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests/dnses.operator.openshift.io/DNSTemplatePlugin.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (15)
features.mdfeatures/features.gooperator/v1/tests/dnses.operator.openshift.io/DNSTemplatePlugin.yamloperator/v1/types_dns.gooperator/v1/zz_generated.deepcopy.gooperator/v1/zz_generated.featuregated-crd-manifests.yamloperator/v1/zz_generated.swagger_doc_generated.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
✅ Files skipped from review due to trivial changes (5)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
- features.md
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- operator/v1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- operator/v1/zz_generated.swagger_doc_generated.go
| // +optional | ||
| // +openshift:enable:FeatureGate=DNSTemplatePlugin | ||
| Template Template `json:"template,omitempty,omitzero"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
gomod=$(fd -a '^go\.mod$' | head -n1)
echo "== toolchain =="
sed -n '/^go /p;/^toolchain /p' "$gomod"
echo
echo "== relevant dependency versions =="
sed -n '/k8s\.io\/apimachinery/p;/sigs\.k8s\.io\/json/p' "$gomod" || true
echo
tmpdir=$(mktemp -d)
python - "$gomod" "$tmpdir" <<'PY'
import io, pathlib, re, sys, urllib.request, zipfile
gomod_text = pathlib.Path(sys.argv[1]).read_text()
tmpdir = pathlib.Path(sys.argv[2])
downloaded = {}
def find_version(text, mod):
m = re.search(rf'^\s*{re.escape(mod)}\s+(v[^\s]+)', text, re.M)
return m.group(1) if m else None
def download(mod, ver):
if not ver or mod in downloaded:
return
data = urllib.request.urlopen(f"https://proxy.golang.org/{mod}/@v/{ver}.zip").read()
out = tmpdir / mod.replace("/", "_")
out.mkdir(parents=True, exist_ok=True)
with zipfile.ZipFile(io.BytesIO(data)) as zf:
zf.extractall(out)
downloaded[mod] = out
print(f"{mod} {ver} -> {out}")
api_ver = find_version(gomod_text, "k8s.io/apimachinery")
json_ver = find_version(gomod_text, "sigs.k8s.io/json")
download("k8s.io/apimachinery", api_ver)
if not json_ver and "k8s.io/apimachinery" in downloaded:
api_gomod = next(downloaded["k8s.io/apimachinery"].rglob("go.mod"), None)
if api_gomod:
json_ver = find_version(api_gomod.read_text(), "sigs.k8s.io/json")
download("sigs.k8s.io/json", json_ver)
PY
echo
echo "== omitzero support in downloaded modules =="
rg -n "omitzero" "$tmpdir" || true
echo
echo "== serializer JSON implementation references =="
rg -n "sigs\.k8s\.io/json|encoding/json" "$tmpdir" || trueRepository: openshift/api
Length of output: 2852
🏁 Script executed:
# First, locate and examine the types_dns.go file
fd -a 'types_dns\.go$' | head -n5Repository: openshift/api
Length of output: 142
🏁 Script executed:
# Examine the Template field and surrounding context (lines around 128-130)
sed -n '120,140p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 1243
🏁 Script executed:
# Examine the Template struct definition (lines 482-564)
sed -n '480,570p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 3254
🏁 Script executed:
# Search for existing omitzero usage in the codebase
rg -n 'omitzero' --type goRepository: openshift/api
Length of output: 50369
🏁 Script executed:
# Search for other optional value-type fields with similar patterns
rg -n 'json:"[^"]*,omitempty"' operator/v1/types_dns.go -A 1Repository: openshift/api
Length of output: 1977
🏁 Script executed:
# Check for tests related to Template field
fd -a '.*test.*\.go$' | xargs grep -l "Template" | head -5Repository: openshift/api
Length of output: 122
🏁 Script executed:
# Check if there are any GitHub issues or comments about omitzero handling
rg -n "omitzero" operator/v1/types_dns.go -B 2 -A 2Repository: openshift/api
Length of output: 293
🏁 Script executed:
# Verify zone validation specifically - check if there's a pattern constraint
sed -n '510,535p' operator/v1/types_dns.goRepository: openshift/api
Length of output: 1274
🏁 Script executed:
# Check if there's a pattern validation elsewhere for zones or DNS names
rg -n "pattern.*dns|pattern.*zone|kubebuilder:validation:Pattern" operator/v1/types_dns.goRepository: openshift/api
Length of output: 481
🏁 Script executed:
# Look for RFC 1123 validation patterns in the codebase
rg -n "RFC|1123|dns.*pattern" operator/v1/ --type go | head -20Repository: openshift/api
Length of output: 5596
🏁 Script executed:
# Check if structured merge diff or other serializers respect omitzero for nested structs
grep -r "omitzero" vendor/sigs.k8s.io/structured-merge-diff/v6/value/ -A 3 | head -30Repository: openshift/api
Length of output: 2618
Add pattern validation for zones to enforce RFC 1123 compliance.
The zones field documentation states entries must conform to RFC 1123, but the CRD schema only validates item length and count. Add a kubebuilder:validation:Pattern constraint matching the DNS name pattern used elsewhere in this file (e.g., line 243).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@operator/v1/types_dns.go` around lines 128 - 130, The CRD is missing RFC1123
pattern validation for the zones slice; update the struct that defines the Zones
field (Zones []string) to include a kubebuilder validation Pattern annotation
using the same DNS name regex used elsewhere in this file (the regex around line
243) so each item is RFC1123-compliant; keep the existing
MinItems/MaxItems/MaxLength annotations and add the
kubebuilder:validation:Pattern line above the Zones field declaration to enforce
the per-item DNS name format.
|
/assign |
| contactPerson("grzpiotrowski"). | ||
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1936"). | ||
| enable(inDevPreviewNoUpgrade()). |
There was a problem hiding this comment.
As we discussed on slack, this is fine to start in Dev Preview. If we'd like to move to TP, we do require the attached enhancement to be merged.
| // If this field is nil, no servers are created. | ||
| // | ||
| // +optional | ||
| // +listType=map |
There was a problem hiding this comment.
I think the default listtype is atomic, so this technically could be a breaking change, since it makes the validation more restrictive. Would you foresee any situation where this could be a problem?
| // | ||
| // +optional | ||
| // +openshift:enable:FeatureGate=DNSTemplatePlugin | ||
| Template Template `json:"template,omitempty,omitzero"` |
There was a problem hiding this comment.
(minor) I think the omitzero is sufficient enough, and the omitempty is redundant.
| type Server struct { | ||
| // name is required and specifies a unique name for the server. Name must comply | ||
| // with the Service Name Syntax of rfc6335. | ||
| // +required |
There was a problem hiding this comment.
Out of curiosity, are you planning on making all legacy fields more conformant with API standards? I think there's some more things we would need to do (example, validations for the syntax here, min/max length, etc.) so maybe it's worth it to separately update old fields and focus this PR on the template field.
Generally also applies to other existing fields.
There was a problem hiding this comment.
This was in response to the failing CI verify-crd-schema with the ListsMustHaveSSATags error and some following errors.
I believe this is because between this PR and the last merged one to this CRD, there were some additional requirements added in the tests.
I kept these as a separate commit, but probably better if these kind of changes were separated into another PR altogether like you said.
| // - ["example.com", "test.com"] matches both domains and their subdomains | ||
| // | ||
| // +listType=set | ||
| // +kubebuilder:validation:MinItems=1 |
There was a problem hiding this comment.
restrictions on the list should also be in the godoc itself (e.g. here // At least 1 and at most 15 zones may be specified.)
There was a problem hiding this comment.
Added the comment in godoc.
| // rcode is the DNS response code to return. | ||
| // Valid values are "NOERROR". | ||
| // | ||
| // When set, the template returns a response with no answer records. For AAAA filtering, |
There was a problem hiding this comment.
The field is required, so the when set comment is a bit confusing. I think we should remove any conditional statements
|
|
||
| // Template defines a template for custom DNS query handling via the CoreDNS template plugin. | ||
| // Template enables filtering or custom responses for DNS queries matching specific zones and query types. | ||
| type Template struct { |
There was a problem hiding this comment.
Should we consider making this slightly less generic? (e.g. DNSTemplate, like we have DNSCache above)
| // Valid values are "AAAA". | ||
| // | ||
| // +required | ||
| QueryType QueryType `json:"queryType,omitempty"` |
There was a problem hiding this comment.
I'm a bit confused on why each of these are a required, set string field that only has 1 option. Do you believe that this will be expanded on in the future? Can these be optional and have the controller set a reasonable default if the user doesn't set them?
There was a problem hiding this comment.
Yes this was with the intention of making it expandable in the future with other query types.
And making it and the other fields required as to ensure that the configuration applied is clear to the user.
Though I reckon a default value could be also considered, but then I imagine we would need to stick to that default (AAAA in this case) in the future, also when other types are added to the available ones.
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
@grzpiotrowski: 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
Add coreDNS template plugin API support to enable custom DNS query handling coreDNS. The main use case is filtering AAAA queries in IPv4-only clusters to reduce DNS latency and upstream load.
Example usage:
Intruducing new types:
Template- Template configuration with zones, query type, query class, and actionsQueryType- DNS query types (AAAA initially, the goal is to have it extensible to A, CNAME etc. in the future, same for other types)QueryClass- DNS query classes (only IN initially)ResponseCode- DNS response codes (NOERROR initially)TemplateAction- Discriminated union for template actionsReturnEmptyAction- initial action for returning empty DNS responses