Skip to content

[WIP] [release-v1.11] Alternative fix for eventtype create-delete loop on built in sources #546

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

Closed
pierDipi wants to merge 4 commits into from
Closed

[WIP] [release-v1.11] Alternative fix for eventtype create-delete loop on built in sources #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a36a278
Revert "Fixed eventtype create-delete loop on built in sources (#7245)"
pierDipi Feb 26, 2024
16491c0
Alternative fix for eventtype create-delete loop on built in sources
pierDipi Feb 26, 2024
975a434
Use DeepDerivative
pierDipi Feb 26, 2024
0302594
Add and fix tests
pierDipi Feb 26, 2024
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
10 changes: 9 additions & 1 deletion pkg/apis/eventing/v1beta1/eventtype_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,22 @@ limitations under the License.

package v1beta1

import "context"
import (
"context"

"knative.dev/pkg/apis"
)

func (et *EventType) SetDefaults(ctx context.Context) {
ctx = apis.WithinParent(ctx, et.ObjectMeta)
et.Spec.SetDefaults(ctx)
}

func (ets *EventTypeSpec) SetDefaults(ctx context.Context) {
if ets.Reference == nil && ets.Broker == "" {
ets.Broker = "default"
}
if ets.Reference != nil {
ets.Reference.SetDefaults(ctx)
}
}
11 changes: 5 additions & 6 deletions pkg/apis/eventing/v1beta2/eventtype_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,17 @@ package v1beta2

import (
"context"

"knative.dev/pkg/apis"
)

func (et *EventType) SetDefaults(ctx context.Context) {
ctx = apis.WithinParent(ctx, et.ObjectMeta)
et.Spec.SetDefaults(ctx)
setReferenceNs(et)
}

func (ets *EventTypeSpec) SetDefaults(ctx context.Context) {
}

func setReferenceNs(et *EventType) {
if et.Spec.Reference != nil && et.Spec.Reference.Namespace == "" {
et.Spec.Reference.Namespace = et.GetNamespace()
if ets.Reference != nil {
ets.Reference.SetDefaults(ctx)
}
}
7 changes: 0 additions & 7 deletions pkg/reconciler/apiserversource/apiserversource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, source *v1.ApiServerSour
return nil
}

func (r *Reconciler) FinalizeKind(ctx context.Context, source *v1.ApiServerSource) pkgreconciler.Event {
logging.FromContext(ctx).Info("Deleting source")
// Allow for eventtypes to be cleaned up
source.Status.CloudEventAttributes = []duckv1.CloudEventAttributes{}
return nil
}
Comment on lines -144 to -149
Copy link
Copy Markdown
Member

@Cali0707 Cali0707 Feb 26, 2024

Choose a reason for hiding this comment

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

@pierDipi I don't quite understand why we are deleting this, would you be able to explain?

Copy link
Copy Markdown
Member Author

@pierDipi pierDipi Feb 26, 2024

Choose a reason for hiding this comment

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

I don't get why we need this knative#7245 (comment)


func (r *Reconciler) namespacesFromSelector(src *v1.ApiServerSource) ([]string, error) {
if src.Spec.NamespaceSelector == nil {
return []string{src.Namespace}, nil
Expand Down
71 changes: 0 additions & 71 deletions pkg/reconciler/apiserversource/apiserversource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,8 @@ func TestReconcile(t *testing.T) {
},
WantErr: true,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeWarning, "InternalError", `insufficient permissions: User system:serviceaccount:testnamespace:default cannot get, list, watch resource "namespaces" in API group "" in Namespace "testnamespace"`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(false)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand Down Expand Up @@ -199,12 +195,6 @@ func TestReconcile(t *testing.T) {
makeSubjectAccessReview("namespaces", "list", "default"),
makeSubjectAccessReview("namespaces", "watch", "default"),
},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand Down Expand Up @@ -266,12 +256,8 @@ func TestReconcile(t *testing.T) {
Object: makeAvailableReceiveAdapterWithNamespaces(t, []string{"test-a", "test-b"}, false),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, "ApiServerSourceDeploymentUpdated", `Deployment "apiserversource-test-apiserver-source-1234" updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand Down Expand Up @@ -336,12 +322,8 @@ func TestReconcile(t *testing.T) {
Object: makeAvailableReceiveAdapterWithNamespaces(t, []string{"test-a", "test-b", "test-c"}, true),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, "ApiServerSourceDeploymentUpdated", `Deployment "apiserversource-test-apiserver-source-1234" updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand Down Expand Up @@ -393,12 +375,6 @@ func TestReconcile(t *testing.T) {
makeSubjectAccessReview("namespaces", "list", "default"),
makeSubjectAccessReview("namespaces", "watch", "default"),
},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand Down Expand Up @@ -448,12 +424,6 @@ func TestReconcile(t *testing.T) {
makeSubjectAccessReview("namespaces", "list", "default"),
makeSubjectAccessReview("namespaces", "watch", "default"),
},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand All @@ -473,13 +443,9 @@ func TestReconcile(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeWarning, "SinkNotFound",
`Sink not found: {"ref":{"kind":"Channel","namespace":"testnamespace","name":"testsink","apiVersion":"messaging.knative.dev/v1"}}`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rttestingv1.NewApiServerSource(sourceName, testNS,
rttestingv1.WithApiServerSourceSpec(sourcesv1.ApiServerSourceSpec{
Expand Down Expand Up @@ -521,15 +487,11 @@ func TestReconcile(t *testing.T) {
Key: testNS + "/" + sourceName,
WantErr: true,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, apiserversourceDeploymentCreated,
"Deployment created, error:inducing failure for create deployments"),
Eventf(corev1.EventTypeWarning, "InternalError",
"inducing failure for create deployments"),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rttestingv1.NewApiServerSource(sourceName, testNS,
rttestingv1.WithApiServerSourceSpec(sourcesv1.ApiServerSourceSpec{
Expand Down Expand Up @@ -618,12 +580,6 @@ func TestReconcile(t *testing.T) {
makeSubjectAccessReview("namespaces", "list", "default"),
makeSubjectAccessReview("namespaces", "watch", "default"),
},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WithReactors: []clientgotesting.ReactionFunc{subjectAccessReviewCreateReactor(true)},
SkipNamespaceValidation: true, // SubjectAccessReview objects are cluster-scoped.
}, {
Expand All @@ -648,12 +604,8 @@ func TestReconcile(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, "ApiServerSourceDeploymentUpdated", `Deployment "apiserversource-test-apiserver-source-1234" updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rttestingv1.NewApiServerSource(sourceName, testNS,
rttestingv1.WithApiServerSourceSpec(sourcesv1.ApiServerSourceSpec{
Expand Down Expand Up @@ -710,12 +662,8 @@ func TestReconcile(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, "ApiServerSourceDeploymentUpdated", `Deployment "apiserversource-test-apiserver-source-1234" updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rttestingv1.NewApiServerSource(sourceName, testNS,
rttestingv1.WithApiServerSourceSpec(sourcesv1.ApiServerSourceSpec{
Expand Down Expand Up @@ -772,12 +720,8 @@ func TestReconcile(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, "ApiServerSourceDeploymentUpdated", `Deployment "apiserversource-test-apiserver-source-1234" updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rttestingv1.NewApiServerSource(sourceName, testNS,
rttestingv1.WithApiServerSourceSpec(sourcesv1.ApiServerSourceSpec{
Expand Down Expand Up @@ -851,12 +795,6 @@ func TestReconcile(t *testing.T) {
rttestingv1.WithApiServerSourceStatusNamespaces([]string{testNS}),
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantCreates: []runtime.Object{
makeSubjectAccessReview("namespaces", "get", "default"),
makeSubjectAccessReview("namespaces", "list", "default"),
Expand Down Expand Up @@ -1083,12 +1021,3 @@ func subjectAccessReviewCreateReactor(allowed bool) clientgotesting.ReactionFunc
return false, nil, nil
}
}

func patchFinalizers(name, namespace string) clientgotesting.PatchActionImpl {
action := clientgotesting.PatchActionImpl{}
action.Name = name
action.Namespace = namespace
patch := `{"metadata":{"finalizers":["apiserversources.sources.knative.dev"],"resourceVersion":""}}`
action.Patch = []byte(patch)
return action
}
7 changes: 0 additions & 7 deletions pkg/reconciler/pingsource/pingsource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, source *sourcesv1.PingSo
return nil
}

func (r *Reconciler) FinalizeKind(ctx context.Context, source *sourcesv1.PingSource) pkgreconciler.Event {
logging.FromContext(ctx).Info("Deleting source")
// Allow for eventtypes to be cleaned up
source.Status.CloudEventAttributes = []duckv1.CloudEventAttributes{}
return nil
}

func (r *Reconciler) reconcileReceiveAdapter(ctx context.Context, source *sourcesv1.PingSource) (*appsv1.Deployment, error) {
args := resources.Args{
ConfigEnvVars: r.configAcc.ToEnvVars(),
Expand Down
49 changes: 0 additions & 49 deletions pkg/reconciler/pingsource/pingsource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,13 +144,9 @@ func TestAllCases(t *testing.T) {
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeWarning, "SinkNotFound",
`Sink not found: {"ref":{"kind":"Channel","namespace":"testnamespace","name":"testsink","apiVersion":"messaging.knative.dev/v1"}}`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
}, {
Name: "sink ref has no namespace",
Objects: []runtime.Object{
Expand Down Expand Up @@ -187,13 +183,9 @@ func TestAllCases(t *testing.T) {
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeWarning, "SinkNotFound",
`Sink not found: {"ref":{"kind":"Channel","namespace":"testnamespace","name":"testsink","apiVersion":"messaging.knative.dev/v1"}}`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
}, {
Name: "error creating deployment",
Objects: []runtime.Object{
Expand All @@ -216,12 +208,8 @@ func TestAllCases(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeWarning, "InternalError", "deployments.apps \"pingsource-mt-adapter\" not found"),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantErr: true,
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rtv1.NewPingSource(sourceName, testNS,
Expand Down Expand Up @@ -283,12 +271,6 @@ func TestAllCases(t *testing.T) {
rtv1.WithPingSourceStatusObservedGeneration(generation),
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
}, {
Name: "Propagate CA certs",
Objects: []runtime.Object{
Expand Down Expand Up @@ -339,12 +321,6 @@ func TestAllCases(t *testing.T) {
rtv1.WithPingSourceStatusObservedGeneration(generation),
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
}, {
Name: "deployment update due to env",
Objects: []runtime.Object{
Expand All @@ -368,12 +344,8 @@ func TestAllCases(t *testing.T) {
},
Key: testNS + "/" + sourceName,
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
Eventf(corev1.EventTypeNormal, pingSourceDeploymentUpdated, `PingSource adapter deployment updated`),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
Object: rtv1.NewPingSource(sourceName, testNS,
rtv1.WithPingSourceSpec(sourcesv1.PingSourceSpec{
Expand Down Expand Up @@ -439,12 +411,6 @@ func TestAllCases(t *testing.T) {
rtv1.WithPingSourceStatusObservedGeneration(generation),
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
}, {
Name: "valid with dataBase64",
Objects: []runtime.Object{
Expand Down Expand Up @@ -487,12 +453,6 @@ func TestAllCases(t *testing.T) {
rtv1.WithPingSourceStatusObservedGeneration(generation),
),
}},
WantEvents: []string{
Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName),
},
WantPatches: []clientgotesting.PatchActionImpl{
patchFinalizers(sourceName, testNS),
},
},
}

Expand Down Expand Up @@ -552,12 +512,3 @@ func makeAvailableMTAdapterWithDifferentEnv() *appsv1.Deployment {
WithDeploymentAvailable()(ma)
return ma
}

func patchFinalizers(name, namespace string) clientgotesting.PatchActionImpl {
action := clientgotesting.PatchActionImpl{}
action.Name = name
action.Namespace = namespace
patch := `{"metadata":{"finalizers":["pingsources.sources.knative.dev"],"resourceVersion":""}}`
action.Patch = []byte(patch)
return action
}
Loading