notifiers: Stop using url.Parse in validating address format. (#4011)

url.Parse() wrongly parses an address of format "address:port"
which is fixed in go1.8.  This inculcates a breaking change
on our end. We should fix this wrong usage everywhere so that
migrating to go1.8 eventually becomes smoother.
master
Harshavardhana 8 years ago committed by GitHub
parent 096427f973
commit f1015a5096
  1. 2
      cmd/admin-handlers_test.go
  2. 4
      cmd/config-v18_test.go
  3. 7
      cmd/notify-kafka.go
  4. 3
      cmd/notify-nats.go
  5. 4
      cmd/notify-redis.go
  6. 8
      cmd/utils.go
  7. 8
      cmd/utils_test.go

@ -1377,7 +1377,7 @@ func TestWriteSetConfigResponse(t *testing.T) {
}, },
} }
testURL, err := url.Parse("dummy.com") testURL, err := url.Parse("http://dummy.com")
if err != nil { if err != nil {
t.Fatalf("Failed to parse a place-holder url") t.Fatalf("Failed to parse a place-holder url")
} }

@ -299,10 +299,10 @@ func TestValidateConfig(t *testing.T) {
{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "format": "namespace", "url": "example.com", "index": "myindex" } }}}`, true}, {`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "elasticsearch": { "1": { "enable": true, "format": "namespace", "url": "example.com", "index": "myindex" } }}}`, true},
// Test 27 - Test Format for Redis // Test 27 - Test Format for Redis
{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "invalid", "address": "example.com", "password": "xxx", "key": "key1" } }}}`, false}, {`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "invalid", "address": "example.com:80", "password": "xxx", "key": "key1" } }}}`, false},
// Test 28 - Test valid Format for Redis // Test 28 - Test valid Format for Redis
{`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "namespace", "address": "example.com", "password": "xxx", "key": "key1" } }}}`, true}, {`{"version": "` + v + `", "credential": { "accessKey": "minio", "secretKey": "minio123" }, "region": "us-east-1", "browser": "on", "notify": { "redis": { "1": { "enable": true, "format": "namespace", "address": "example.com:80", "password": "xxx", "key": "key1" } }}}`, true},
} }
for i, testCase := range testCases { for i, testCase := range testCases {

@ -20,6 +20,7 @@ import (
"errors" "errors"
"fmt" "fmt"
"io/ioutil" "io/ioutil"
"net"
"github.com/Sirupsen/logrus" "github.com/Sirupsen/logrus"
@ -47,6 +48,12 @@ func (k *kafkaNotify) Validate() error {
if len(k.Brokers) == 0 { if len(k.Brokers) == 0 {
return errors.New("No broker specified") return errors.New("No broker specified")
} }
// Validate all specified brokers.
for _, brokerAddr := range k.Brokers {
if _, _, err := net.SplitHostPort(brokerAddr); err != nil {
return err
}
}
return nil return nil
} }

@ -18,6 +18,7 @@ package cmd
import ( import (
"io/ioutil" "io/ioutil"
"net"
"github.com/Sirupsen/logrus" "github.com/Sirupsen/logrus"
"github.com/nats-io/go-nats-streaming" "github.com/nats-io/go-nats-streaming"
@ -52,7 +53,7 @@ func (n *natsNotify) Validate() error {
if !n.Enable { if !n.Enable {
return nil return nil
} }
if _, err := checkURL(n.Address); err != nil { if _, _, err := net.SplitHostPort(n.Address); err != nil {
return err return err
} }
return nil return nil

@ -20,6 +20,7 @@ import (
"encoding/json" "encoding/json"
"fmt" "fmt"
"io/ioutil" "io/ioutil"
"net"
"time" "time"
"github.com/Sirupsen/logrus" "github.com/Sirupsen/logrus"
@ -52,7 +53,7 @@ func (r *redisNotify) Validate() error {
if r.Format != formatNamespace && r.Format != formatAccess { if r.Format != formatNamespace && r.Format != formatAccess {
return rdNFormatError return rdNFormatError
} }
if _, err := checkURL(r.Addr); err != nil { if _, _, err := net.SplitHostPort(r.Addr); err != nil {
return err return err
} }
if r.Key == "" { if r.Key == "" {
@ -73,6 +74,7 @@ func dialRedis(rNotify redisNotify) (*redis.Pool, error) {
if !rNotify.Enable { if !rNotify.Enable {
return nil, errNotifyNotEnabled return nil, errNotifyNotEnabled
} }
addr := rNotify.Addr addr := rNotify.Addr
password := rNotify.Password password := rNotify.Password
rPool := &redis.Pool{ rPool := &redis.Pool{

@ -266,13 +266,13 @@ func isFile(path string) bool {
} }
// checkURL - checks if passed address correspond // checkURL - checks if passed address correspond
func checkURL(address string) (*url.URL, error) { func checkURL(urlStr string) (*url.URL, error) {
if address == "" { if urlStr == "" {
return nil, errors.New("Address cannot be empty") return nil, errors.New("Address cannot be empty")
} }
u, err := url.Parse(address) u, err := url.Parse(urlStr)
if err != nil { if err != nil {
return nil, fmt.Errorf("`%s` invalid: %s", address, err.Error()) return nil, fmt.Errorf("`%s` invalid: %s", urlStr, err.Error())
} }
return u, nil return u, nil
} }

@ -389,16 +389,14 @@ func TestLocalAddress(t *testing.T) {
} }
// TestCheckURL tests valid address // TestCheckURL tests valid url.
func TestCheckURL(t *testing.T) { func TestCheckURL(t *testing.T) {
testCases := []struct { testCases := []struct {
addr string urlStr string
shouldPass bool shouldPass bool
}{ }{
{"", false}, {"", false},
{":", false}, {":", false},
{"localhost", true},
{"127.0.0.1", true},
{"http://localhost/", true}, {"http://localhost/", true},
{"http://127.0.0.1/", true}, {"http://127.0.0.1/", true},
{"proto://myhostname/path", true}, {"proto://myhostname/path", true},
@ -406,7 +404,7 @@ func TestCheckURL(t *testing.T) {
// Validates fetching local address. // Validates fetching local address.
for i, testCase := range testCases { for i, testCase := range testCases {
_, err := checkURL(testCase.addr) _, err := checkURL(testCase.urlStr)
if testCase.shouldPass && err != nil { if testCase.shouldPass && err != nil {
t.Errorf("Test %d: expected to pass but got an error: %v\n", i+1, err) t.Errorf("Test %d: expected to pass but got an error: %v\n", i+1, err)
} }

Loading…
Cancel
Save