Generate names with kmeta.ChildName. Delete deprecated named resources

pkg/reconciler/apiserversource/apiserversource.go

@@ -37,6 +37,7 @@ import (
 	listers "knative.dev/eventing/pkg/client/listers/sources/v1alpha1"
 	"knative.dev/eventing/pkg/logging"
 	"knative.dev/eventing/pkg/reconciler/apiserversource/resources"
+	"knative.dev/eventing/pkg/utils"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 	duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
 	"knative.dev/pkg/controller"
@@ -50,6 +51,7 @@ const (
 	// Name of the corev1.Events emitted from the reconciliation process
 	apiserversourceDeploymentCreated = "ApiServerSourceDeploymentCreated"
 	apiserversourceDeploymentUpdated = "ApiServerSourceDeploymentUpdated"
+	apiserversourceDeploymentDeleted = "ApiServerSourceDeploymentDeleted"
 
 	component = "apiserversource"
 )
@@ -170,6 +172,15 @@ func (r *Reconciler) createReceiveAdapter(ctx context.Context, src *v1alpha1.Api
 
 	ra, err := r.kubeClientSet.AppsV1().Deployments(src.Namespace).Get(expected.Name, metav1.GetOptions{})
 	if apierrors.IsNotFound(err) {
+		// Issue #2842: Adater deployment name uses kmeta.ChildName. If a deployment by the previous name pattern is found, it should
+		// be deleted. This might cause temporary downtime.
+		if deprecatedName := utils.GenerateFixedName(adapterArgs.Source, fmt.Sprintf("apiserversource-%s", adapterArgs.Source.Name)); deprecatedName != expected.Name {
+			if err := r.kubeClientSet.AppsV1().Deployments(src.Namespace).Delete(deprecatedName, &metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+				return nil, fmt.Errorf("error deleting deprecated named deployment: %v", err)
+			}
+			controller.GetEventRecorder(ctx).Eventf(src, corev1.EventTypeNormal, apiserversourceDeploymentDeleted, "Deprecated deployment removed: \"%s/%s\"", src.Namespace, deprecatedName)
+		}
+
 		ra, err = r.kubeClientSet.AppsV1().Deployments(src.Namespace).Create(expected)
 		msg := "Deployment created"
 		if err != nil {

pkg/reconciler/apiserversource/apiserversource_test.go

@@ -18,6 +18,7 @@ package apiserversource
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake"
@@ -74,10 +75,11 @@ var (
 )
 
 const (
-	image      = "github.com/knative/test/image"
-	sourceName = "test-apiserver-source"
-	sourceUID  = "1234"
-	testNS     = "testnamespace"
+	image          = "github.com/knative/test/image"
+	sourceName     = "test-apiserver-source"
+	sourceNameLong = "test-apiserver-source-with-a-very-long-name"
+	sourceUID      = "1234"
+	testNS         = "testnamespace"
 
 	sinkName = "testsink"
 	source   = "apiserveraddr"
@@ -581,6 +583,63 @@ func TestReconcile(t *testing.T) {
 		},
 		WithReactors:            []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
 		SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
+	}, {
+		Name: "deprecated named adapter deployment found",
+		Objects: []runtime.Object{
+			NewApiServerSource(sourceNameLong, testNS,
+				WithApiServerSourceSpec(sourcesv1alpha1.ApiServerSourceSpec{
+					Resources: []sourcesv1alpha1.ApiServerResource{{
+						APIVersion: "",
+						Kind:       "Namespace",
+					}},
+					Sink: &sinkDest,
+				}),
+				WithApiServerSourceUID(sourceUID),
+				WithApiServerSourceObjectMetaGeneration(generation),
+			),
+			NewChannel(sinkName, testNS,
+				WithInitChannelConditions,
+				WithChannelAddress(sinkDNS),
+			),
+			makeAvailableReceiveAdapterDeprecatedName(sourceNameLong),
+		},
+		Key: testNS + "/" + sourceNameLong,
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, apiserversourceDeploymentDeleted, `Deprecated deployment removed: "%s/%s"`, testNS, makeAvailableReceiveAdapterDeprecatedName(sourceNameLong).Name),
+			Eventf(corev1.EventTypeNormal, apiserversourceDeploymentCreated, "Deployment created"),
+			Eventf(corev1.EventTypeNormal, "ApiServerSourceReconciled", `ApiServerSource reconciled: "%s/%s"`, testNS, sourceNameLong),
+		},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: NewApiServerSource(sourceNameLong, testNS,
+				WithApiServerSourceSpec(sourcesv1alpha1.ApiServerSourceSpec{
+					Resources: []sourcesv1alpha1.ApiServerResource{{
+						APIVersion: "",
+						Kind:       "Namespace",
+					}},
+					Sink: &sinkDest,
+				}),
+				WithApiServerSourceUID(sourceUID),
+				WithApiServerSourceObjectMetaGeneration(generation),
+				// Status Update:
+				WithInitApiServerSourceConditions,
+				WithApiServerSourceStatusObservedGeneration(generation),
+				WithApiServerSourceSink(sinkURI),
+				WithApiServerSourceSufficientPermissions,
+				WithApiServerSourceEventTypes(source),
+				WithApiServerSourceDeploymentUnavailable,
+			),
+		}},
+		WantDeletes: []clientgotesting.DeleteActionImpl{{
+			Name: makeAvailableReceiveAdapterDeprecatedName(sourceNameLong).Name,
+		}},
+		WantCreates: []runtime.Object{
+			makeSubjectAccessReview("namespaces", "get", "default"),
+			makeSubjectAccessReview("namespaces", "list", "default"),
+			makeSubjectAccessReview("namespaces", "watch", "default"),
+			makeReceiveAdapterWithName(sourceNameLong),
+		},
+		WithReactors:            []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
+		SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
 	}}
 
 	logger := logtesting.TestLogger(t)
@@ -603,6 +662,10 @@ func TestReconcile(t *testing.T) {
 }
 
 func makeReceiveAdapter() *appsv1.Deployment {
+	return makeReceiveAdapterWithName(sourceName)
+}
+
+func makeReceiveAdapterWithName(sourceName string) *appsv1.Deployment {
 	src := NewApiServerSource(sourceName, testNS,
 		WithApiServerSourceSpec(sourcesv1alpha1.ApiServerSourceSpec{
 			Resources: []sourcesv1alpha1.ApiServerResource{{
@@ -633,6 +696,16 @@ func makeAvailableReceiveAdapter() *appsv1.Deployment {
 	return ra
 }
 
+// makeAvailableReceiveAdapterDeprecatedName needed to simulate pre 0.14 adapter whose name was generated using utils.GenerateFixedName
+func makeAvailableReceiveAdapterDeprecatedName(sourceName string) *appsv1.Deployment {
+	ra := makeReceiveAdapter()
+	src := &sourcesv1alpha1.ApiServerSource{}
+	src.UID = sourceUID
+	ra.Name = utils.GenerateFixedName(src, fmt.Sprintf("apiserversource-%s", sourceName))
+	WithDeploymentAvailable()(ra)
+	return ra
+}
+
 func makeAvailableReceiveAdapterWithTargetURI() *appsv1.Deployment {
 	src := NewApiServerSource(sourceName, testNS,
 		WithApiServerSourceSpec(sourcesv1alpha1.ApiServerSourceSpec{

pkg/reconciler/apiserversource/resources/receive_adapter.go

@@ -23,7 +23,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/eventing/pkg/apis/sources/v1alpha1"
-	"knative.dev/eventing/pkg/utils"
 	"knative.dev/pkg/kmeta"
 )
 
@@ -45,7 +44,7 @@ func MakeReceiveAdapter(args *ReceiveAdapterArgs) *v1.Deployment {
 	return &v1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: args.Source.Namespace,
-			Name:      utils.GenerateFixedName(args.Source, fmt.Sprintf("apiserversource-%s", args.Source.Name)),
+			Name:      kmeta.ChildName(fmt.Sprintf("apiserversource-%s-", args.Source.Name), string(args.Source.GetUID())),
 			Labels:    args.Labels,
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(args.Source),

pkg/reconciler/apiserversource/resources/receive_adapter_test.go

@@ -25,6 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/eventing/pkg/apis/sources/v1alpha1"
+	"knative.dev/pkg/kmeta"
 	_ "knative.dev/pkg/metrics/testing"
 )
 
@@ -100,7 +101,7 @@ func TestMakeReceiveAdapter(t *testing.T) {
 	want := &v1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "source-namespace",
-			Name:      fmt.Sprintf("apiserversource-%s-1234", name),
+			Name:      kmeta.ChildName(fmt.Sprintf("apiserversource-%s-", name), string(src.UID)),
 			Labels: map[string]string{
 				"test-key1": "test-value1",
 				"test-key2": "test-value2",

pkg/reconciler/broker/broker_test.go

@@ -79,9 +79,11 @@ const (
 	filterContainerName  = "filter"
 	ingressContainerName = "ingress"
 
-	triggerChannel channelType = "TriggerChannel"
-	triggerName                = "test-trigger"
-	triggerUID                 = "test-trigger-uid"
+	triggerChannel  channelType = "TriggerChannel"
+	triggerName                 = "test-trigger"
+	triggerUID                  = "test-trigger-uid"
+	triggerNameLong             = "test-trigger-name-is-a-long-name"
+	triggerUIDLong              = "cafed00d-cafed00d-cafed00d-cafed00d-cafed00d"
 
 	subscriberURI     = "http://example.com/subscriber/"
 	subscriberKind    = "Service"
@@ -1464,6 +1466,40 @@ func TestReconcile(t *testing.T) {
 					WithTriggerDependencyReady(),
 				),
 			}},
+		}, {
+			Name: "Trigger has deprecated named subscriber",
+			Key:  testKey,
+			Objects: allBrokerObjectsReadyPlus([]runtime.Object{
+				makeReadySubscriptionDeprecatedName(triggerNameLong, triggerUIDLong),
+				NewTrigger(triggerNameLong, testNS, brokerName,
+					WithTriggerUID(triggerUIDLong),
+					WithTriggerSubscriberURI(subscriberURI),
+					WithInitTriggerConditions,
+				)}...),
+			WantErr: false,
+			WantEvents: []string{
+				Eventf(corev1.EventTypeNormal, subscriptionDeleted, `Deprecated subscription removed: "%s/%s"`, testNS, makeReadySubscriptionDeprecatedName(triggerNameLong, triggerUIDLong).Name),
+				Eventf(corev1.EventTypeNormal, "TriggerReconciled", "Trigger reconciled"),
+			},
+			WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+				Object: NewTrigger(triggerNameLong, testNS, brokerName,
+					WithTriggerUID(triggerUIDLong),
+					WithTriggerSubscriberURI(subscriberURI),
+					// The first reconciliation will initialize the status conditions.
+					WithInitTriggerConditions,
+					WithTriggerBrokerReady(),
+					WithTriggerSubscribedUnknown("SubscriptionNotConfigured", "Subscription has not yet been reconciled."),
+					WithTriggerStatusSubscriberURI(subscriberURI),
+					WithTriggerSubscriberResolvedSucceeded(),
+					WithTriggerDependencyReady(),
+				),
+			}},
+			WantCreates: []runtime.Object{
+				makeIngressSubscriptionWithCustomData(triggerNameLong, triggerUIDLong),
+			},
+			WantDeletes: []clientgotesting.DeleteActionImpl{{
+				Name: makeReadySubscriptionDeprecatedName(triggerNameLong, triggerUIDLong).Name,
+			}},
 		},
 	}
 
@@ -1740,6 +1776,17 @@ func makeIngressSubscription() *messagingv1alpha1.Subscription {
 	return resources.NewSubscription(makeTrigger(), createTriggerChannelRef(), makeBrokerRef(), makeServiceURI(), makeEmptyDelivery())
 }
 
+func makeIngressSubscriptionWithCustomData(triggerName, triggerUID string) *messagingv1alpha1.Subscription {
+	t := makeTrigger()
+	t.Name = triggerName
+	t.UID = types.UID(triggerUID)
+
+	uri := makeServiceURI()
+	uri.Path = fmt.Sprintf("/triggers/%s/%s/%s", testNS, triggerName, triggerUID)
+
+	return resources.NewSubscription(t, createTriggerChannelRef(), makeBrokerRef(), uri, makeEmptyDelivery())
+}
+
 func makeTrigger() *v1alpha1.Trigger {
 	return &v1alpha1.Trigger{
 		TypeMeta: metav1.TypeMeta{
@@ -1867,6 +1914,15 @@ func makeReadySubscription() *messagingv1alpha1.Subscription {
 	return s
 }
 
+func makeReadySubscriptionDeprecatedName(triggerName, triggerUID string) *messagingv1alpha1.Subscription {
+	s := makeIngressSubscription()
+	t := NewTrigger(triggerName, testNS, brokerName)
+	t.UID = types.UID(triggerUID)
+	s.Name = utils.GenerateFixedName(t, fmt.Sprintf("%s-%s", brokerName, triggerName))
+	s.Status = *v1alpha1.TestHelper.ReadySubscriptionStatus()
+	return s
+}
+
 func makeSubscriberAddressableAsUnstructured() *unstructured.Unstructured {
 	return &unstructured.Unstructured{
 		Object: map[string]interface{}{

pkg/reconciler/broker/resources/subscription.go

@@ -29,7 +29,6 @@ import (
 	"knative.dev/eventing/pkg/apis/eventing"
 	"knative.dev/eventing/pkg/apis/eventing/v1alpha1"
 	messagingv1alpha1 "knative.dev/eventing/pkg/apis/messaging/v1alpha1"
-	"knative.dev/eventing/pkg/utils"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 )
 
@@ -38,7 +37,7 @@ func MakeSubscription(b *v1alpha1.Broker, c *duckv1alpha1.Channelable, svc *core
 	return &messagingv1alpha1.Subscription{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: b.Namespace,
-			Name:      utils.GenerateFixedName(b, fmt.Sprintf("internal-ingress-%s", b.Name)),
+			Name:      kmeta.ChildName(fmt.Sprintf("internal-ingress-%s", b.Name), string(b.GetUID())),
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(b),
 			},
@@ -75,7 +74,7 @@ func NewSubscription(t *v1alpha1.Trigger, brokerTrigger, brokerRef *corev1.Objec
 	return &messagingv1alpha1.Subscription{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: t.Namespace,
-			Name:      utils.GenerateFixedName(t, fmt.Sprintf("%s-%s", t.Spec.Broker, t.Name)),
+			Name:      kmeta.ChildName(fmt.Sprintf("%s-%s-", t.Spec.Broker, t.Name), string(t.GetUID())),
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(t),
 			},

pkg/reconciler/broker/resources/subscription_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resources
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -29,6 +30,7 @@ import (
 	messagingv1alpha1 "knative.dev/eventing/pkg/apis/messaging/v1alpha1"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/kmeta"
 )
 
 func TestMakeSubscription(t *testing.T) {
@@ -106,7 +108,8 @@ func TestNewSubscription(t *testing.T) {
 	trigger := &v1alpha1.Trigger{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "t-namespace",
-			Name:      "t-name",
+			Name:      "t-name-is-a-long-name",
+			UID:       "cafed00d-cafed00d-cafed00d-cafed00d",
 		},
 		Spec: v1alpha1.TriggerSpec{
 			Broker: "broker-name",
@@ -132,17 +135,18 @@ func TestNewSubscription(t *testing.T) {
 	want := &messagingv1alpha1.Subscription{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: "t-namespace",
-			Name:      "broker-name-t-name-",
+			Name:      kmeta.ChildName(fmt.Sprintf("%s-%s-", "broker-name", "t-name-is-a-long-name"), "cafed00d-cafed00d-cafed00d-cafed00d"),
 			OwnerReferences: []metav1.OwnerReference{{
 				APIVersion:         "eventing.knative.dev/v1alpha1",
 				Kind:               "Trigger",
-				Name:               "t-name",
+				Name:               "t-name-is-a-long-name",
+				UID:                "cafed00d-cafed00d-cafed00d-cafed00d",
 				Controller:         &TrueValue,
 				BlockOwnerDeletion: &TrueValue,
 			}},
 			Labels: map[string]string{
 				eventing.BrokerLabelKey:        "broker-name",
-				"eventing.knative.dev/trigger": "t-name",
+				"eventing.knative.dev/trigger": "t-name-is-a-long-name",
 			},
 		},
 		Spec: messagingv1alpha1.SubscriptionSpec{

pkg/reconciler/broker/trigger.go

@@ -35,6 +35,7 @@ import (
 	"knative.dev/eventing/pkg/reconciler/broker/resources"
 	"knative.dev/eventing/pkg/reconciler/names"
 	"knative.dev/eventing/pkg/reconciler/trigger/path"
+	"knative.dev/eventing/pkg/utils"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
 	"knative.dev/pkg/kmeta"
@@ -48,6 +49,7 @@ const (
 	subscriptionDeleteFailed  = "SubscriptionDeleteFailed"
 	subscriptionCreateFailed  = "SubscriptionCreateFailed"
 	subscriptionGetFailed     = "SubscriptionGetFailed"
+	subscriptionDeleted       = "SubscriptionDeleted"
 )
 
 func (r *Reconciler) reconcileTrigger(ctx context.Context, b *v1alpha1.Broker, t *v1alpha1.Trigger, filterSvc kmeta.Accessor) error {
@@ -121,6 +123,15 @@ func (r *Reconciler) subscribeToBrokerChannel(ctx context.Context, b *v1alpha1.B
 	sub, err := r.subscriptionLister.Subscriptions(t.Namespace).Get(expected.Name)
 	// If the resource doesn't exist, we'll create it.
 	if apierrs.IsNotFound(err) {
+		// Issue #2842: Subscription name uses kmeta.ChildName. If a subscription by the previous name pattern is found, it should
+		// be deleted. This might cause temporary downtime.
+		if deprecatedName := utils.GenerateFixedName(t, fmt.Sprintf("%s-%s", t.Spec.Broker, t.Name)); deprecatedName != expected.Name {
+			if err := r.eventingClientSet.MessagingV1alpha1().Subscriptions(t.Namespace).Delete(deprecatedName, &metav1.DeleteOptions{}); err != nil && !apierrs.IsNotFound(err) {
+				return nil, fmt.Errorf("error deleting deprecated named subscription: %v", err)
+			}
+			controller.GetEventRecorder(ctx).Eventf(t, corev1.EventTypeNormal, subscriptionDeleted, "Deprecated subscription removed: \"%s/%s\"", t.Namespace, deprecatedName)
+		}
+
 		logging.FromContext(ctx).Info("Creating subscription")
 		sub, err = r.eventingClientSet.MessagingV1alpha1().Subscriptions(t.Namespace).Create(expected)
 		if err != nil {