Skip to content

Create subscriber.uri and deprecate subscriber.dnsName #994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 2 commits into from
Mar 28, 2019
Merged

Create subscriber.uri and deprecate subscriber.dnsName #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07a94a7
Add URI and deprecate dnsName.
Harwayne Mar 28, 2019
b1643a5
Code changes for subscriber.uri.
Harwayne Mar 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions pkg/apis/eventing/v1alpha1/channel_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
"k8s.io/apimachinery/pkg/runtime"
)

var dnsName = "example.com"

func TestChannelValidation(t *testing.T) {
tests := []CRDTest{{
name: "valid",
Expand Down
9 changes: 8 additions & 1 deletion pkg/apis/eventing/v1alpha1/subscription_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,18 @@ type SubscriberSpec struct {
// +optional
Ref *corev1.ObjectReference `json:"ref,omitempty"`

// Deprecated: Use URI instead.
// Reference to a 'known' endpoint where no resolving is done.
// http://k8s-service for example
// http://myexternalhandler.example.com/foo/bar
// +optional
DNSName *string `json:"dnsName,omitempty"`
DeprecatedDNSName *string `json:"dnsName,omitempty"`
Copy link
Copy Markdown
Member

@matzew matzew Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when will it be removed? the DeprecatedDNSName ?

Copy link
Copy Markdown
Contributor Author

@Harwayne Harwayne Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#993 tracks removing it. The plan is to leave it deprecated for one release. Then remove it in the next. So if this PR merges into 0.5.0, we would remove it in 0.6.0.


// Reference to a 'known' endpoint where no resolving is done.
// http://k8s-service for example
// http://myexternalhandler.example.com/foo/bar
// +optional
URI *string `json:"uri,omitempty"`
}

// ReplyStrategy specifies the handling of the SubscriberSpec's returned replies.
Expand Down
34 changes: 27 additions & 7 deletions pkg/apis/eventing/v1alpha1/subscription_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ func (s *Subscription) Validate(ctx context.Context) *apis.FieldError {
return s.Spec.Validate(ctx).ViaField("spec")
}

// We require always Channel
// Also at least one of 'subscriber' and 'reply' must be defined (non-nill and non-empty)
func (ss *SubscriptionSpec) Validate(ctx context.Context) *apis.FieldError {
// We require always Channel.
// Also at least one of 'subscriber' and 'reply' must be defined (non-nil and non-empty).

var errs *apis.FieldError
if isChannelEmpty(ss.Channel) {
fe := apis.ErrMissingField("channel")
Expand Down Expand Up @@ -66,15 +67,34 @@ func (ss *SubscriptionSpec) Validate(ctx context.Context) *apis.FieldError {
}

func isSubscriberSpecNilOrEmpty(s *SubscriberSpec) bool {
return s == nil || equality.Semantic.DeepEqual(s, &SubscriberSpec{}) ||
(equality.Semantic.DeepEqual(s.Ref, &corev1.ObjectReference{}) && s.DNSName == nil)

if s == nil || equality.Semantic.DeepEqual(s, &SubscriberSpec{}) {
return true
}
if equality.Semantic.DeepEqual(s.Ref, &corev1.ObjectReference{}) &&
s.DeprecatedDNSName == nil &&
s.URI == nil {
return true
}
return false
}

func isValidSubscriberSpec(s SubscriberSpec) *apis.FieldError {
var errs *apis.FieldError
if s.DNSName != nil && *s.DNSName != "" && s.Ref != nil && !equality.Semantic.DeepEqual(s.Ref, &corev1.ObjectReference{}) {
errs = errs.Also(apis.ErrMultipleOneOf("ref", "dnsName"))

fieldsSet := make([]string, 0, 0)
if s.Ref != nil && !equality.Semantic.DeepEqual(s.Ref, &corev1.ObjectReference{}) {
fieldsSet = append(fieldsSet, "ref")
}
if s.DeprecatedDNSName != nil && *s.DeprecatedDNSName != "" {
fieldsSet = append(fieldsSet, "dnsName")
Copy link
Copy Markdown
Member

@matzew matzew Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it populate the uri ?

Copy link
Copy Markdown
Contributor Author

@Harwayne Harwayne Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hesitation for doing that is code like our Trigger controller, which will compare the spec, if it differs, it will delete the Subscription and create a new one. Our Trigger controller wouldn't hit that problem because it uses ref, not dnsName, but I could see a similar controller hitting the problem.

What do you think? Is it worth the potential trouble to move things for the user?

Copy link
Copy Markdown
Member

@matzew matzew Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fair point w/ the Trigger controller.

So, no need to "adjust" it

}
if s.URI != nil && *s.URI != "" {
fieldsSet = append(fieldsSet, "uri")
}
if len(fieldsSet) == 0 {
errs = errs.Also(apis.ErrMissingOneOf("ref", "dnsName", "uri"))
} else if len(fieldsSet) > 1 {
errs = errs.Also(apis.ErrMultipleOneOf(fieldsSet...))
}

// If Ref given, check the fields.
Expand Down
63 changes: 60 additions & 3 deletions pkg/apis/eventing/v1alpha1/subscription_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,8 @@ func TestValidChannel(t *testing.T) {
}

func TestValidgetValidSubscriber(t *testing.T) {
dnsName := "example.com"
uri := "http://example.com"
empty := ""
tests := []struct {
name string
s SubscriberSpec
Expand All @@ -521,7 +522,7 @@ func TestValidgetValidSubscriber(t *testing.T) {
}, {
name: "valid dnsName",
s: SubscriberSpec{
DNSName: &dnsName,
DeprecatedDNSName: &uri,
},
want: nil,
}, {
Expand All @@ -532,12 +533,68 @@ func TestValidgetValidSubscriber(t *testing.T) {
APIVersion: channelAPIVersion,
Kind: channelKind,
},
DNSName: &dnsName,
DeprecatedDNSName: &uri,
},
want: func() *apis.FieldError {
fe := apis.ErrMultipleOneOf("ref", "dnsName")
return fe
}(),
}, {
name: "valid uri",
s: SubscriberSpec{
URI: &uri,
},
want: nil,
}, {
name: "both ref and uri given",
s: SubscriberSpec{
Ref: &corev1.ObjectReference{
Name: channelName,
APIVersion: channelAPIVersion,
Kind: channelKind,
},
URI: &uri,
},
want: func() *apis.FieldError {
fe := apis.ErrMultipleOneOf("ref", "uri")
return fe
}(),
}, {
name: "both dnsName and uri given",
s: SubscriberSpec{
DeprecatedDNSName: &uri,
URI: &uri,
},
want: func() *apis.FieldError {
fe := apis.ErrMultipleOneOf("dnsName", "uri")
return fe
}(),
}, {
name: "ref, dnsName, and uri given",
s: SubscriberSpec{
Ref: &corev1.ObjectReference{
Name: channelName,
APIVersion: channelAPIVersion,
Kind: channelKind,
},
DeprecatedDNSName: &uri,
URI: &uri,
},
want: func() *apis.FieldError {
fe := apis.ErrMultipleOneOf("ref", "dnsName", "uri")
return fe
}(),
}, {
name: "empty ref, dnsName, and uri given",
s: SubscriberSpec{
Ref: &corev1.ObjectReference{},
DeprecatedDNSName: &empty,
URI: &empty,
},
want: func() *apis.FieldError {
fe := apis.ErrMissingOneOf("ref", "dnsName", "uri")
return fe
}(),
}, {
name: "missing name in ref",
s: SubscriberSpec{
Expand Down
13 changes: 11 additions & 2 deletions pkg/apis/eventing/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/broker/broker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ func makeDifferentSubscription() *v1alpha1.Subscription {
s := makeTestSubscription()
s.Spec.Subscriber.Ref = nil
url := "http://example.com/"
s.Spec.Subscriber.DNSName = &url
s.Spec.Subscriber.URI = &url
return s
}

Expand Down
7 changes: 5 additions & 2 deletions pkg/utils/resolve/subscriber.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,11 @@ func SubscriberSpec(ctx context.Context, dynamicClient dynamic.Interface, namesp
if isNilOrEmptySubscriber(s) {
return "", nil
}
if s.DNSName != nil && *s.DNSName != "" {
return *s.DNSName, nil
if s.URI != nil && *s.URI != "" {
return *s.URI, nil
}
if s.DeprecatedDNSName != nil && *s.DeprecatedDNSName != "" {
return *s.DeprecatedDNSName, nil
}

obj, err := ObjectReference(ctx, dynamicClient, namespace, s.Ref)
Expand Down
12 changes: 9 additions & 3 deletions pkg/utils/resolve/subscriber_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const (
)

var (
dnsName = "dns-name"
uri = "http://example.com"

channelAddress = "test-channel.hostname"
channelURL = fmt.Sprintf("http://%s/", channelAddress)
Expand Down Expand Up @@ -108,9 +108,15 @@ func TestSubscriberSpec(t *testing.T) {
},
"DNS Name": {
Sub: &v1alpha1.SubscriberSpec{
DNSName: &dnsName,
DeprecatedDNSName: &uri,
},
Expected: dnsName,
Expected: uri,
},
"URI": {
Sub: &v1alpha1.SubscriberSpec{
URI: &uri,
},
Expected: uri,
},
"Doesn't exist": {
Sub: &v1alpha1.SubscriberSpec{
Expand Down