Skip to content
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.

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

Add EventPolicy reconciliation for Sequence #8106

Merged
merged 32 commits into from
Aug 9, 2024

Conversation

Leo6Leo
Copy link
Member

@Leo6Leo Leo6Leo commented Jul 19, 2024

Fixes #7983

Proposed Changes

  • Adding the new reconcilation function for eventPolicy for Sequence
  • Adding the corresponding unit tests

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


Docs

Signed-off-by: Leo Li <[email protected]>
Copy link

knative-prow bot commented Jul 19, 2024

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

@knative-prow 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2024
@knative-prow knative-prow bot requested review from aliok and matzew July 19, 2024 20:14
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2024
@Leo6Leo Leo6Leo marked this pull request as ready for review July 23, 2024 01:32
@knative-prow knative-prow bot requested a review from pierDipi July 23, 2024 01:32
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 83.87097% with 25 lines in your changes missing coverage. Please review.

Project coverage is 68.06%. Comparing base (32f8491) to head (6c8fa1a).
Report is 4 commits behind head on main.

Files Patch % Lines
pkg/reconciler/sequence/sequence.go 71.26% 13 Missing and 12 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8106      +/-   ##
==========================================
+ Coverage   67.88%   68.06%   +0.18%     
==========================================
  Files         368      369       +1     
  Lines       17565    17725     +160     
==========================================
+ Hits        11924    12065     +141     
- Misses       4893     4901       +8     
- Partials      748      759      +11     

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

@Leo6Leo Leo6Leo changed the title [WIP] Add EventPolicy reonciliation for Sequence Add EventPolicy reonciliation for Sequence Jul 23, 2024
@knative-prow 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 23, 2024
Comment on lines 343 to 354
if featureFlags.IsOIDCAuthentication() {
// Create or update EventPolicies, and we skip the first channel as it's the input channel!
for i := 1; i < len(channels); i++ {
if err := r.reconcileChannelEventPolicy(ctx, s, channels[i], subs[i-1]); err != nil {
return err
}
}

// Handle input channel EventPolicy
if err := r.reconcileInputChannelEventPolicy(ctx, s, channels[0]); err != nil {
return err
}
Copy link
Member

@pierDipi pierDipi Jul 24, 2024

Choose a reason for hiding this comment

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

there are a few more cases to handle in the logic here, for example, when a channel is removed or added (meaning a step is removed or added) channels will change and we will need to remove event policies, what I would do is:

  • list all the policies that belong to the given sequence (via <prefix> + sequence-name label)
  • based on the channels, partition the list in:
    • to be removed
    • to be updated
  • go through the to be updated list and check if there is any update needed, if so, update the policy
  • add new policies for new channels
  • remove the policies in the to be removed partition

Optimize the algorithm where possible

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

We need to add a handler here https://github.com/knative/eventing/blob/main/pkg/reconciler/sequence/controller.go to requeue a given sequence when event policies for a sequence change

Comment on lines 73 to 76
SequenceChannelEventPolicyLabelPrefix + "sequence-group": flowsv1.SchemeGroupVersion.Group,
SequenceChannelEventPolicyLabelPrefix + "sequence-version": flowsv1.SchemeGroupVersion.Version,
SequenceChannelEventPolicyLabelPrefix + "sequence-kind": sequenceKind,
SequenceChannelEventPolicyLabelPrefix + "sequence-name": sequenceName,
Copy link
Member

@pierDipi pierDipi Jul 24, 2024

Choose a reason for hiding this comment

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

Are we using the kind, version or group label? I can only see the name being useful, and therefore the GVK can be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand the flow correctly, I think the GK is used when the controller is trying to watch which eventpolicy get changed.

sequenceGK := flowsv1.SchemeGroupVersion.WithKind("Sequence").GroupKind()
// Enqueue the Sequence, if we have an EventPolicy which was referencing
// or got updated and now is referencing the Sequence
eventPolicyInformer.Informer().AddEventHandler(auth.EventPolicyEventHandler(
sequenceInformer.Informer().GetIndexer(),
sequenceGK,
impl.EnqueueKey,
))

Comment on lines 81 to 83
// if channel name is empty, it means the event policy is for the output channel
if channelName == "" {
return kmeta.ChildName(sequenceName, "-ep") // no need to add the channel name
Copy link
Member

Choose a reason for hiding this comment

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

when is the channel name empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we are making the eventpolicy for the sequence.

As you can see from the test here:
https://github.com/knative/eventing/pull/8106/files#diff-42ed9804c3511a386317c39a39cea9571e16d694153edb7d79c800ae4589ffaaR2302

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I didn't get it, is empty only for testing? when is the channel name empty in the real production case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see now by trying to answer your question. It is only for the testing purposes. I couldn't come up with any use case in the real production use. I will try to see what I can do and provide update in this thread.

@knative-prow knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 24, 2024
@Leo6Leo
Copy link
Member Author

Leo6Leo commented Jul 24, 2024

We need to add a handler here https://github.com/knative/eventing/blob/main/pkg/reconciler/sequence/controller.go to requeue a given sequence when event policies for a sequence change

Isn't it already here?

// Enqueue the Sequence, if we have an EventPolicy which was referencing
// or got updated and now is referencing the Sequence
eventPolicyInformer.Informer().AddEventHandler(auth.EventPolicyEventHandler(
sequenceInformer.Informer().GetIndexer(),
sequenceGK,
impl.EnqueueKey,
))

@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 25, 2024
@pierDipi
Copy link
Member

@Leo6Leo I'm getting an error when installing the components and creating a sequence like this one:

apiVersion: v1
kind: Service
metadata:
  name: first
spec:
  selector:
    app: first
  ports:
    - protocol: TCP
      port: 80
      targetPort: 8080
---
apiVersion: v1
kind: Pod
metadata:
  name: first
  labels:
    app: first
spec:
  containers:
  - name: source
    image: gcr.io/knative-releases/knative.dev/eventing/cmd/appender
    ports:
    - containerPort: 8080
    env:
    - name: MESSAGE
      value: " - Handled by 1"
---
apiVersion: v1
kind: Service
metadata:
  name: second
spec:
  selector:
    app: second
  ports:
    - protocol: TCP
      port: 80
      targetPort: 8080
---
apiVersion: v1
kind: Pod
metadata:
  name: second
  labels:
    app: second
spec:
  containers:
  - name: source
    image: gcr.io/knative-releases/knative.dev/eventing/cmd/appender
    ports:
    - containerPort: 8080
    env:
    - name: MESSAGE
      value: " - Handled by 2"
---
apiVersion: v1
kind: Service
metadata:
  name: event-display
spec:
  selector:
    app: event-display
  ports:
    - protocol: TCP
      port: 80
      targetPort: 8080
---
apiVersion: v1
kind: Pod
metadata:
  name: event-display
  labels:
    app: eventd-display
spec:
  containers:
  - name: event-display-source
    image: gcr.io/knative-releases/knative.dev/eventing/cmd/event_display
    ports:
    - containerPort: 8080
---
apiVersion: flows.knative.dev/v1
kind: Sequence
metadata:
  name: sequence
spec:
  channelTemplate:
    apiVersion: messaging.knative.dev/v1
    kind: InMemoryChannel
  steps:
    - ref:
        apiVersion: v1
        kind: Service
        name: first
    - ref:
        apiVersion: v1
        kind: Service
        name: second
  reply:
    ref:
      kind: Service
      apiVersion: v1
      name: event-display
Warning  InternalError  39s (x16 over 3m23s)  sequence-controller  failed to reconcile EventPolicies: failed to create EventPolicies: EventPolicy.eventing.knative.dev "sequencesequence-kn-sequence-1-ep" is invalid: metadata.ownerReferences.uid: Invalid value: "": uid must not be empty

@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2024

kind create cluster
./hack/install.sh

kubectl apply -f test/config-authentication-oidc/

There is an issue with the feature flag watch in the sequence reconciler

feature.FromContext(ctx) returns empty FF, so the auth FF is always disabled, I'll open a PR with the fix

@pierDipi
Copy link
Member

pierDipi commented Aug 1, 2024

@Leo6Leo #8124

@Leo6Leo
Copy link
Member Author

Leo6Leo commented Aug 1, 2024

The current issues I have found:

1. Even though the feature flag for OIDC has been disabled, the resources still have eventpolicies issue.

For example, when describing the sequence, it will still show EventPoliciesReady field.

Conditions:
    Last Transition Time:  2024-08-01T19:06:46Z
    Status:                True
    Type:                  Addressable
    Last Transition Time:  2024-08-01T19:06:46Z
    Status:                True
    Type:                  ChannelsReady
    Last Transition Time:  2024-08-01T19:06:46Z
    Status:                Unknown
    Type:                  EventPoliciesReady
    Last Transition Time:  2024-08-01T19:06:46Z
    Status:                Unknown
    Type:                  Ready
    Last Transition Time:  2024-08-01T19:06:46Z
    Status:                True
    Type:                  SubscriptionsReady

When I tried to create the pingSource to send the events to event-display. This happens when OIDC is disabled.

2024-08-01T19:08:00.162Z	debug	pingsource-mt-adapter	mtping/runner.go:153	sending cloudevent id: 7cb7402d-f3e6-4764-be88-43fdf3c1ef43, source: /apis/v1/namespaces/default/pingsources/ping-source, target: http://event-display.default.svc.cluster.local	{"commit": "dce2679-dirty"}
W0801 19:08:02.192506       1 reflector.go:539] k8s.io/[email protected]/tools/cache/reflector.go:229: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
E0801 19:08:02.192526       1 reflector.go:147] k8s.io/[email protected]/tools/cache/reflector.go:229: Failed to watch *v1alpha1.EventPolicy: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
W0801 19:08:27.978094       1 reflector.go:539] k8s.io/[email protected]/tools/cache/reflector.go:229: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
E0801 19:08:27.978202       1 reflector.go:147] k8s.io/[email protected]/tools/cache/reflector.go:229: Failed to watch *v1alpha1.EventPolicy: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
W0801 19:09:13.315103       1 reflector.go:539] k8s.io/[email protected]/tools/cache/reflector.go:229: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
E0801 19:09:13.315175       1 reflector.go:147] k8s.io/[email protected]/tools/cache/reflector.go:229: Failed to watch *v1alpha1.EventPolicy: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
2024-08-01T19:10:00.072Z	debug	pingsource-mt-adapter	mtping/runner.go:153	sending cloudevent id: 8ce8a7e6-a248-4244-a875-9ad47d7b69a6, source: /apis/v1/namespaces/default/pingsources/ping-source, target: http://event-display.default.svc.cluster.local	{"commit": "dce2679-dirty"}
W0801 19:10:09.859331       1 reflector.go:539] k8s.io/[email protected]/tools/cache/reflector.go:229: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope
E0801 19:10:09.859450       1 reflector.go:147] k8s.io/[email protected]/tools/cache/reflector.go:229: Failed to watch *v1alpha1.EventPolicy: failed to list *v1alpha1.EventPolicy: eventpolicies.eventing.knative.dev is forbidden: User "system:serviceaccount:knative-eventing:pingsource-mt-adapter" cannot list resource "eventpolicies" in API group "eventing.knative.dev" at the cluster scope

2. Cannot get the Kind and APIVersion of the subscriptions

The error logs show that when creating the eventpolicies,

validation failed: missing field(s): spec.from[0].ref.apiVersion, spec.from[0].ref.kind

The code that create the EventPolicies is here

It seems like the value in the sub[] array doesn't have those 2 fields.

Trying to figure things out. But if anyone has any insight, feel free to share it here!

Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @Leo6Leo!
I left a few comments. Let me know if anything is unclear

pkg/reconciler/sequence/resources/eventpolicy.go Outdated Show resolved Hide resolved
pkg/reconciler/sequence/resources/eventpolicy.go Outdated Show resolved Hide resolved
pkg/reconciler/sequence/sequence.go Outdated Show resolved Hide resolved
pkg/reconciler/sequence/sequence.go Outdated Show resolved Hide resolved
pkg/reconciler/sequence/resources/eventpolicy.go Outdated Show resolved Hide resolved
@Leo6Leo Leo6Leo requested a review from creydr August 7, 2024 05:38
@Leo6Leo
Copy link
Member Author

Leo6Leo commented Aug 7, 2024

/meow

Copy link

knative-prow bot commented Aug 7, 2024

@Leo6Leo: cat image

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…cies requiring update and cleanup.

Signed-off-by: Leo Li <[email protected]>
@Leo6Leo Leo6Leo requested a review from creydr August 7, 2024 21:01
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2024
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

Thanks a lot finishing this and checking on all the review comments 💪

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2024
Copy link

knative-prow bot commented Aug 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@creydr creydr changed the title Add EventPolicy reonciliation for Sequence Add EventPolicy reconciliation for Sequence Aug 9, 2024
@knative-prow knative-prow bot merged commit 5c81d76 into knative:main Aug 9, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sequence Reconciler: Create EventPolicies for Sequence
5 participants