馃尡 Validate Host DNS name in BareMetalHost resource #1040
Conversation
metal3-io-bot
added
the
size/M
| if err != nil { | ||
| host = hurl.Host | ||
| } | ||
| valid, _ := regexp.MatchString(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`, host) |
There was a problem hiding this comment.
I think, we should also handle error case in here instead hiding it.
| } | ||
| valid, _ := regexp.MatchString(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`, host) | ||
|
|
||
| if !(valid) { |
There was a problem hiding this comment.
There is no need for parenthesis in here.
| @@ -71,6 +71,17 @@ func TestValidateCreate(t *testing.T) { | |||
| oldBMH: nil, | |||
| wantedErr: "hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time", | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
I think, we can add more test like happy path or more edge cases.
There was a problem hiding this comment.
Okay, I will add a few more test cases.
| } | ||
|
|
||
| hurl, _ := url.Parse(hostaddress) | ||
| host, port, _ := net.SplitHostPort(hurl.Host) |
There was a problem hiding this comment.
@hroyrh, if port is empty host = hurl.Host seems ok to me. But why we are not checking error in here?
There was a problem hiding this comment.
It raises error for port empty too and as for the validity of the url, the regex checks that.
|
/retest-required |
| @@ -18,6 +21,10 @@ func (host *BareMetalHost) validateHost() []error { | |||
| errs = append(errs, err) | |||
| } | |||
|
|
|||
| if err := validateDNSName(host.Spec.BMC.Address); err != nil { | |||
There was a problem hiding this comment.
@hroyrh hardwareutils/bmc package is added in this module. Maybe it would be useful using bmc's functionality for that validation if it is possible.
There was a problem hiding this comment.
I agree, the validation itself should probably go to the bmc package, if it's not there already
| @@ -53,3 +60,24 @@ func validateRAID(r *RAIDConfig) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func validateDNSName(hostaddress string) error { | |||
There was a problem hiding this comment.
@hroyrh what I was thinking is something like that;
if hostaddress == "" {
return nil
}
hurl, err := url.Parse(hostaddress)
if err != nil {
if !strings.Contains(hostaddress, ":") {
return fmt.Errorf("Invalid format")
}
}
host, port, err := net.SplitHostPort(hurl.Host)
if err != nil {
return fmt.Errorf("another invalid format")
}
if port == "" {
host = hurl.Host
}
valid, err := regexp.MatchString(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`, host)
if err != nil {
return fmt.Errorf("regex error")
}
if !valid {
return fmt.Errorf("Host DNS name is invalid")
}
return nil
|
/hold |
metal3-io-bot
added
the
do-not-merge/hold
|
/hold cancel |
metal3-io-bot
removed
the
do-not-merge/hold
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
metal3-io-bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
metal3-io-bot
removed
the
lifecycle/stale
|
/uncc |
hroyrh
force-pushed
the
bmo-validate-dnsname
branch
from
260d5ce
to
2061aa4
Compare
metal3-io-bot
removed
the
needs-rebase
|
|
||
| _, err := bmc.GetParsedURL(hostaddress) | ||
| if err != nil { | ||
| return fmt.Errorf("host DNS name is invalid") |
There was a problem hiding this comment.
Could you include the original error using errors.Wrap?
There was a problem hiding this comment.
@dtantsur I've wrapped the errors, and added a few more test cases, can you please take another look.
metal3-io-bot
added
size/L
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
|
/lgtm |
|
/test-centos-integration-main |
furkatgofurov7
changed the title
Validate host dns name part of BareMetalHost bmc address.