Reconcile event policies for mt-broker #8090
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ import ( | |
|
|
||
| "knative.dev/eventing/pkg/apis/eventing" | ||
| eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" | ||
| eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1" | ||
| "knative.dev/eventing/pkg/apis/feature" | ||
| "knative.dev/eventing/pkg/auth" | ||
| fakeeventingclient "knative.dev/eventing/pkg/client/injection/client/fake" | ||
|
|
@@ -57,6 +58,7 @@ import ( | |
| . "knative.dev/pkg/reconciler/testing" | ||
|
|
||
| _ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/trigger/fake" | ||
| "knative.dev/eventing/pkg/reconciler/broker/resources" | ||
| . "knative.dev/eventing/pkg/reconciler/testing/v1" | ||
| ) | ||
|
|
||
|
|
@@ -144,6 +146,12 @@ var ( | |
| Version: "v1", | ||
| Kind: "Broker", | ||
| } | ||
|
|
||
| channelV1GVK = metav1.GroupVersionKind{ | ||
| Group: "messaging.knative.dev", | ||
| Version: "v1", | ||
| Kind: "InMemoryChannel", | ||
| } | ||
| ) | ||
|
|
||
| func TestReconcile(t *testing.T) { | ||
|
|
@@ -780,6 +788,9 @@ func TestReconcile(t *testing.T) { | |
| WithEndpointsAddresses(corev1.EndpointAddress{IP: "127.0.0.1"})), | ||
| }, | ||
| WantErr: false, | ||
| WantCreates: []runtime.Object{ | ||
| makeEventPolicy(), | ||
| }, | ||
| WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ | ||
| Object: NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
|
|
@@ -824,6 +835,9 @@ func TestReconcile(t *testing.T) { | |
| WithEventPolicyToRef(brokerV1GVK, brokerName), | ||
| ), | ||
| }, | ||
| WantCreates: []runtime.Object{ | ||
| makeEventPolicy(), | ||
| }, | ||
| WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ | ||
| Object: NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
|
|
@@ -866,6 +880,9 @@ func TestReconcile(t *testing.T) { | |
| WithEventPolicyToRef(brokerV1GVK, brokerName), | ||
| ), | ||
| }, | ||
| WantCreates: []runtime.Object{ | ||
| makeEventPolicy(), | ||
| }, | ||
| WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ | ||
| Object: NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
|
|
@@ -911,6 +928,9 @@ func TestReconcile(t *testing.T) { | |
| WithEventPolicyToRef(brokerV1GVK, brokerName), | ||
| ), | ||
| }, | ||
| WantCreates: []runtime.Object{ | ||
| makeEventPolicy(), | ||
| }, | ||
| WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ | ||
| Object: NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
|
|
@@ -932,6 +952,47 @@ func TestReconcile(t *testing.T) { | |
| Ctx: feature.ToContext(context.Background(), feature.Flags{ | ||
| feature.OIDCAuthentication: feature.Enabled, | ||
| }), | ||
| }, { | ||
| Name: "Should create an Event Policy for a Broker's underlying Channel", | ||
| Key: testKey, | ||
| Objects: []runtime.Object{ | ||
| NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
| WithBrokerConfig(config()), | ||
| WithInitBrokerConditions), | ||
| createChannel(withChannelReady), | ||
| imcConfigMap(), | ||
| NewEndpoints(filterServiceName, systemNS, | ||
| WithEndpointsLabels(FilterLabels()), | ||
| WithEndpointsAddresses(corev1.EndpointAddress{IP: "127.0.0.1"})), | ||
| NewEndpoints(ingressServiceName, systemNS, | ||
| WithEndpointsLabels(IngressLabels()), | ||
| WithEndpointsAddresses(corev1.EndpointAddress{IP: "127.0.0.1"})), | ||
| }, | ||
| WantCreates: []runtime.Object{ | ||
| makeEventPolicy(), | ||
| }, | ||
| WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ | ||
| Object: NewBroker(brokerName, testNS, | ||
| WithBrokerClass(eventing.MTChannelBrokerClassValue), | ||
| WithBrokerConfig(config()), | ||
| WithBrokerReady, | ||
| WithBrokerAddress(&duckv1.Addressable{ | ||
| URL: brokerAddress, | ||
| Audience: &brokerAudience, | ||
| }), | ||
| WithChannelAddressAnnotation(triggerChannelURL), | ||
| WithChannelAPIVersionAnnotation(triggerChannelAPIVersion), | ||
| WithChannelKindAnnotation(triggerChannelKind), | ||
| WithChannelNameAnnotation(triggerChannelName), | ||
| WithDLSNotConfigured(), | ||
| WithBrokerEventPoliciesReadyBecauseNoPolicyAndOIDCEnabled(), | ||
| ), | ||
| }}, | ||
| Ctx: feature.ToContext(context.Background(), feature.Flags{ | ||
| feature.OIDCAuthentication: feature.Enabled, | ||
| feature.AuthorizationDefaultMode: feature.AuthorizationAllowSameNamespace, | ||
| }), | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -1227,3 +1288,23 @@ func makeTLSSecret() *corev1.Secret { | |
| Type: corev1.SecretTypeTLS, | ||
| } | ||
| } | ||
|
|
||
| func makeEventPolicy() *eventingv1alpha1.EventPolicy { | ||
| return NewEventPolicy(brokerName+"-event-policy", testNS, | ||
| WithEventPolicyToRef(channelV1GVK, triggerChannelName), | ||
| WithEventPolicyFromSub(resources.OIDCBrokerSub), | ||
| WithEventPolicyOwnerReferences([]metav1.OwnerReference{ | ||
| { | ||
| APIVersion: "eventing.knative.dev/v1", | ||
| Kind: "Broker", | ||
| Name: brokerName, | ||
| }, | ||
| }...), | ||
| WithEventPolicyLabels(map[string]string{ | ||
| "eventing.knative.dev/" + "broker-group": brokerV1GVK.Group, | ||
| "eventing.knative.dev/" + "broker-version": brokerV1GVK.Version, | ||
| "eventing.knative.dev/" + "broker-kind": brokerV1GVK.Kind, | ||
| "eventing.knative.dev/" + "broker-name": brokerName, | ||
| }), | ||
| ) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| /* | ||
| Copyright 2024 The Knative Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package resources | ||
|
|
||
| import ( | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" | ||
| eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" | ||
| eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1" | ||
| ) | ||
|
|
||
| const ( | ||
| BackingChannelEventPolicyLabelPrefix = "eventing.knative.dev/" | ||
| OIDCBrokerSub = "system:serviceaccount:knative-eventing:mt-broker-ingress-oidc" | ||
| brokerAPIVersion = "eventing.knative.dev/v1" | ||
| version = "v1" | ||
| brokerKind = "Broker" | ||
| brokerGroup = "eventing.knative.dev" | ||
| ) | ||
|
|
||
| func MakeEventPolicyForBackingChannel(b *eventingv1.Broker, backingChannel *eventingduckv1.Channelable) *eventingv1alpha1.EventPolicy { | ||
| return &eventingv1alpha1.EventPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: backingChannel.Namespace, | ||
| Name: b.Name + "-event-policy", | ||
|
There was a problem hiding this comment. 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 |
||
| OwnerReferences: []metav1.OwnerReference{ | ||
| { | ||
| APIVersion: brokerAPIVersion, | ||
| Kind: brokerKind, | ||
| Name: b.Name, | ||
| }, | ||
| }, | ||
| Labels: LabelsForBackingChannelsEventPolicy(b), | ||
| }, | ||
| Spec: eventingv1alpha1.EventPolicySpec{ | ||
| To: []eventingv1alpha1.EventPolicySpecTo{ | ||
| { | ||
| Ref: &eventingv1alpha1.EventPolicyToReference{ | ||
| APIVersion: backingChannel.APIVersion, | ||
| Kind: backingChannel.Kind, | ||
| Name: backingChannel.Name, | ||
| }, | ||
| }, | ||
| }, | ||
| From: []eventingv1alpha1.EventPolicySpecFrom{ | ||
| { | ||
| Sub: toStrPtr(OIDCBrokerSub), | ||
|
There was a problem hiding this comment. Instead of using our own implementation here, we normally use the k8s |
||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func LabelsForBackingChannelsEventPolicy(broker *eventingv1.Broker) map[string]string { | ||
| return map[string]string{ | ||
| BackingChannelEventPolicyLabelPrefix + "broker-group": brokerGroup, | ||
| BackingChannelEventPolicyLabelPrefix + "broker-version": version, | ||
| BackingChannelEventPolicyLabelPrefix + "broker-kind": brokerKind, | ||
| BackingChannelEventPolicyLabelPrefix + "broker-name": broker.Name, | ||
|
There was a problem hiding this comment. I'm curious if we actually need all of these labels for the EventPolicies, what exactly are you using them all for? There was a problem hiding this comment. @Cali0707 I took reference from channel - https://github.com/knative/eventing/blob/main/pkg/reconciler/channel/resources/eventpolicy.go#L69 There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 ? There was a problem hiding this comment. 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:
|
||
| } | ||
| } | ||
|
|
||
| func toStrPtr(s string) *string { | ||
| return &s | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| /* | ||
| Copyright 2024 The Knative Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package resources | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" | ||
| eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1" | ||
| eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1" | ||
| ) | ||
|
|
||
| func TestMakeEventPolicyForBackingChannel(t *testing.T) { | ||
| broker := &eventingv1.Broker{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-broker", | ||
| Namespace: "test-namespace", | ||
| }, | ||
| } | ||
| backingChannel := &eventingduckv1.Channelable{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "test-channel", | ||
| Namespace: "test-namespace", | ||
| }, | ||
| } | ||
| want := &eventingv1alpha1.EventPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Namespace: "test-namespace", | ||
| Name: "test-broker-event-policy", | ||
| OwnerReferences: []metav1.OwnerReference{ | ||
| { | ||
| APIVersion: "eventing.knative.dev/v1", | ||
| Kind: "Broker", | ||
| Name: "test-broker", | ||
| }, | ||
| }, | ||
| Labels: LabelsForBackingChannelsEventPolicy(broker), | ||
| }, | ||
| Spec: eventingv1alpha1.EventPolicySpec{ | ||
| To: []eventingv1alpha1.EventPolicySpecTo{ | ||
| { | ||
| Ref: &eventingv1alpha1.EventPolicyToReference{ | ||
| APIVersion: backingChannel.APIVersion, | ||
| Kind: backingChannel.Kind, | ||
| Name: backingChannel.Name, | ||
| }, | ||
| }, | ||
| }, | ||
| From: []eventingv1alpha1.EventPolicySpecFrom{ | ||
| { | ||
| Sub: toStrPtr(OIDCBrokerSub), | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| got := MakeEventPolicyForBackingChannel(broker, backingChannel) | ||
| if diff := cmp.Diff(want, got); diff != "" { | ||
| t.Errorf("MakeEventPolicyForBackingChannel() (-want, +got) = %v", diff) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,14 @@ func WithEventPolicyFrom(gvk metav1.GroupVersionKind, name, namespace string) Ev | |
| } | ||
| } | ||
|
|
||
| func WithEventPolicyFromSub(sub string) EventPolicyOption { | ||
| return func(ep *v1alpha1.EventPolicy) { | ||
| ep.Spec.From = append(ep.Spec.From, v1alpha1.EventPolicySpecFrom{ | ||
| Sub: &sub, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func WithEventPolicyLabels(labels map[string]string) EventPolicyOption { | ||
| return func(ep *v1alpha1.EventPolicy) { | ||
| ep.ObjectMeta.Labels = labels | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Infolevel, it generally seems to beDebuglogs. 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 somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, PTAL!