add SetDefaults from parent (using context), change Destination to us… (#1031)

* add SetDefaults from parent (using context), change Destination to use KnativeReference

* address pr comments, change address resolver to use kref
This commit is contained in:
Ville Aikas 2020-02-01 11:59:29 -08:00 committed by GitHub
parent b88c371782
commit 99abcc2ff5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 182 additions and 66 deletions

View File

@ -19,7 +19,6 @@ package v1
import (
"context"
corev1 "k8s.io/api/core/v1"
"knative.dev/pkg/apis"
)
@ -27,7 +26,7 @@ import (
type Destination struct {
// Ref points to an Addressable.
// +optional
Ref *corev1.ObjectReference `json:"ref,omitempty"`
Ref *KnativeReference `json:"ref,omitempty"`
// URI can be an absolute URL(non-empty scheme and non-empty host) pointing to the target or a relative URI. Relative URIs will be resolved using the base URI retrieved from Ref.
// +optional
@ -38,56 +37,41 @@ func (dest *Destination) Validate(ctx context.Context) *apis.FieldError {
if dest == nil {
return nil
}
return ValidateDestination(*dest).ViaField(apis.CurrentField)
return ValidateDestination(ctx, *dest).ViaField(apis.CurrentField)
}
// ValidateDestination validates Destination.
func ValidateDestination(dest Destination) *apis.FieldError {
var ref *corev1.ObjectReference
if dest.Ref != nil {
ref = dest.Ref
}
if ref == nil && dest.URI == nil {
func ValidateDestination(ctx context.Context, dest Destination) *apis.FieldError {
ref := dest.Ref
uri := dest.URI
if ref == nil && uri == nil {
return apis.ErrGeneric("expected at least one, got none", "ref", "uri")
}
if ref != nil && dest.URI != nil && dest.URI.URL().IsAbs() {
if ref != nil && uri != nil && uri.URL().IsAbs() {
return apis.ErrGeneric("Absolute URI is not allowed when Ref or [apiVersion, kind, name] is present", "[apiVersion, kind, name]", "ref", "uri")
}
// IsAbs() check whether the URL has a non-empty scheme. Besides the non-empty scheme, we also require dest.URI has a non-empty host
if ref == nil && dest.URI != nil && (!dest.URI.URL().IsAbs() || dest.URI.Host == "") {
// IsAbs() check whether the URL has a non-empty scheme. Besides the non-empty scheme, we also require uri has a non-empty host
if ref == nil && uri != nil && (!uri.URL().IsAbs() || uri.Host == "") {
return apis.ErrInvalidValue("Relative URI is not allowed when Ref and [apiVersion, kind, name] is absent", "uri")
}
if ref != nil && dest.URI == nil {
if dest.Ref != nil {
return validateDestinationRef(*ref).ViaField("ref")
}
if ref != nil && uri == nil {
return ref.Validate(ctx).ViaField("ref")
}
return nil
}
// GetRef gets the ObjectReference from this Destination, if one is present. If no ref is present,
// GetRef gets the KnativeReference from this Destination, if one is present. If no ref is present,
// then nil is returned.
func (dest *Destination) GetRef() *corev1.ObjectReference {
func (dest *Destination) GetRef() *KnativeReference {
if dest == nil {
return nil
}
return dest.Ref
}
func validateDestinationRef(ref corev1.ObjectReference) *apis.FieldError {
// Check the object.
var errs *apis.FieldError
// Required Fields
if ref.Name == "" {
errs = errs.Also(apis.ErrMissingField("name"))
func (d *Destination) SetDefaults(ctx context.Context) {
if d.Ref != nil && d.Ref.Namespace == "" {
d.Ref.Namespace = apis.ParentMeta(ctx).Namespace
}
if ref.APIVersion == "" {
errs = errs.Also(apis.ErrMissingField("apiVersion"))
}
if ref.Kind == "" {
errs = errs.Also(apis.ErrMissingField("kind"))
}
return errs
}

View File

@ -21,7 +21,7 @@ import (
"testing"
"github.com/google/go-cmp/cmp"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)
@ -35,10 +35,11 @@ const (
func TestValidateDestination(t *testing.T) {
ctx := context.TODO()
validRef := corev1.ObjectReference{
validRef := KnativeReference{
Kind: kind,
APIVersion: apiVersion,
Name: name,
Namespace: namespace,
}
validURL := apis.URL{
@ -60,9 +61,20 @@ func TestValidateDestination(t *testing.T) {
},
want: "",
},
"invalid ref, missing namespace": {
dest: &Destination{
Ref: &KnativeReference{
Name: name,
Kind: kind,
APIVersion: apiVersion,
},
},
want: "missing field(s): ref.namespace",
},
"invalid ref, missing name": {
dest: &Destination{
Ref: &corev1.ObjectReference{
Ref: &KnativeReference{
Namespace: namespace,
Kind: kind,
APIVersion: apiVersion,
},
@ -71,16 +83,18 @@ func TestValidateDestination(t *testing.T) {
},
"invalid ref, missing api version": {
dest: &Destination{
Ref: &corev1.ObjectReference{
Kind: kind,
Name: apiVersion,
Ref: &KnativeReference{
Namespace: namespace,
Kind: kind,
Name: name,
},
},
want: "missing field(s): ref.apiVersion",
},
"invalid ref, missing kind": {
dest: &Destination{
Ref: &corev1.ObjectReference{
Ref: &KnativeReference{
Namespace: namespace,
APIVersion: apiVersion,
Name: name,
},
@ -145,14 +159,14 @@ func TestValidateDestination(t *testing.T) {
}
func TestDestination_GetRef(t *testing.T) {
ref := &corev1.ObjectReference{
ref := &KnativeReference{
APIVersion: apiVersion,
Kind: kind,
Name: name,
}
tests := map[string]struct {
dest *Destination
want *corev1.ObjectReference
want *KnativeReference
}{
"nil destination": {
dest: nil,
@ -183,3 +197,57 @@ func TestDestination_GetRef(t *testing.T) {
})
}
}
func TestDestinationSetDefaults(t *testing.T) {
ctx := context.Background()
parentNamespace := "parentNamespace"
tests := map[string]struct {
d *Destination
ctx context.Context
want string
}{
"uri set, nothing in ref, not modified ": {
d: &Destination{URI: apis.HTTP("example.com")},
ctx: ctx,
want: "",
},
"namespace set, nothing in context, not modified ": {
d: &Destination{Ref: &KnativeReference{Namespace: namespace}},
ctx: ctx,
want: namespace,
},
"namespace set, context set, not modified ": {
d: &Destination{Ref: &KnativeReference{Namespace: namespace}},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: namespace,
},
"namespace set, uri set, context set, not modified ": {
d: &Destination{Ref: &KnativeReference{Namespace: namespace}, URI: apis.HTTP("example.com")},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: namespace,
},
"namespace not set, context set, defaulted": {
d: &Destination{Ref: &KnativeReference{}},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: parentNamespace,
},
"namespace not set, uri set, context set, defaulted": {
d: &Destination{Ref: &KnativeReference{}, URI: apis.HTTP("example.com")},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: parentNamespace,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
tc.d.SetDefaults(tc.ctx)
if tc.d.Ref != nil && tc.d.Ref.Namespace != tc.want {
t.Errorf("Got: %s wanted %s", tc.d.Ref.Namespace, tc.want)
}
if tc.d.Ref == nil && tc.want != "" {
t.Errorf("Got: nil Ref wanted %s", tc.want)
}
})
}
}

View File

@ -27,18 +27,18 @@ import (
type KnativeReference struct {
// Kind of the referent.
// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
Kind string `json:"kind,omitempty"`
Kind string `json:"kind"`
// Namespace of the referent.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/
Namespace string `json:"namespace,omitempty"`
Namespace string `json:"namespace"`
// Name of the referent.
// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
Name string `json:"name,omitempty"`
Name string `json:"name"`
// API version of the referent.
APIVersion string `json:"apiVersion,omitempty"`
APIVersion string `json:"apiVersion"`
}
func (kr *KnativeReference) Validate(ctx context.Context) *apis.FieldError {
@ -64,3 +64,9 @@ func (kr *KnativeReference) Validate(ctx context.Context) *apis.FieldError {
}
return errs
}
func (kr *KnativeReference) SetDefaults(ctx context.Context) {
if kr.Namespace == "" {
kr.Namespace = apis.ParentMeta(ctx).Namespace
}
}

View File

@ -21,11 +21,12 @@ import (
"testing"
"github.com/google/go-cmp/cmp"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"knative.dev/pkg/apis"
)
func TestValidate(t *testing.T) {
ctx := context.TODO()
ctx := context.Background()
validRef := KnativeReference{
Kind: kind,
@ -93,10 +94,46 @@ func TestValidate(t *testing.T) {
if tc.want != nil {
if diff := cmp.Diff(tc.want.Error(), gotErr.Error()); diff != "" {
t.Errorf("%s: got: %v wanted %v", name, gotErr, tc.want)
t.Errorf("Got: %v wanted %v", gotErr, tc.want)
}
} else if gotErr != nil {
t.Errorf("%s: Validate() = %v, wanted nil", name, gotErr)
t.Errorf("Validate() = %v, wanted nil", gotErr)
}
})
}
}
func TestKnativeReferenceSetDefaults(t *testing.T) {
ctx := context.Background()
parentNamespace := "parentNamespace"
tests := map[string]struct {
ref *KnativeReference
ctx context.Context
want string
}{
"namespace set, nothing in context, not modified ": {
ref: &KnativeReference{Namespace: namespace},
ctx: ctx,
want: namespace,
},
"namespace set, context set, not modified ": {
ref: &KnativeReference{Namespace: namespace},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: namespace,
},
"namespace not set, context set, defaulted": {
ref: &KnativeReference{},
ctx: apis.WithinParent(ctx, metav1.ObjectMeta{Namespace: parentNamespace}),
want: parentNamespace,
},
}
for name, tc := range tests {
t.Run(name, func(t *testing.T) {
tc.ref.SetDefaults(tc.ctx)
if tc.ref.Namespace != tc.want {
t.Errorf("Got: %s wanted %s", tc.ref.Namespace, tc.want)
}
})
}

View File

@ -21,7 +21,6 @@ limitations under the License.
package v1
import (
corev1 "k8s.io/api/core/v1"
runtime "k8s.io/apimachinery/pkg/runtime"
apis "knative.dev/pkg/apis"
)
@ -178,7 +177,7 @@ func (in *Destination) DeepCopyInto(out *Destination) {
*out = *in
if in.Ref != nil {
in, out := &in.Ref, &out.Ref
*out = new(corev1.ObjectReference)
*out = new(KnativeReference)
**out = **in
}
if in.URI != nil {

View File

@ -106,7 +106,7 @@ func (r *URIResolver) URIFromDestination(dest duckv1beta1.Destination, parent in
// URIFromDestinationV1 resolves a v1.Destination into a URL.
func (r *URIResolver) URIFromDestinationV1(dest duckv1.Destination, parent interface{}) (*apis.URL, error) {
if dest.Ref != nil {
url, err := r.URIFromObjectReference(dest.Ref, parent)
url, err := r.URIFromKnativeReference(dest.Ref, parent)
if err != nil {
return nil, err
}
@ -130,6 +130,10 @@ func (r *URIResolver) URIFromDestinationV1(dest duckv1.Destination, parent inter
return nil, errors.New("destination missing Ref and URI, expected at least one")
}
func (r *URIResolver) URIFromKnativeReference(ref *duckv1.KnativeReference, parent interface{}) (*apis.URL, error) {
return r.URIFromObjectReference(&corev1.ObjectReference{Name: ref.Name, Namespace: ref.Namespace, APIVersion: ref.APIVersion, Kind: ref.Kind}, parent)
}
// URIFromObjectReference resolves an ObjectReference to a URI string.
func (r *URIResolver) URIFromObjectReference(ref *corev1.ObjectReference, parent interface{}) (*apis.URL, error) {
if ref == nil {

View File

@ -395,7 +395,7 @@ func TestGetURIDestinationV1(t *testing.T) {
objects: []runtime.Object{
getAddressable(),
},
dest: duckv1.Destination{Ref: getAddressableRef()},
dest: duckv1.Destination{Ref: getAddressableKnativeRef()},
wantURI: addressableDNS,
}, "happy ref to k8s service": {
objects: []runtime.Object{
@ -408,7 +408,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressable(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "/foo",
},
@ -419,7 +419,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressable(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "foo",
},
@ -430,7 +430,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressableWithPathAndTrailingSlash(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "foo",
},
@ -441,7 +441,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressableWithPathAndTrailingSlash(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "/foo",
},
@ -452,7 +452,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressableWithPathAndNoTrailingSlash(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "foo",
},
@ -463,7 +463,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressableWithPathAndNoTrailingSlash(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Path: "/foo",
},
@ -474,7 +474,7 @@ func TestGetURIDestinationV1(t *testing.T) {
getAddressable(),
},
dest: duckv1.Destination{
Ref: getAddressableRef(),
Ref: getAddressableKnativeRef(),
URI: &apis.URL{
Scheme: "http",
Host: "example.com",
@ -487,29 +487,29 @@ func TestGetURIDestinationV1(t *testing.T) {
objects: []runtime.Object{
getAddressableNilURL(),
},
dest: duckv1.Destination{Ref: getUnaddressableRef()},
dest: duckv1.Destination{Ref: getUnaddressableKnativeRef()},
wantErr: fmt.Sprintf("url missing in address of %+v", getUnaddressableRef()),
},
"nil address": {
objects: []runtime.Object{
getAddressableNilAddress(),
},
dest: duckv1.Destination{Ref: getUnaddressableRef()},
dest: duckv1.Destination{Ref: getUnaddressableKnativeRef()},
wantErr: fmt.Sprintf("address not set for %+v", getUnaddressableRef()),
}, "missing host": {
objects: []runtime.Object{
getAddressableNoHostURL(),
},
dest: duckv1.Destination{Ref: getUnaddressableRef()},
dest: duckv1.Destination{Ref: getUnaddressableKnativeRef()},
wantErr: fmt.Sprintf("hostname missing in address of %+v", getUnaddressableRef()),
}, "missing status": {
objects: []runtime.Object{
getAddressableNoStatus(),
},
dest: duckv1.Destination{Ref: getUnaddressableRef()},
dest: duckv1.Destination{Ref: getUnaddressableKnativeRef()},
wantErr: fmt.Sprintf("address not set for %+v", getUnaddressableRef()),
}, "notFound": {
dest: duckv1.Destination{Ref: getUnaddressableRef()},
dest: duckv1.Destination{Ref: getUnaddressableKnativeRef()},
wantErr: fmt.Sprintf("failed to get ref %+v: %s %q not found", getUnaddressableRef(), unaddressableResource, unaddressableName),
}}
@ -713,8 +713,8 @@ func getInvalidObjectReference() *corev1.ObjectReference {
}
func getK8SServiceRef() *corev1.ObjectReference {
return &corev1.ObjectReference{
func getK8SServiceRef() *duckv1.KnativeReference {
return &duckv1.KnativeReference{
Kind: "Service",
Name: addressableName,
APIVersion: "v1",
@ -723,6 +723,15 @@ func getK8SServiceRef() *corev1.ObjectReference {
}
func getAddressableKnativeRef() *duckv1.KnativeReference {
return &duckv1.KnativeReference{
Kind: addressableKind,
Name: addressableName,
APIVersion: addressableAPIVersion,
Namespace: testNS,
}
}
func getAddressableRef() *corev1.ObjectReference {
return &corev1.ObjectReference{
Kind: addressableKind,
@ -732,6 +741,15 @@ func getAddressableRef() *corev1.ObjectReference {
}
}
func getUnaddressableKnativeRef() *duckv1.KnativeReference {
return &duckv1.KnativeReference{
Kind: unaddressableKind,
Name: unaddressableName,
APIVersion: unaddressableAPIVersion,
Namespace: testNS,
}
}
func getUnaddressableRef() *corev1.ObjectReference {
return &corev1.ObjectReference{
Kind: unaddressableKind,