Disallow @ character in URL patterns, so that people don't mistakenly…

… try

to add URL patterns of the form username@hostname/path.

Extend admission controller to reject invalid URL patterns on secrets to
provide early feedback to end users when their patterns are wrong.

pkg/build/admission/secretinjector/admission.go

@@ -7,7 +7,9 @@ import (
 
 	"github.com/golang/glog"
 
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/admission"
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/kubernetes/pkg/api"
@@ -21,7 +23,7 @@ import (
 func init() {
 	admission.RegisterPlugin("openshift.io/BuildConfigSecretInjector", func(config io.Reader) (admission.Interface, error) {
 		return &secretInjector{
-			Handler: admission.NewHandler(admission.Create),
+			Handler: admission.NewHandler(admission.Create, admission.Update),
 		}, nil
 	})
 }
@@ -34,11 +36,20 @@ type secretInjector struct {
 var _ = oadmission.WantsRESTClientConfig(&secretInjector{})
 
 func (si *secretInjector) Admit(attr admission.Attributes) (err error) {
-	bc, ok := attr.GetObject().(*buildapi.BuildConfig)
-	if !ok {
-		return nil
+	obj := attr.GetObject()
+
+	if bc, ok := obj.(*buildapi.BuildConfig); ok && attr.GetOperation() == admission.Create {
+		return si.admitNewBuildConfig(attr, bc)
 	}
 
+	if secret, ok := obj.(*api.Secret); ok {
+		return si.admitSecret(attr, secret)
+	}
+
+	return nil
+}
+
+func (si *secretInjector) admitNewBuildConfig(attr admission.Attributes, bc *buildapi.BuildConfig) (err error) {
 	if bc.Spec.Source.SourceSecret != nil || bc.Spec.Source.Git == nil {
 		return nil
 	}
@@ -98,6 +109,30 @@ func (si *secretInjector) Admit(attr admission.Attributes) (err error) {
 	return nil
 }
 
+func (si *secretInjector) admitSecret(attr admission.Attributes, secret *api.Secret) (err error) {
+	errs := field.ErrorList{}
+
+	for k, v := range secret.GetAnnotations() {
+		if strings.HasPrefix(k, buildapi.BuildSourceSecretMatchURIAnnotationPrefix) {
+			v = strings.TrimSpace(v)
+			if v == "" {
+				continue
+			}
+
+			_, err := urlpattern.NewURLPattern(v)
+			if err != nil {
+				errs = append(errs, field.Invalid(field.NewPath("metadata.annotations", k), v, err.Error()))
+			}
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.NewInvalid(api.Kind("secret"), secret.Name, errs)
+	}
+
+	return nil
+}
+
 func (si *secretInjector) SetRESTClientConfig(restClientConfig restclient.Config) {
 	si.restClientConfig = restClientConfig
 }

pkg/util/urlpattern/urlpattern.go

@@ -12,7 +12,7 @@ var InvalidPatternError = errors.New("invalid pattern")
 
 var urlPatternRegex = regexp.MustCompile(`^` +
 	`(?:(\*|git|http|https|ssh)://)` +
-	`(\*|(?:\*\.)?[^/*]+)` +
+	`(\*|(?:\*\.)?[^@/*]+)` +
 	`(/.*)` +
 	`$`)
 

pkg/util/urlpattern/urlpattern_test.go

@@ -44,7 +44,7 @@ func TestMatchPattern(t *testing.T) {
 			expectedScheme:   `^(git|http|https|ssh)$`,
 			expectedHost:     `^.*$`,
 			expectedPath:     `^/.*$`,
-			expectedMatch:    []string{`https://github.com/`},
+			expectedMatch:    []string{`https://github.com/`, `https://user:password@github.com/`, `ssh://git@github.com/`},
 			expectedNotMatch: []string{`ftp://github.com/`},
 		},
 		{
@@ -80,15 +80,15 @@ func TestMatchPattern(t *testing.T) {
 			expectedScheme:   `^https$`,
 			expectedHost:     `^github\.com$`,
 			expectedPath:     `^/.*$`,
-			expectedMatch:    []string{`https://github.com/`},
+			expectedMatch:    []string{`https://github.com/`, `https://user:password@github.com/`},
 			expectedNotMatch: []string{`https://test.github.com/`},
 		},
 		{
 			pattern:        `https://*.git.luolix.top/*`,
 			expectedScheme: `^https$`,
 			expectedHost:   `^(?:.*\.)?github\.com$`,
 			expectedPath:   `^/.*$`,
-			expectedMatch:  []string{`https://github.com/`, `https://test.github.com/`},
+			expectedMatch:  []string{`https://github.com/`, `https://user:password@github.com/`, `https://test.github.com/`},
 		},
 		{
 			pattern:        `https://\.+?()|[]{}^$/*`,
@@ -108,6 +108,10 @@ func TestMatchPattern(t *testing.T) {
 			pattern:     `https://git*hub.com/*`,
 			expectedErr: true,
 		},
+		{
+			pattern:     `*://git@github.com/*`,
+			expectedErr: true,
+		},
 		{
 			pattern:        `https://github.com/`,
 			expectedScheme: `^https$`,