Reconcile event policies for mt-broker #8090

rahulii · 2024-07-09T19:51:21Z

Proposed Changes

🎁 Reconcile Event Policies for MT-Broker
🎁 Create EventPolicy for underlying Channel in case OIDC Authentication is enabled
🎁 Delete orphaned EventPolicies.

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

 Reconcile Event Policies for MT-Broker

Docs

Signed-off-by: rahulii <[email protected]>

knative-prow · 2024-07-09T19:51:24Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Cali0707

nice start @rahulii - I left a few comments on what you have so far (I know it's a draft, but hopefully they help!)

pkg/reconciler/broker/broker.go

… per review comments Signed-off-by: rahulii <[email protected]>

Signed-off-by: rahulii <[email protected]>

codecov · 2024-07-10T11:00:29Z

Codecov Report

Attention: Patch coverage is 66.66667% with 26 lines in your changes missing coverage. Please review.

Project coverage is 67.84%. Comparing base (768f1bd) to head (cc53eba).
Report is 5 commits behind head on main.

Files	Patch %	Lines
pkg/reconciler/broker/broker.go	31.57%	21 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8090      +/-   ##
==========================================
+ Coverage   67.80%   67.84%   +0.04%     
==========================================
  Files         367      368       +1     
  Lines       17373    17505     +132     
==========================================
+ Hits        11779    11877      +98     
- Misses       4859     4885      +26     
- Partials      735      743       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: rahulii <[email protected]>

Cali0707

Hey @rahulii thanks for this great PR! I've left a few comments (one asking for clarification, the rest with tips on how to improve the code), but once we work through those I think we will be good to merge this - great work!!

Cali0707 · 2024-07-10T18:54:44Z

pkg/reconciler/broker/resources/eventpolicy.go

+			},
+			From: []eventingv1alpha1.EventPolicySpecFrom{
+				{
+					Sub: toStrPtr(OIDCBrokerSub),


Instead of using our own implementation here, we normally use the k8s ptr package, so this would become ptr.To(OIDCBrokerSub)

done, PTAL!

Cali0707 · 2024-07-10T18:58:19Z

pkg/reconciler/broker/resources/eventpolicy.go

+	return &eventingv1alpha1.EventPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: backingChannel.Namespace,
+			Name:      b.Name + "-event-policy",


There is a bit more complexity here than just adding the suffix to the name of the broker, since we might run into scenarios where the resulting name exceeds the allowed name length in k8s. Instead, we use kmeta.ChildName(parent, suffix) to generate the names normally. See here: https://pkg.go.dev/knative.dev/pkg/kmeta#ChildName

done, PTAL!

Cali0707 · 2024-07-10T19:00:10Z

pkg/reconciler/broker/resources/eventpolicy.go

+		BackingChannelEventPolicyLabelPrefix + "broker-group":   brokerGroup,
+		BackingChannelEventPolicyLabelPrefix + "broker-version": version,
+		BackingChannelEventPolicyLabelPrefix + "broker-kind":    brokerKind,
+		BackingChannelEventPolicyLabelPrefix + "broker-name":    broker.Name,


I'm curious if we actually need all of these labels for the EventPolicies, what exactly are you using them all for?

@Cali0707 I took reference from channel - https://github.com/knative/eventing/blob/main/pkg/reconciler/channel/resources/eventpolicy.go#L69
Hence, thought of adding the same here. LMK if you think it shouldn't be there, I will remove it !

Yeah looking there it seems like those labels are used to correctly select the EventPolicies which were created by the channel. We should also use the labels to make a label selector when checking which EventPolicies are owned by the broker. See here for how it is used in the channel:

eventing/pkg/reconciler/channel/channel.go

Lines 142 to 166 in bb2e0a3

applyingEventPoliciesForBackingChannel, err := auth.GetEventPoliciesForResource(r.eventPolicyLister, backingChannel.GroupVersionKind(), backingChannel.ObjectMeta)

if err != nil {

return fmt.Errorf("could not get applying EventPolicies for for backing channel %s/%s: %w", channel.Namespace, channel.Name, err)

}

selector, err := labels.ValidatedSelectorFromSet(resources.LabelsForBackingChannelsEventPolicy(backingChannel))

if err != nil {

return fmt.Errorf("could not get valid selector for backing channels EventPolicy %s/%s: %w", backingChannel.Namespace, backingChannel.Name, err)

}

existingEventPoliciesForBackingChannel, err := r.eventPolicyLister.EventPolicies(backingChannel.Namespace).List(selector)

if err != nil {

return fmt.Errorf("could not get existing EventPolicies in backing channels namespace %q: %w", backingChannel.Namespace, err)

}

for _, policy := range existingEventPoliciesForBackingChannel {

if !r.containsPolicy(policy.Name, applyingEventPoliciesForBackingChannel) {

// the existing policy is not in the list of applying policies anymore --> is outdated --> delete it

err := r.eventingClientSet.EventingV1alpha1().EventPolicies(policy.Namespace).Delete(ctx, policy.Name, metav1.DeleteOptions{})

if err != nil && apierrs.IsNotFound(err) {

return fmt.Errorf("could not delete old EventPolicy %s/%s: %w", policy.Namespace, policy.Name, err)

}

}

}

gotcha. So just to clarify: currently we are filtering EventPolicies based on the Owner References of broker, instead filter out based on the labels, right ?

No, filtering the eventpolicies by ownerreference is good, but when we list the eventpolicies, we should use a label selector with these labels (see line 152 in the code I linked above). So, to summarize what we would do is:

List the EventPolicy resources using a label selector to only retrieve relevant ones

Filter the EventPolicy resources by owner reference

done, PTAL!

Cali0707 · 2024-07-10T19:03:00Z

pkg/reconciler/broker/broker.go

+		foundEP, err := r.eventPolicyLister.EventPolicies(expected.Namespace).Get(expected.Name)
+		if apierrs.IsNotFound(err) {
+			// create the EventPolicy if it doesn't exists.
+			logging.FromContext(ctx).Info("Creating EventPolicy for Broker %s", expected.Name)


I'm adding a comment here, but it applies to all of the logging in this function:

I don't think these need to be logged at the Info level, it generally seems to be Debug logs. Additionally, if we are going to be using the logger everywhere, it seems to make sense to create the logger once at the start of the function and then re-use it throughout, instead of fetching it from the context every time we want to log something

sure, will change from Info -> Debug.
On the other point, I was trying to be consistent with the rest of the code base!
So, should I keep it as it is for change to create a single logger object?

I would create a single logger object for this function if you are going to have this many log statements. In other places, since there isn't a ton of logging it might be fine to just fetch the logger everytime. But, normally we fetch the logger once when we have this much logging to do in a function

done, PTAL!

Cali0707 · 2024-07-10T19:05:44Z

pkg/reconciler/broker/broker.go

+			_, err = r.eventingClientSet.EventingV1alpha1().EventPolicies(expected.Namespace).Create(ctx, expected, metav1.CreateOptions{})
+			if err != nil {
+				return fmt.Errorf("failed to create EventPolicy for Broker %s: %w", expected.Name, err)
+			}
+		} else if err != nil {
+			return fmt.Errorf("failed to get EventPolicy for Broker %s: %w", expected.Name, err)
+		} else if r.policyNeedsUpdate(foundEP, expected) {
+			// update the EventPolicy if it exists and needs update.
+			logging.FromContext(ctx).Info("Updating EventPolicy for Broker %s", expected.Name)
+			expected.SetResourceVersion(foundEP.GetResourceVersion())
+			_, err = r.eventingClientSet.EventingV1alpha1().EventPolicies(expected.Namespace).Update(ctx, expected, metav1.UpdateOptions{})
+			if err != nil {
+				return fmt.Errorf("failed to update EventPolicy for Broker %s: %w", expected.Name, err)
+			}
+		}
+	} else {


all of these if/else blocks can make it a little unclear what is going on, I might refactor this to return early in many of these cases. For example, if the EventPolicy is successfully created, we can return nil. This gets rid of the need for the else if r.policyNeedsUpdate(...), which can be turned into a simpler if r.policyNeedsUpdate(...).

good point, thanks for the feedback, I will make the changes!

done, PTAL!

Cali0707 · 2024-07-10T19:07:16Z

pkg/reconciler/broker/broker.go

+	return nil
+}
+
+func (r *Reconciler) policyNeedsUpdate(foundEP, expected *eventingv1alpha1.EventPolicy) bool {


nit: this function doesn't really need to be a method on the Reconciler struct

Suggested change

func (r *Reconciler) policyNeedsUpdate(foundEP, expected *eventingv1alpha1.EventPolicy) bool {

func policyNeedsUpdate(foundEP, expected *eventingv1alpha1.EventPolicy) bool {

done, PTAL!

Signed-off-by: rahulii <[email protected]>

…ext and some minor fixes Signed-off-by: rahulii <[email protected]>

rahulii · 2024-07-12T07:18:51Z

/ok-to-test

Cali0707

/lgtm
/approve

Thanks @rahulii !

knative-prow · 2024-07-12T12:28:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, rahulii

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Cali0707]

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

reconcile event policies for broker

0d02e63

Signed-off-by: rahulii <[email protected]>

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2024

knative-prow bot requested review from Cali0707 and Leo6Leo July 9, 2024 19:51

Cali0707 reviewed Jul 9, 2024

View reviewed changes

pkg/reconciler/broker/broker.go Outdated Show resolved Hide resolved

pkg/reconciler/broker/broker.go Outdated Show resolved Hide resolved

rahulii added 2 commits July 10, 2024 12:39

delete the orphaned event policies by owner references and changes as…

36ba38b

… per review comments Signed-off-by: rahulii <[email protected]>

fix tests and move event policy creation to new file

eac9d90

Signed-off-by: rahulii <[email protected]>

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 10, 2024

rahulii added 2 commits July 10, 2024 16:36

add test cases

6d6c791

Signed-off-by: rahulii <[email protected]>

add test cases

1babdd5

Signed-off-by: rahulii <[email protected]>

rahulii marked this pull request as ready for review July 10, 2024 11:26

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2024

knative-prow bot requested review from aslom and Cali0707 July 10, 2024 11:26

Cali0707 requested changes Jul 10, 2024

View reviewed changes

knative-prow bot assigned Cali0707 Jul 10, 2024

rahulii added 5 commits July 11, 2024 12:26

changes to review comments

06cfe3a

Signed-off-by: rahulii <[email protected]>

changes to review comments

692d303

Signed-off-by: rahulii <[email protected]>

refactor the code to reduce redundant if else statements

a568dbd

Signed-off-by: rahulii <[email protected]>

change logs from info to debug

60076af

Signed-off-by: rahulii <[email protected]>

use a single logger object instead of creating it everytime from cont…

cc53eba

…ext and some minor fixes Signed-off-by: rahulii <[email protected]>

knative-prow bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 12, 2024

Cali0707 approved these changes Jul 12, 2024

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 12, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 12, 2024

knative-prow bot merged commit a61107c into knative:main Jul 12, 2024
34 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconcile event policies for mt-broker #8090

Reconcile event policies for mt-broker #8090

rahulii commented Jul 9, 2024 •

edited

Loading

knative-prow bot commented Jul 9, 2024

Cali0707 left a comment

codecov bot commented Jul 10, 2024 •

edited

Loading

Cali0707 left a comment

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024

Cali0707 Jul 11, 2024

rahulii Jul 11, 2024

Cali0707 Jul 11, 2024

rahulii Jul 12, 2024

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024 •

edited

Loading

Cali0707 Jul 11, 2024

rahulii Jul 12, 2024

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024

rahulii Jul 11, 2024

Cali0707 Jul 10, 2024

rahulii Jul 11, 2024

rahulii commented Jul 12, 2024

Cali0707 left a comment

knative-prow bot commented Jul 12, 2024

	applyingEventPoliciesForBackingChannel, err := auth.GetEventPoliciesForResource(r.eventPolicyLister, backingChannel.GroupVersionKind(), backingChannel.ObjectMeta)
	if err != nil {
	return fmt.Errorf("could not get applying EventPolicies for for backing channel %s/%s: %w", channel.Namespace, channel.Name, err)
	}

	selector, err := labels.ValidatedSelectorFromSet(resources.LabelsForBackingChannelsEventPolicy(backingChannel))
	if err != nil {
	return fmt.Errorf("could not get valid selector for backing channels EventPolicy %s/%s: %w", backingChannel.Namespace, backingChannel.Name, err)
	}

	existingEventPoliciesForBackingChannel, err := r.eventPolicyLister.EventPolicies(backingChannel.Namespace).List(selector)
	if err != nil {
	return fmt.Errorf("could not get existing EventPolicies in backing channels namespace %q: %w", backingChannel.Namespace, err)
	}

	for _, policy := range existingEventPoliciesForBackingChannel {
	if !r.containsPolicy(policy.Name, applyingEventPoliciesForBackingChannel) {

	// the existing policy is not in the list of applying policies anymore --> is outdated --> delete it
	err := r.eventingClientSet.EventingV1alpha1().EventPolicies(policy.Namespace).Delete(ctx, policy.Name, metav1.DeleteOptions{})
	if err != nil && apierrs.IsNotFound(err) {
	return fmt.Errorf("could not delete old EventPolicy %s/%s: %w", policy.Namespace, policy.Name, err)
	}
	}
	}

	func (r Reconciler) policyNeedsUpdate(foundEP, expected eventingv1alpha1.EventPolicy) bool {
	func policyNeedsUpdate(foundEP, expected *eventingv1alpha1.EventPolicy) bool {

Reconcile event policies for mt-broker #8090

Reconcile event policies for mt-broker #8090

Conversation

rahulii commented Jul 9, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

knative-prow bot commented Jul 9, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

Cali0707 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulii Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulii commented Jul 12, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jul 12, 2024

rahulii commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

rahulii Jul 11, 2024 •

edited

Loading