Populate a Subscriptions subscriber and reply namespace only if not set already

[creydr]

Fixes #6539

In case a Subscription with a .spec.subscriber.ref pointing to a service in another namespace, the Subscription will not become ready because it is trying to find the service on the subscription's namespace (see #6539). Same happens with a subscriptions .spec.reply.ref.
This PR addresses it and only uses the namespace of the Subscription, if no namespace was given for the subscriber/reply.

Proposed Changes

• 🐛 Populate a subscriptions subscriber & reply namespace field with the Subscriptions namespace only in case it is not set

Pre-review Checklist

• At least 80% unit test coverage
• E2E tests for any new behavior
• Docs PR for any user-facing impact
• Spec PR for any new API feature
• Conformance test for any change to the spec

Release Note

How to verify

1. Install a eventing with this patch
2. Create a sequence with steps and a reply in different namespaces (e.g. https://gist.github.com/creydr/57235f35b03ab8f1285eee0a64350576)
3. Check that the subscriptions and sequence becomes ready

[codecov]

Codecov Report

Base: 80.60% // Head: 80.55% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (d9affc4) compared to base (9e692b8).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6671      +/-   ##
==========================================
- Coverage   80.60%   80.55%   -0.05%     
==========================================
  Files         236      236              
  Lines       12018    12018              
==========================================
- Hits         9687     9681       -6     
- Misses       1851     1855       +4     
- Partials      480      482       +2     

Impacted Files	Coverage Δ
pkg/reconciler/subscription/subscription.go	81.74% <50.00%> (-1.59%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gab-satchi]

I see that the defaulting was always happening here but it may be better to move it to the defaulting webhook for subscriptions.

[creydr]

Thanks for the review. I agree the defaulting webhook would be a better place. Let me move it...

[pierDipi]

This defaulting should be kept here for backward compatibility since existing resources won't have it defaulted until an update happens

[creydr]

@pierDipi: So we don't we resync the resources after a couple of time?

[pierDipi]

resync is not necessarily producing an update (knative/pkg tries to reduce updates as much as possible to reduce API server load) therefore the webhook is not always applied

[creydr]

what I meant is that even with the current code (before this PR), the namespace was set (latest on a periodic resync) on the resource. So my chance from this PR (being in a webhook or not) would not change anything IIUC. My PR would only change anything on resources which don't have the namespace set on reply/subscribers - which should only be the case for new resources. Or am I missing something?

[pierDipi]

This is only a status update:

eventing/pkg/client/injection/reconciler/messaging/v1/subscription/reconciler.go

Lines 311 to 341 in d418419

	func (r *reconcilerImpl) updateStatus(ctx context.Context, existing *v1.Subscription, desired *v1.Subscription) error {
	existing = existing.DeepCopy()
	return reconciler.RetryUpdateConflicts(func(attempts int) (err error) {
	// The first iteration tries to use the injectionInformer's state, subsequent attempts fetch the latest state via API.
	if attempts > 0 {
	getter := r.Client.MessagingV1().Subscriptions(desired.Namespace)
	existing, err = getter.Get(ctx, desired.Name, metav1.GetOptions{})
	if err != nil {
	return err
	}
	}
	// If there's nothing to update, just return.
	if equality.Semantic.DeepEqual(existing.Status, desired.Status) {
	return nil
	}
	if diff, err := kmp.SafeDiff(existing.Status, desired.Status); err == nil && diff != "" {
	logging.FromContext(ctx).Debug("Updating status with: ", diff)
	}
	existing.Status = desired.Status
	updater := r.Client.MessagingV1().Subscriptions(existing.Namespace)
	_, err = updater.UpdateStatus(ctx, existing, metav1.UpdateOptions{})
	return err
	})
	}

, so spec changes in ReconcileKind are not persisted.

In addition, on upgrade controller and webhook are not atomically updated at the same time, so during this time period we might not have this logic running anywhere

[creydr]

I think I got the point. This logic is only operating on a copy of the subscription. The sequence webhook sets the namespace correctly, but it gets overwritten by the subscription reconciler. As the subscription reconciler needs the namespace of the ref, it must be set - and since we can create subscriptions directly without a namespace for the ref this is an issue (and there might be some out there without a ref.namespace already).

[creydr]

/assign @pierDipi @gab-satchi

[pierDipi]

We can make the webhook addition as a follow up PR

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[creydr]

/test upgrade-tests