feat(pdns): add validation for MX and SRV records #4871
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Hi @julillae. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
Thanks for this PR, which is tested. |
|
/retitle feat(pdns): add validation for MX and SRV records |
k8s-ci-robot
changed the title
|
@mloiseleur That is a great point! I moved the checkEndpoint function to endpoint/endpoint.go. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/assign @ivankatliarchuk for a first review |
|
/assign |
|
Hi @julillae was it tested on real cluster? Could you also provide a set of manual test steps using manifests and kubectl commands? This will help in understanding and verifying the implementation. |
| @@ -396,3 +397,41 @@ func RemoveDuplicates(endpoints []*Endpoint) []*Endpoint { | |||
|
|
|||
| return result | |||
| } | |||
|
|
|||
| // Check endpoint if is it properly formatted according to RFC standards | |||
| func CheckEndpoint(endpoint Endpoint) bool { | |||
There was a problem hiding this comment.
Can we convert this to a method of the Endpoint type?
func (e *Endpoint) CheckEndpointTargets() bool {
// function body
}
| } | ||
|
|
||
| preferenceRaw := targetParts[0] | ||
| _, err := strconv.ParseInt(preferenceRaw, 10, 32) |
There was a problem hiding this comment.
what this numbers are e.g. 10 and 32?
| for _, part := range targetParts[:3] { | ||
| _, err := strconv.ParseInt(part, 10, 32) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
shell we add debug logging here, so it's clear why record is rejected?
pseudo code
debug("Invalid %s record target: %s and reason %s", type, target, reason))
| return false | ||
| } | ||
| } | ||
| case "SRV": |
There was a problem hiding this comment.
Could we use constants instead?
case RecordTypeSRV:
| } | ||
| } | ||
| case "SRV": | ||
| for _, target := range endpoint.Targets { |
There was a problem hiding this comment.
An optional, nice to have solution, is to move actual validation from this block to a Target
func (t Target) ValidateSRVRecord() {
...body...
}
func (t Target) ValidateMXRecord() {
...body...
}
Isolating the validation logic into separate functions makes it easier to update or modify the validation rules for each record type without affecting the main function. And should simplify testing as well
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // AdjustEndpoints performs checks on the provided endpoints and will skip any potentially failing changes. | ||
| func (p *PDNSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) { | ||
| var validEndpoints []*endpoint.Endpoint | ||
| for i := 0; i < len(endpoints); i++ { |
There was a problem hiding this comment.
Is this require a null check guard?
the *endpoints could be nill or value of *endpoints[i] could be nil
| @@ -433,6 +433,19 @@ func (p *PDNSProvider) Records(ctx context.Context) (endpoints []*endpoint.Endpo | |||
| return endpoints, nil | |||
| } | |||
|
|
|||
| // AdjustEndpoints performs checks on the provided endpoints and will skip any potentially failing changes. | |||
| func (p *PDNSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) { | |||
There was a problem hiding this comment.
is this method need to be public?
Description
Adds a check to validate the format of MX and SRV records. In addition, implements AdjustEndpoints to filter incorrectly formatted records which we know will fail to be created by PowerDNS. This change improves error handling for MX and SRV records and provides a base to validate other record types in the future.
Fixes #ISSUE
Checklist