Skip to content

Commit 2ce0a80

Browse files
asergeant01asergeant01
committed
Improve handling of redfish error responses
Signed-off-by: Alan Sergeant <alan.sergeant@sap.com>
1 parent 6f69807 commit 2ce0a80

3 files changed

Lines changed: 161 additions & 71 deletions

File tree

‎bmc/redfish.go‎

Lines changed: 9 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1065,46 +1065,20 @@ func (r *RedfishBaseBMC) CreateEventSubscription(
10651065
// Sending empty strings ("") causes 400 errors on BMCs that validate enum values.
10661066
resp, err := client.Post(ev.SubscriptionsLink, payload)
10671067
if err != nil {
1068+
// Gofish returns non-2xx responses as errors (format: "STATUS_CODE: BODY").
1069+
// Check if it's a "resource already exists" error and recover the existing subscription URI.
1070+
if strings.Contains(err.Error(), "ResourceAlreadyExists") || strings.Contains(err.Error(), "PropertyValueModified") {
1071+
existingLink, findErr := r.findExistingSubscription(destination)
1072+
if findErr == nil {
1073+
return existingLink, nil
1074+
}
1075+
return "", fmt.Errorf("failed to recover existing subscription (%w) after duplicate error: %v", findErr, err)
1076+
}
10681077
return "", err
10691078
}
10701079
defer func() {
10711080
_ = resp.Body.Close()
10721081
}()
1073-
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
1074-
// Read error response body
1075-
bodyBytes, err := io.ReadAll(resp.Body)
1076-
if err != nil {
1077-
return "", fmt.Errorf("failed to create event subscription status code: %d", resp.StatusCode)
1078-
}
1079-
1080-
// Parse Redfish error response
1081-
var redfishError struct {
1082-
Error struct {
1083-
MessageExtendedInfo []struct {
1084-
MessageId string `json:"MessageId"`
1085-
Message string `json:"Message"`
1086-
} `json:"@Message.ExtendedInfo"`
1087-
} `json:"error"`
1088-
}
1089-
1090-
if err := json.Unmarshal(bodyBytes, &redfishError); err == nil {
1091-
// Check if it's a "resource already exists" error
1092-
for _, info := range redfishError.Error.MessageExtendedInfo {
1093-
if strings.Contains(info.MessageId, "ResourceAlreadyExists") ||
1094-
strings.Contains(info.MessageId, "PropertyValueModified") {
1095-
// Handle duplicate subscription - try to find existing one
1096-
existingLink, findErr := r.findExistingSubscription(destination)
1097-
if findErr == nil {
1098-
return existingLink, nil
1099-
}
1100-
return "", fmt.Errorf("failed to recover existing subscription (%w) after duplicate error: %s", findErr, string(bodyBytes))
1101-
}
1102-
}
1103-
}
1104-
1105-
// Not a duplicate error - return original error with details
1106-
return "", fmt.Errorf("failed to create event subscription status code: %d: %s", resp.StatusCode, string(bodyBytes))
1107-
}
11081082
// return subscription link from returned location
11091083
subscriptionLink := resp.Header.Get("Location")
11101084
if subscriptionLink == "" {

‎bmc/redfish_hpe.go‎

Lines changed: 9 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -82,47 +82,20 @@ func (r *HPERedfishBMC) CreateEventSubscription(
8282

8383
resp, err := client.Post(ev.SubscriptionsLink, payload)
8484
if err != nil {
85+
// Gofish returns non-2xx responses as errors (format: "STATUS_CODE: BODY").
86+
// Check if it's a "resource already exists" error and recover the existing subscription URI.
87+
if strings.Contains(err.Error(), "ResourceAlreadyExists") || strings.Contains(err.Error(), "PropertyValueModified") {
88+
existingLink, findErr := r.findExistingSubscription(destination)
89+
if findErr == nil {
90+
return existingLink, nil
91+
}
92+
return "", fmt.Errorf("failed to recover existing subscription (%w) after duplicate error: %v", findErr, err)
93+
}
8594
return "", err
8695
}
8796
defer func() {
8897
_ = resp.Body.Close()
8998
}()
90-
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
91-
// Read error response body
92-
bodyBytes, err := io.ReadAll(resp.Body)
93-
if err != nil {
94-
return "", fmt.Errorf("failed to create event subscription status code: %d", resp.StatusCode)
95-
}
96-
97-
// Parse Redfish error response
98-
var redfishError struct {
99-
Error struct {
100-
MessageExtendedInfo []struct {
101-
MessageID string `json:"MessageId"`
102-
Message string `json:"Message"`
103-
} `json:"@Message.ExtendedInfo"`
104-
} `json:"error"`
105-
}
106-
107-
if err := json.Unmarshal(bodyBytes, &redfishError); err == nil {
108-
// Check if it's a "resource already exists" error
109-
for _, info := range redfishError.Error.MessageExtendedInfo {
110-
if strings.Contains(info.MessageID, "ResourceAlreadyExists") ||
111-
strings.Contains(info.MessageID, "PropertyValueModified") {
112-
// Handle duplicate subscription - try to find existing one
113-
existingLink, findErr := r.findExistingSubscription(destination)
114-
if findErr == nil {
115-
return existingLink, nil
116-
}
117-
return "", fmt.Errorf("failed to recover existing subscription (%w) after duplicate error: %s", findErr, string(bodyBytes))
118-
}
119-
}
120-
}
121-
122-
// Not a duplicate error - return original error with details
123-
return "", fmt.Errorf("failed to create event subscription status code: %d: %s", resp.StatusCode, string(bodyBytes))
124-
}
125-
12699
// return subscription link from returned location
127100
subscriptionLink := resp.Header.Get("Location")
128101
if subscriptionLink == "" {

‎bmc/redfish_test.go‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,3 +511,146 @@ var _ = Describe("RedfishBaseBMC findExistingSubscription", func() {
511511
Expect(link).To(BeEmpty())
512512
})
513513
})
514+
515+
var _ = Describe("RedfishBaseBMC CreateEventSubscription", func() {
516+
ctx := context.Background()
517+
const destination = "http://operator/serverevents/alerts/node1"
518+
519+
// baseHandlers registers the service root and event service on mux.
520+
baseHandlers := func(mux *http.ServeMux) {
521+
mux.HandleFunc("/redfish/v1/", func(w http.ResponseWriter, _ *http.Request) {
522+
w.Header().Set("Content-Type", "application/json")
523+
w.Write(serviceRootWithEventServiceJSON()) //nolint:errcheck
524+
})
525+
mux.HandleFunc("/redfish/v1/EventService", func(w http.ResponseWriter, _ *http.Request) {
526+
w.Header().Set("Content-Type", "application/json")
527+
w.Write(eventServiceJSON()) //nolint:errcheck
528+
})
529+
}
530+
531+
It("should return the subscription link from Location header on success", func() {
532+
mux := http.NewServeMux()
533+
baseHandlers(mux)
534+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions", func(w http.ResponseWriter, r *http.Request) {
535+
if r.Method == http.MethodPost {
536+
w.Header().Set("Location", "/redfish/v1/EventService/Subscriptions/new-1")
537+
w.WriteHeader(http.StatusCreated)
538+
return
539+
}
540+
w.Header().Set("Content-Type", "application/json")
541+
w.Write(subscriptionsCollectionJSON([]string{})) //nolint:errcheck
542+
})
543+
544+
server := httptest.NewServer(mux)
545+
defer server.Close()
546+
547+
bmc := newTestRedfishBMC(server)
548+
link, err := bmc.CreateEventSubscription(ctx, destination, schemas.EventFormatType("Event"), schemas.DeliveryRetryPolicy("RetryForever"))
549+
Expect(err).NotTo(HaveOccurred())
550+
Expect(link).To(Equal("/redfish/v1/EventService/Subscriptions/new-1"))
551+
})
552+
553+
// This test is the regression guard for the fix. Gofish returns non-2xx responses as errors —
554+
// the subscription link can never be recovered from resp.StatusCode because resp is nil when
555+
// err != nil. The recovery MUST happen via err.Error() in the if-err block.
556+
// If someone re-introduces a resp.StatusCode check instead, this test will fail because
557+
// findExistingSubscription will never be called and the function will return the raw error.
558+
It("should recover existing subscription when BMC returns ResourceAlreadyExists (via err.Error)", func() {
559+
mux := http.NewServeMux()
560+
baseHandlers(mux)
561+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions", func(w http.ResponseWriter, r *http.Request) {
562+
if r.Method == http.MethodPost {
563+
// Gofish wraps this 409 as an error containing the body text.
564+
// strings.Contains(err.Error(), "ResourceAlreadyExists") must be true.
565+
w.WriteHeader(http.StatusConflict)
566+
w.Write([]byte(`{"error":{"@Message.ExtendedInfo":[{"MessageId":"Base.1.0.ResourceAlreadyExists","Message":"already exists"}]}}`)) //nolint:errcheck
567+
return
568+
}
569+
w.Header().Set("Content-Type", "application/json")
570+
w.Write(subscriptionsCollectionJSON([]string{"/redfish/v1/EventService/Subscriptions/existing-1"})) //nolint:errcheck
571+
})
572+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions/existing-1", func(w http.ResponseWriter, _ *http.Request) {
573+
w.Header().Set("Content-Type", "application/json")
574+
w.Write(subscriptionJSON("existing-1", destination, "metal-operator")) //nolint:errcheck
575+
})
576+
577+
server := httptest.NewServer(mux)
578+
defer server.Close()
579+
580+
bmc := newTestRedfishBMC(server)
581+
link, err := bmc.CreateEventSubscription(ctx, destination, schemas.EventFormatType("Event"), schemas.DeliveryRetryPolicy("RetryForever"))
582+
Expect(err).NotTo(HaveOccurred())
583+
Expect(link).To(Equal("/redfish/v1/EventService/Subscriptions/existing-1"))
584+
})
585+
586+
It("should recover existing subscription when BMC returns PropertyValueModified (via err.Error)", func() {
587+
mux := http.NewServeMux()
588+
baseHandlers(mux)
589+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions", func(w http.ResponseWriter, r *http.Request) {
590+
if r.Method == http.MethodPost {
591+
w.WriteHeader(http.StatusBadRequest)
592+
w.Write([]byte(`{"error":{"@Message.ExtendedInfo":[{"MessageId":"Base.1.0.PropertyValueModified","Message":"already exists"}]}}`)) //nolint:errcheck
593+
return
594+
}
595+
w.Header().Set("Content-Type", "application/json")
596+
w.Write(subscriptionsCollectionJSON([]string{"/redfish/v1/EventService/Subscriptions/existing-1"})) //nolint:errcheck
597+
})
598+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions/existing-1", func(w http.ResponseWriter, _ *http.Request) {
599+
w.Header().Set("Content-Type", "application/json")
600+
w.Write(subscriptionJSON("existing-1", destination, "metal-operator")) //nolint:errcheck
601+
})
602+
603+
server := httptest.NewServer(mux)
604+
defer server.Close()
605+
606+
bmc := newTestRedfishBMC(server)
607+
link, err := bmc.CreateEventSubscription(ctx, destination, schemas.EventFormatType("Event"), schemas.DeliveryRetryPolicy("RetryForever"))
608+
Expect(err).NotTo(HaveOccurred())
609+
Expect(link).To(Equal("/redfish/v1/EventService/Subscriptions/existing-1"))
610+
})
611+
612+
It("should return error when BMC returns ResourceAlreadyExists but existing subscription cannot be found", func() {
613+
mux := http.NewServeMux()
614+
baseHandlers(mux)
615+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions", func(w http.ResponseWriter, r *http.Request) {
616+
if r.Method == http.MethodPost {
617+
w.WriteHeader(http.StatusConflict)
618+
w.Write([]byte(`{"error":{"@Message.ExtendedInfo":[{"MessageId":"Base.1.0.ResourceAlreadyExists"}]}}`)) //nolint:errcheck
619+
return
620+
}
621+
// Return empty collection so findExistingSubscription finds nothing.
622+
w.Header().Set("Content-Type", "application/json")
623+
w.Write(subscriptionsCollectionJSON([]string{})) //nolint:errcheck
624+
})
625+
626+
server := httptest.NewServer(mux)
627+
defer server.Close()
628+
629+
bmc := newTestRedfishBMC(server)
630+
link, err := bmc.CreateEventSubscription(ctx, destination, schemas.EventFormatType("Event"), schemas.DeliveryRetryPolicy("RetryForever"))
631+
Expect(err).To(HaveOccurred())
632+
Expect(err.Error()).To(ContainSubstring("failed to recover existing subscription"))
633+
Expect(link).To(BeEmpty())
634+
})
635+
636+
It("should propagate non-duplicate POST errors unchanged", func() {
637+
mux := http.NewServeMux()
638+
baseHandlers(mux)
639+
mux.HandleFunc("/redfish/v1/EventService/Subscriptions", func(w http.ResponseWriter, r *http.Request) {
640+
if r.Method == http.MethodPost {
641+
w.WriteHeader(http.StatusInternalServerError)
642+
w.Write([]byte(`{"error":{"message":"internal server error"}}`)) //nolint:errcheck
643+
return
644+
}
645+
})
646+
647+
server := httptest.NewServer(mux)
648+
defer server.Close()
649+
650+
bmc := newTestRedfishBMC(server)
651+
link, err := bmc.CreateEventSubscription(ctx, destination, schemas.EventFormatType("Event"), schemas.DeliveryRetryPolicy("RetryForever"))
652+
Expect(err).To(HaveOccurred())
653+
Expect(err.Error()).NotTo(ContainSubstring("failed to recover existing subscription"))
654+
Expect(link).To(BeEmpty())
655+
})
656+
})

0 commit comments

Comments
 (0)