-
Notifications
You must be signed in to change notification settings - Fork 22
feat(controller): emit events for MCP handshake failures #186
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
Merged
k8s-ci-robot
merged 7 commits into
kubernetes-sigs:main
from
ibm-adarsh:emit-events-handshake
May 21, 2026
Merged
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
087ea54
feat(controller): emit events for MCP handshake failures
ibm-adarsh 5ec79f8
fix: satisfy golangci-lint gocyclo and modernize for handshake events
ibm-adarsh 5fca5f7
fix(controller): include MCPServer name in all emitted event messages
ibm-adarsh 3c66914
Merge main and re-apply handshake events on split controller
ibm-adarsh b6f3b1f
test(controller): assert second handshake-failed event on message change
ibm-adarsh 099e114
test(controller): replace goto with drainEvents in exhausted test
ibm-adarsh 627be9c
test(controller): fix revive import-shadowing in drainEvents
ibm-adarsh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or 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 hidden or 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 hidden or 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 |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ package controller | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/modelcontextprotocol/go-sdk/mcp" | ||
|
|
@@ -449,6 +450,134 @@ var _ = Describe("MCPServer Controller - MCP Handshake Validation", func() { | |
| Consistently(fr.Events, 300*time.Millisecond, 20*time.Millisecond).ShouldNot(Receive()) | ||
| }) | ||
|
|
||
| It("should emit a Warning MCPHandshakeFailed event only when handshake error message changes", func() { | ||
|
ibm-adarsh marked this conversation as resolved.
|
||
| failMsg := "intentional failure" | ||
| reconciler, fr := newReconcilerForTestWithFakeEvents(k8sClient, k8sClient.Scheme()) | ||
| reconciler.MCPDialer = func(ctx context.Context, url string) (*mcpv1alpha1.MCPServerInfo, error) { | ||
| return nil, fmt.Errorf("%s", failMsg) | ||
| } | ||
|
|
||
| By("Initial reconciliation creates deployment") | ||
| _, err := reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| drainFakeRecorderEvents(fr) | ||
|
|
||
| By("Simulating deployment becoming available") | ||
| deployment := &appsv1.Deployment{} | ||
| Expect(k8sClient.Get(ctx, client.ObjectKey{ | ||
| Name: resourceName, Namespace: "default", | ||
| }, deployment)).To(Succeed()) | ||
| deployment.Status.Replicas = 1 | ||
| deployment.Status.ReadyReplicas = 1 | ||
| deployment.Status.Conditions = []appsv1.DeploymentCondition{ | ||
| {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue}, | ||
| {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue}, | ||
| } | ||
| Expect(k8sClient.Status().Update(ctx, deployment)).To(Succeed()) | ||
|
|
||
| By("First handshake failure — Warning event emitted once") | ||
| _, err = reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| var handshakeFailedEvent string | ||
| Eventually(fr.Events).Should(Receive(&handshakeFailedEvent)) | ||
| Expect(handshakeFailedEvent).To(ContainSubstring(corev1.EventTypeWarning)) | ||
| Expect(handshakeFailedEvent).To(ContainSubstring(ReasonMCPEndpointUnavailable)) | ||
| Expect(handshakeFailedEvent).To(ContainSubstring(failMsg)) | ||
|
|
||
| By("Second reconcile with same error — no duplicate handshake failed event") | ||
| _, err = reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Consistently(fr.Events, 300*time.Millisecond, 20*time.Millisecond).ShouldNot(Receive()) | ||
| }) | ||
|
|
||
| It("should emit MCPHandshakeRetriesExhausted once when max handshake retries is reached", func() { | ||
| reconciler, fr := newReconcilerForTestWithFakeEvents(k8sClient, k8sClient.Scheme()) | ||
| reconciler.MCPDialer = func(ctx context.Context, url string) (*mcpv1alpha1.MCPServerInfo, error) { | ||
| return nil, fmt.Errorf("intentional failure") | ||
| } | ||
|
|
||
| By("Initial reconciliation creates deployment") | ||
| _, err := reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| drainFakeRecorderEvents(fr) | ||
|
|
||
| By("Simulating deployment becoming available") | ||
| deployment := &appsv1.Deployment{} | ||
| Expect(k8sClient.Get(ctx, client.ObjectKey{ | ||
| Name: resourceName, Namespace: "default", | ||
| }, deployment)).To(Succeed()) | ||
| deployment.Status.Replicas = 1 | ||
| deployment.Status.ReadyReplicas = 1 | ||
| deployment.Status.Conditions = []appsv1.DeploymentCondition{ | ||
| {Type: appsv1.DeploymentAvailable, Status: corev1.ConditionTrue}, | ||
| {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue}, | ||
| } | ||
| Expect(k8sClient.Status().Update(ctx, deployment)).To(Succeed()) | ||
|
|
||
| By("Reconciling until handshake retries are exhausted") | ||
| for i := range maxMCPHandshakeRetries { | ||
| result, recErr := reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(recErr).NotTo(HaveOccurred()) | ||
| if i < maxMCPHandshakeRetries-1 { | ||
| Expect(result.RequeueAfter).To(BeNumerically(">", 0)) | ||
| } | ||
| } | ||
|
|
||
| var collected []string | ||
| Eventually(func(g Gomega) { | ||
| for { | ||
| select { | ||
| case ev := <-fr.Events: | ||
| collected = append(collected, ev) | ||
| default: | ||
| goto check | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To get rid of this func drainEvents(ch <-chan string) []string {
var events []string
for {
select {
case ev := <-ch:
events = append(events, ev)
default:
return events
}
}
}
Then the test becomes:
Eventually(func(g Gomega) {
collected = drainEvents(fr.Events)
exhausted := 0
for _, ev := range collected {
if strings.Contains(ev, "retries exhausted") {
exhausted++
}
}
g.Expect(exhausted).To(Equal(1))
}).Should(Succeed())Alternative is using a labeled break, which is also not preferred.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DONE! |
||
| } | ||
| } | ||
| check: | ||
| exhausted := 0 | ||
| for _, ev := range collected { | ||
| if strings.Contains(ev, "retries exhausted") { | ||
| exhausted++ | ||
| } | ||
| } | ||
| g.Expect(exhausted).To(Equal(1)) | ||
| }).Should(Succeed()) | ||
| var exhaustedEvent string | ||
| for _, ev := range collected { | ||
| if strings.Contains(ev, "retries exhausted") { | ||
| exhaustedEvent = ev | ||
| break | ||
| } | ||
| } | ||
| Expect(exhaustedEvent).To(ContainSubstring(corev1.EventTypeWarning)) | ||
| Expect(exhaustedEvent).To(ContainSubstring(ReasonMCPEndpointUnavailable)) | ||
| Expect(exhaustedEvent).To(ContainSubstring(resourceName)) | ||
|
|
||
| mcpServer := &mcpv1alpha1.MCPServer{} | ||
| Expect(k8sClient.Get(ctx, typeNamespacedName, mcpServer)).To(Succeed()) | ||
| Expect(mcpServer.Status.HandshakeRetryCount).To(BeNumerically(">=", maxMCPHandshakeRetries)) | ||
|
|
||
| By("Further reconcile — no duplicate exhausted event") | ||
| drainFakeRecorderEvents(fr) | ||
| result, err := reconciler.Reconcile(ctx, reconcile.Request{ | ||
| NamespacedName: typeNamespacedName, | ||
| }) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(result.RequeueAfter).To(BeZero()) | ||
| Consistently(fr.Events, 300*time.Millisecond, 20*time.Millisecond).ShouldNot(Receive()) | ||
| }) | ||
|
|
||
| It("should pass a context with timeout to the dialer", func() { | ||
| var receivedCtx context.Context | ||
| reconciler := &MCPServerReconciler{ | ||
|
|
||
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.
in the
emitMCPHandshakeRetriesExhaustedthe name of the mcp server is used - perhaps we can do here too for more consistent approach?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.
Thank you @matzew for pointing it out.
Done in 5fca5f7 — emitMCPHandshakeFailed now includes the MCPServer name. Updated configuration accepted/invalid messages in the same PR for consistency across all emitted events.