fix(gateway): circuit breaker should check if upstream exists before creating

gateway/internal/features/feature/circuit_breaker.go

@@ -7,6 +7,7 @@
 import (
 	"context"
 	"fmt"
+
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	"github.com/telekom/controlplane/common/pkg/util/contextutil"
@@ -101,7 +102,7 @@
 
 func isDeleteScenario(route *gatewayv1.Route) bool {
 	// completely removed or turned to false
 	if (route.Spec.Traffic.CircuitBreaker != nil && route.Spec.Traffic.CircuitBreaker.Enabled == false) && route.GetUpstreamId() != "" {
 		return true
 	} else {
 		return false
@@ -161,35 +162,78 @@
 
 	upstreamResponse, err := kongAdminApi.UpsertUpstreamWithResponse(ctx, upstreamName, upstreamBody)
 	if err != nil {
-		return errors.Wrap(err, "failed to create upstream")
+		return errors.Wrapf(err, "failed to create upstream [PUT /upstreams/%s]", upstreamName)
 	}
 	if err := client.CheckStatusCode(upstreamResponse, 200); err != nil {
-		return errors.Wrap(fmt.Errorf("error body from kong admin api: %s", string(upstreamResponse.Body)), "failed to create upstream")
+		return errors.Wrap(fmt.Errorf("error body from kong admin api [%s %s]: %s", upstreamResponse.HTTPResponse.Request.Method, upstreamResponse.HTTPResponse.Request.URL.Path, string(upstreamResponse.Body)), "failed to create upstream")
 	}
 	route.SetUpstreamId(*upstreamResponse.JSON200.Id)
 
-	targetsName := routeName
+	// Only create the target when it does not already exist in Kong.
+	// On re-reconciliation the upstream is upserted (PUT, idempotent) but the
+	// target is left untouched — matching the behaviour of the old Java gateway.
+	targetId, err := findExistingTargetId(ctx, kongAdminApi, upstreamName, DefaultTargetsTarget)
+	if err != nil {
+		return err
+	}
+	if targetId == "" {
+		targetId, err = createTarget(ctx, kongAdminApi, upstreamName, routeName, route.GetName())
+		if err != nil {
+			return err
+		}
+	}
+	route.SetTargetsId(targetId)
+
+	return nil
+}
+
+// findExistingTargetId fetches a single target by its target value from the
+// given upstream and returns its ID. Returns empty string if not found (404).
+func findExistingTargetId(ctx context.Context, kongAdminApi client.KongAdminApi, upstreamName, targetValue string) (string, error) {
+	response, err := kongAdminApi.FetchTargetForUpstreamWithResponse(ctx, upstreamName, targetValue)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to fetch target for upstream %s", upstreamName)
+	}
+	// 404 means either the upstream or the target does not exist yet
+	if response.StatusCode() == 404 {
+		return "", nil
+	}
+	if err := client.CheckStatusCode(response, 200); err != nil {
+		return "", errors.Wrap(
+			fmt.Errorf("error body from kong admin api: %s", string(response.Body)),
+			"failed to fetch target for upstream")
+	}
+	if response.JSON200 != nil && response.JSON200.Id != nil {
+		return *response.JSON200.Id, nil
+	}
+	return "", nil
+}
+
+// createTarget creates a new target for the given upstream via POST.
+func createTarget(ctx context.Context, kongAdminApi client.KongAdminApi, upstreamName, targetsName, routeName string) (string, error) {
 	targetsTarget := DefaultTargetsTarget
 	targetsWeight := 100
 	targetsBody := kong.CreateTargetForUpstreamJSONRequestBody{
 		Tags: &[]string{
 			client.BuildTag("env", contextutil.EnvFromContextOrDie(ctx)),
 			client.BuildTag("targets", targetsName),
-			client.BuildTag("route", route.GetName()),
+			client.BuildTag("route", routeName),
 		},
 		Target: &targetsTarget,
 		Weight: &targetsWeight,
 	}
 
-	// this is a special case with the kong admin API - this endpoint /upstreams/:upstreamName/targets actually accepts multiple POST requests, so this is not a mistake
 	targetsResponse, err := kongAdminApi.CreateTargetForUpstreamWithResponse(ctx, upstreamName, targetsBody)
 	if err != nil {
-		return errors.Wrap(err, "failed to create targets for upstream")
+		return "", errors.Wrapf(err, "failed to create target for upstream [POST /upstreams/%s/targets]", upstreamName)
 	}
-	if err := client.CheckStatusCode(targetsResponse, 200, 201); err != nil {
-		return errors.Wrap(fmt.Errorf("error body from kong admin api: %s", string(targetsResponse.Body)), "failed to create targets for upstream")
+	if err := client.CheckStatusCode(targetsResponse, 200); err != nil {
+		return "", errors.Wrap(
+			fmt.Errorf("error body from kong admin api [%s %s]: %s",
+				targetsResponse.HTTPResponse.Request.Method,
+				targetsResponse.HTTPResponse.Request.URL.Path,
+				string(targetsResponse.Body)),
+			"failed to create target for upstream")
 	}
-	route.SetTargetsId(*targetsResponse.JSON200.Id)
-
-	return nil
+	return *targetsResponse.JSON200.Id, nil
 }

gateway/internal/features/feature/circuit_breaker_test.go

@@ -6,12 +6,14 @@ package feature_test
 
 import (
 	"context"
+	"fmt"
+	"net/http"
+
 	"github.com/telekom/controlplane/common/pkg/util/contextutil"
 	"github.com/telekom/controlplane/gateway/internal/features/feature/config"
 	kong "github.com/telekom/controlplane/gateway/pkg/kong/api"
 	"github.com/telekom/controlplane/gateway/pkg/kong/client/mock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"net/http"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -113,7 +115,7 @@ var _ = Describe("CircuitBreakerFeature", func() {
 				Expect(err.Error()).Should(ContainSubstring("cannot find route"))
 			})
 
-			It("should create kong upstream and targets and update feature builder upstream value", func() {
+			It("should create kong upstream and target on first reconciliation (target not found in Kong)", func() {
 				// Setup
 				ctx := context.Background()
 				ctx = contextutil.WithEnv(ctx, "test")
@@ -155,7 +157,13 @@ var _ = Describe("CircuitBreakerFeature", func() {
 				}
 				mockKongAdminApi.EXPECT().UpsertUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(upsertUpstreamWithResponse_func).Times(1)
 
-				// mock CreateTargetForUpstreamWithResponse
+				// mock FetchTargetForUpstreamWithResponse — target does not exist yet (404)
+				mockKongAdminApi.EXPECT().FetchTargetForUpstreamWithResponse(gomock.Any(), gomock.Eq("test-route-name"), gomock.Eq("localhost:8080"), gomock.Any()).
+					Return(&kong.FetchTargetForUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 404},
+					}, nil).Times(1)
+
+				// mock CreateTargetForUpstreamWithResponse — target is created
 				var createTargetForUpstreamWithResponse_upstreamNameArg string
 				var createTargetForUpstreamWithResponse_targetBodyArg kong.CreateTargetForUpstreamJSONRequestBody
 
@@ -180,7 +188,6 @@ var _ = Describe("CircuitBreakerFeature", func() {
 
 				// Verify
 				Expect(err).Should(Not(HaveOccurred()))
-				// pointer vs non-pointer
 				Expect(*setUpstreamArg).To(BeEquivalentTo(client.CustomUpstream{
 					Scheme: "http",
 					Host:   "test-route-name",
@@ -191,7 +198,6 @@ var _ = Describe("CircuitBreakerFeature", func() {
 				Expect(upsertUpstreamWithResponse_upstreamNameArg).To(Equal("test-route-name"))
 
 				expectedUpstreamBody := createTestCreateUpstreamJSONRequestBody(ctx, "test-route-name")
-				// pointer vs non-pointer
 				Expect(upsertUpstreamWithResponse_upstreamBodyArg).To(Equal(*expectedUpstreamBody))
 				Expect(route.GetUpstreamId()).To(Equal("kong_upstream_response_id"))
 
@@ -206,6 +212,166 @@ var _ = Describe("CircuitBreakerFeature", func() {
 				Expect(route.GetTargetsId()).To(Equal("kong_target_response_id"))
 			})
 
+			It("should upsert upstream but skip target creation when target already exists in Kong", func() {
+				// Setup
+				ctx := context.Background()
+				ctx = contextutil.WithEnv(ctx, "test")
+				route := &gatewayv1.Route{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-route-name",
+					},
+					Spec: gatewayv1.RouteSpec{
+						Traffic: gatewayv1.Traffic{
+							CircuitBreaker: &gatewayv1.CircuitBreaker{
+								Enabled: true,
+							},
+						},
+					},
+				}
+				mockFeatureBuilder.EXPECT().GetRoute().Return(route, true).Times(1)
+				mockFeatureBuilder.EXPECT().GetKongClient().Return(mockKongClient).Times(1)
+				mockKongClient.EXPECT().GetKongAdminApi().Return(mockKongAdminApi).Times(1)
+				mockFeatureBuilder.EXPECT().SetUpstream(gomock.Any())
+
+				// mock UpsertUpstreamWithResponse — still called on every reconciliation
+				upsertUpstreamResponseId := "kong_upstream_response_id"
+				mockKongAdminApi.EXPECT().UpsertUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.UpsertUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Upstream{Id: &upsertUpstreamResponseId},
+					}, nil).Times(1)
+
+				// mock FetchTargetForUpstreamWithResponse — target already exists in Kong
+				existingTargetId := "existing-kong-target-id"
+				mockKongAdminApi.EXPECT().FetchTargetForUpstreamWithResponse(gomock.Any(), gomock.Eq("test-route-name"), gomock.Eq("localhost:8080"), gomock.Any()).
+					Return(&kong.FetchTargetForUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Target{Id: &existingTargetId},
+					}, nil).Times(1)
+
+				// CreateTargetForUpstreamWithResponse must NOT be called — target already exists
+				mockKongAdminApi.EXPECT().CreateTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0)
+
+				// Execute
+				err := feature.InstanceCircuitBreakerFeature.Apply(ctx, mockFeatureBuilder)
+
+				// Verify
+				Expect(err).Should(Not(HaveOccurred()))
+				Expect(route.GetUpstreamId()).To(Equal("kong_upstream_response_id"))
+				Expect(route.GetTargetsId()).To(Equal("existing-kong-target-id"))
+			})
+
+			It("should return error when FetchTargetForUpstream fails", func() {
+				ctx := context.Background()
+				ctx = contextutil.WithEnv(ctx, "test")
+				route := &gatewayv1.Route{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-route-name"},
+					Spec: gatewayv1.RouteSpec{
+						Traffic: gatewayv1.Traffic{
+							CircuitBreaker: &gatewayv1.CircuitBreaker{Enabled: true},
+						},
+					},
+				}
+				mockFeatureBuilder.EXPECT().GetRoute().Return(route, true).Times(1)
+				mockFeatureBuilder.EXPECT().GetKongClient().Return(mockKongClient).Times(1)
+				mockKongClient.EXPECT().GetKongAdminApi().Return(mockKongAdminApi).Times(1)
+				mockFeatureBuilder.EXPECT().SetUpstream(gomock.Any())
+
+				upsertUpstreamResponseId := "kong_upstream_response_id"
+				mockKongAdminApi.EXPECT().UpsertUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.UpsertUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Upstream{Id: &upsertUpstreamResponseId},
+					}, nil).Times(1)
+
+				mockKongAdminApi.EXPECT().FetchTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(nil, fmt.Errorf("connection refused")).Times(1)
+
+				err := feature.InstanceCircuitBreakerFeature.Apply(ctx, mockFeatureBuilder)
+				Expect(err).Should(HaveOccurred())
+				Expect(err.Error()).Should(ContainSubstring("failed to fetch target for upstream"))
+			})
+
+			It("should create target when FetchTargetForUpstream returns 404 (new upstream)", func() {
+				ctx := context.Background()
+				ctx = contextutil.WithEnv(ctx, "test")
+				route := &gatewayv1.Route{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-route-name"},
+					Spec: gatewayv1.RouteSpec{
+						Traffic: gatewayv1.Traffic{
+							CircuitBreaker: &gatewayv1.CircuitBreaker{Enabled: true},
+						},
+					},
+				}
+				mockFeatureBuilder.EXPECT().GetRoute().Return(route, true).Times(1)
+				mockFeatureBuilder.EXPECT().GetKongClient().Return(mockKongClient).Times(1)
+				mockKongClient.EXPECT().GetKongAdminApi().Return(mockKongAdminApi).Times(1)
+				mockFeatureBuilder.EXPECT().SetUpstream(gomock.Any())
+
+				upsertUpstreamResponseId := "kong_upstream_response_id"
+				mockKongAdminApi.EXPECT().UpsertUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.UpsertUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Upstream{Id: &upsertUpstreamResponseId},
+					}, nil).Times(1)
+
+				// FetchTargetForUpstream returns 404 — target doesn't exist yet
+				mockKongAdminApi.EXPECT().FetchTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.FetchTargetForUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 404},
+					}, nil).Times(1)
+
+				// Since 404 means no target, CreateTargetForUpstream should be called
+				createTargetResponseId := "new-target-id"
+				mockKongAdminApi.EXPECT().CreateTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.CreateTargetForUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Target{Id: &createTargetResponseId},
+					}, nil).Times(1)
+
+				err := feature.InstanceCircuitBreakerFeature.Apply(ctx, mockFeatureBuilder)
+				Expect(err).Should(Not(HaveOccurred()))
+				Expect(route.GetTargetsId()).To(Equal("new-target-id"))
+			})
+
+			It("should return error when CreateTargetForUpstream fails", func() {
+				ctx := context.Background()
+				ctx = contextutil.WithEnv(ctx, "test")
+				route := &gatewayv1.Route{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-route-name"},
+					Spec: gatewayv1.RouteSpec{
+						Traffic: gatewayv1.Traffic{
+							CircuitBreaker: &gatewayv1.CircuitBreaker{Enabled: true},
+						},
+					},
+				}
+				mockFeatureBuilder.EXPECT().GetRoute().Return(route, true).Times(1)
+				mockFeatureBuilder.EXPECT().GetKongClient().Return(mockKongClient).Times(1)
+				mockKongClient.EXPECT().GetKongAdminApi().Return(mockKongAdminApi).Times(1)
+				mockFeatureBuilder.EXPECT().SetUpstream(gomock.Any())
+
+				upsertUpstreamResponseId := "kong_upstream_response_id"
+				mockKongAdminApi.EXPECT().UpsertUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.UpsertUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 200},
+						JSON200:      &kong.Upstream{Id: &upsertUpstreamResponseId},
+					}, nil).Times(1)
+
+				// Target does not exist
+				mockKongAdminApi.EXPECT().FetchTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(&kong.FetchTargetForUpstreamResponse{
+						HTTPResponse: &http.Response{StatusCode: 404},
+					}, nil).Times(1)
+
+				// CreateTargetForUpstream fails
+				mockKongAdminApi.EXPECT().CreateTargetForUpstreamWithResponse(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
+					Return(nil, fmt.Errorf("connection refused")).Times(1)
+
+				err := feature.InstanceCircuitBreakerFeature.Apply(ctx, mockFeatureBuilder)
+				Expect(err).Should(HaveOccurred())
+				Expect(err.Error()).Should(ContainSubstring("failed to create target for upstream"))
+			})
+
 			It("should delete kong upstream and targets if CB is disabled and upstreamId is not empty", func() {
 				// Setup
 				ctx := context.Background()

gateway/pkg/kong/client/client.go

@@ -52,6 +52,7 @@
 
 	UpsertUpstreamWithResponse(ctx context.Context, upstreamIdOrName string, body kong.UpsertUpstreamJSONRequestBody, reqEditors ...kong.RequestEditorFn) (*kong.UpsertUpstreamResponse, error)
 	CreateTargetForUpstreamWithResponse(ctx context.Context, upstreamIdOrName string, body kong.CreateTargetForUpstreamJSONRequestBody, reqEditors ...kong.RequestEditorFn) (*kong.CreateTargetForUpstreamResponse, error)
+	FetchTargetForUpstreamWithResponse(ctx context.Context, upstreamIdOrName string, targetIdOrTarget string, reqEditors ...kong.RequestEditorFn) (*kong.FetchTargetForUpstreamResponse, error)
 	DeleteUpstreamWithResponse(ctx context.Context, upstreamIdOrName string, reqEditors ...kong.RequestEditorFn) (*kong.DeleteUpstreamResponse, error)
 	DeleteUpstreamTargetWithResponse(ctx context.Context, upstreamIdOrName string, targetIdOrTarget string, reqEditors ...kong.RequestEditorFn) (*kong.DeleteUpstreamTargetResponse, error)
 
@@ -71,7 +72,7 @@
 var _ KongClient = &kongClient{}
 
 type kongClient struct {
 	//client     kong.ClientWithResponsesInterface
 	client     KongAdminApi
 	commonTags []string
 }