diff --git a/gateway/internal/features/feature/circuit_breaker.go b/gateway/internal/features/feature/circuit_breaker.go index 2febcdd31..08e5e139e 100644 --- a/gateway/internal/features/feature/circuit_breaker.go +++ b/gateway/internal/features/feature/circuit_breaker.go @@ -7,6 +7,7 @@ package feature import ( "context" "fmt" + "github.com/go-logr/logr" "github.com/pkg/errors" "github.com/telekom/controlplane/common/pkg/util/contextutil" @@ -161,35 +162,81 @@ func handleApply(ctx context.Context, builder features.FeaturesBuilder, route *g 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 + targetId := route.GetTargetsId() + if targetId == "" { + 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 } diff --git a/gateway/internal/features/feature/circuit_breaker_test.go b/gateway/internal/features/feature/circuit_breaker_test.go index 746abe61f..17b9352b2 100644 --- a/gateway/internal/features/feature/circuit_breaker_test.go +++ b/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() diff --git a/gateway/pkg/kong/client/client.go b/gateway/pkg/kong/client/client.go index eb04fe83d..9a76b1dfa 100644 --- a/gateway/pkg/kong/client/client.go +++ b/gateway/pkg/kong/client/client.go @@ -52,6 +52,7 @@ type KongAdminApi interface { 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) diff --git a/gateway/pkg/kong/client/mock/client.gen.go b/gateway/pkg/kong/client/mock/client.gen.go index abfc525fb..427bc937f 100644 --- a/gateway/pkg/kong/client/mock/client.gen.go +++ b/gateway/pkg/kong/client/mock/client.gen.go @@ -413,6 +413,26 @@ func (mr *MockKongAdminApiMockRecorder) ListPluginWithResponse(ctx, params any, return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListPluginWithResponse", reflect.TypeOf((*MockKongAdminApi)(nil).ListPluginWithResponse), varargs...) } +// FetchTargetForUpstreamWithResponse mocks base method. +func (m *MockKongAdminApi) FetchTargetForUpstreamWithResponse(ctx context.Context, upstreamIdOrName, targetIdOrTarget string, reqEditors ...kong.RequestEditorFn) (*kong.FetchTargetForUpstreamResponse, error) { + m.ctrl.T.Helper() + varargs := []any{ctx, upstreamIdOrName, targetIdOrTarget} + for _, a := range reqEditors { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "FetchTargetForUpstreamWithResponse", varargs...) + ret0, _ := ret[0].(*kong.FetchTargetForUpstreamResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchTargetForUpstreamWithResponse indicates an expected call of FetchTargetForUpstreamWithResponse. +func (mr *MockKongAdminApiMockRecorder) FetchTargetForUpstreamWithResponse(ctx, upstreamIdOrName, targetIdOrTarget any, reqEditors ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx, upstreamIdOrName, targetIdOrTarget}, reqEditors...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchTargetForUpstreamWithResponse", reflect.TypeOf((*MockKongAdminApi)(nil).FetchTargetForUpstreamWithResponse), varargs...) +} + // UpsertConsumerWithResponse mocks base method. func (m *MockKongAdminApi) UpsertConsumerWithResponse(ctx context.Context, consumerUsernameOrId string, body kong.UpsertConsumerJSONRequestBody, reqEditors ...kong.RequestEditorFn) (*kong.UpsertConsumerResponse, error) { m.ctrl.T.Helper()