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.

Sign up for GitHub

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

Jump to bottom

SB: Make sure Service Bindings do not do not block Service Instance deletion #1362

Merged
merged 8 commits into from
Oct 23, 2024
Merged

SB: Make sure Service Bindings do not do not block Service Instance deletion #1362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5b1adda
initial version of deprovisioning test
jaroslaw-pieszka Oct 18, 2024
9f5fc93
gi imports
jaroslaw-pieszka Oct 22, 2024
204f095
review remark
jaroslaw-pieszka Oct 23, 2024
a8da3d3
review remark
jaroslaw-pieszka Oct 23, 2024
e4391ce
review remark
jaroslaw-pieszka Oct 23, 2024
47f7831
http method enum used
jaroslaw-pieszka Oct 23, 2024
a07c9cc
name corrected
jaroslaw-pieszka Oct 23, 2024
0ed8383
Merge branch 'main' into handle-deprovisoning-scenarios
jaroslaw-pieszka Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions cmd/broker/bind_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,21 @@ func TestCreateBindingEndpoint(t *testing.T) {

err = db.Instances().Insert(fixture.FixInstance(instanceID1))
require.NoError(t, err)
operation := fixture.FixOperation("operation-001", instanceID1, internal.OperationTypeProvision)
err = db.Operations().InsertOperation(operation)
require.NoError(t, err)

err = db.Instances().Insert(fixture.FixInstance(instanceID2))
require.NoError(t, err)
operation = fixture.FixOperation("operation-002", instanceID2, internal.OperationTypeProvision)
err = db.Operations().InsertOperation(operation)
require.NoError(t, err)

err = db.Instances().Insert(fixture.FixInstance(instanceID3))
require.NoError(t, err)
operation = fixture.FixOperation("operation-003", instanceID3, internal.OperationTypeProvision)
err = db.Operations().InsertOperation(operation)
require.NoError(t, err)

skrK8sClientProvider := kubeconfig.NewK8sClientFromSecretProvider(kcpClient)

Expand All @@ -190,9 +199,9 @@ func TestCreateBindingEndpoint(t *testing.T) {
}

//// api handler
bindEndpoint := broker.NewBind(*bindingCfg, db.Instances(), db.Bindings(), logs, skrK8sClientProvider, skrK8sClientProvider)
getBindingEndpoint := broker.NewGetBinding(logs, db.Bindings())
unbindEndpoint := broker.NewUnbind(logs, db.Bindings(), db.Instances(), brokerBindings.NewServiceAccountBindingsManager(skrK8sClientProvider, skrK8sClientProvider))
bindEndpoint := broker.NewBind(*bindingCfg, db, logs, skrK8sClientProvider, skrK8sClientProvider)
getBindingEndpoint := broker.NewGetBinding(logs, db)
unbindEndpoint := broker.NewUnbind(logs, db, brokerBindings.NewServiceAccountBindingsManager(skrK8sClientProvider, skrK8sClientProvider))
apiHandler := handlers.NewApiHandler(broker.KymaEnvironmentBroker{
ServicesEndpoint: nil,
ProvisionEndpoint: nil,
Expand Down
151 changes: 141 additions & 10 deletions cmd/broker/binding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package main
import (
"encoding/json"
"fmt"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"io"
"net/http"
"testing"

"github.com/google/uuid"
"github.com/pivotal-cf/brokerapi/v8/domain"
"github.com/stretchr/testify/assert"
)

type ErrorResponse struct {
Expand All @@ -23,7 +26,7 @@ func TestBinding(t *testing.T) {
iid := uuid.New().String()
bid := uuid.New().String()

resp := suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s?accepts_incomplete=true", iid),
resp := suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s?accepts_incomplete=true", iid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
Expand All @@ -42,7 +45,7 @@ func TestBinding(t *testing.T) {
suite.WaitForOperationState(opID, domain.Succeeded)

// when
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15"
Expand All @@ -52,13 +55,13 @@ func TestBinding(t *testing.T) {
suite.Log(string(b))
suite.Log(resp.Status)

respRuntimes := suite.CallAPI("GET", "/runtimes?bindings=true", "")
respRuntimes := suite.CallAPI(http.MethodGet, "/runtimes?bindings=true", "")
b, _ = io.ReadAll(respRuntimes.Body)
suite.Log(string(b))
suite.Log(resp.Status)

t.Run("should return 400 when expiration seconds parameter is string instead of int", func(t *testing.T) {
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
Expand All @@ -80,15 +83,15 @@ func TestBinding(t *testing.T) {

t.Run("should return 200 when creating a second binding with the same id and params as an existing one", func(t *testing.T) {
bid = uuid.New().String()
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
"parameters": {
"expiration_seconds": 600
}
}`)
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
Expand All @@ -99,7 +102,7 @@ func TestBinding(t *testing.T) {
assert.Equal(t, http.StatusOK, resp.StatusCode)

// "expiration_seconds": 600 is a default value from the config in tests
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15"
Expand All @@ -109,15 +112,15 @@ func TestBinding(t *testing.T) {

t.Run("should return 409 when creating a second binding with the same id as an existing one but different params", func(t *testing.T) {
bid = uuid.New().String()
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
"parameters": {
"expiration_seconds": 900
}
}`)
resp = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
resp = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
Expand All @@ -130,3 +133,131 @@ func TestBinding(t *testing.T) {
})

}

func TestDeprovisioningWithExistingBindings(t *testing.T) {
// given
cfg := fixConfig()
// Disable EDP to have all steps successfully executed
cfg.EDP.Disabled = true
suite := NewBrokerSuiteTestWithConfig(t, cfg)
defer suite.TearDown()
iid := uuid.New().String()
bindingID1 := uuid.New().String()
bindingID2 := uuid.New().String()

response := suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s?accepts_incomplete=true", iid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15",
"context": {
"globalaccount_id": "g-account-id",
"subaccount_id": "sub-id",
"user_id": "[email protected]"
},
"parameters": {
"name": "testing-cluster",
"region": "eu-central-1"
}
}`)
opID := suite.DecodeOperationID(response)
suite.processProvisioningByOperationID(opID)
suite.WaitForOperationState(opID, domain.Succeeded)

// when we create two bindings
response = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bindingID1),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15"
}`)
require.Equal(t, http.StatusCreated, response.StatusCode)

response = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bindingID2),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "361c511f-f939-4621-b228-d0fb79a1fe15"
}`)
require.Equal(t, http.StatusCreated, response.StatusCode)

// when we deprovision successfully
response = suite.CallAPI(http.MethodDelete, fmt.Sprintf("oauth/v2/service_instances/%s?accepts_incomplete=true&plan_id=361c511f-f939-4621-b228-d0fb79a1fe15&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid),
``)
deprovisioningID := suite.DecodeOperationID(response)
suite.FinishDeprovisioningOperationByProvisioner(deprovisioningID)
suite.WaitForInstanceRemoval(iid)
ralikio marked this conversation as resolved.
Show resolved Hide resolved

// when we remove bindings and the instance is already removed
response = suite.CallAPI(http.MethodDelete, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s?plan_id=361c511f-f939-4621-b228-d0fb79a1fe15&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid, bindingID1), "")
assert.Equal(t, http.StatusGone, response.StatusCode)
response = suite.CallAPI(http.MethodDelete, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s?plan_id=361c511f-f939-4621-b228-d0fb79a1fe15&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid, bindingID2), "")
assert.Equal(t, http.StatusGone, response.StatusCode)

// then expect bindings to be removed
suite.AssertBindingRemoval(iid, bindingID1)
suite.AssertBindingRemoval(iid, bindingID2)
}

func TestRemoveBindingsFromSuspended(t *testing.T) {
ralikio marked this conversation as resolved.
Show resolved Hide resolved
// given
cfg := fixConfig()
cfg.Broker.Binding.BindablePlans = []string{"trial"}
suite := NewBrokerSuiteTestWithConfig(t, cfg)
defer suite.TearDown()
iid := uuid.New().String()
bindingID1 := uuid.New().String()
bindingID2 := uuid.New().String()

response := suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/cf-eu10/v2/service_instances/%s?accepts_incomplete=true&plan_id=7d55d31d-35ae-4438-bf13-6ffdfa107d9f&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "7d55d31d-35ae-4438-bf13-6ffdfa107d9f",
"context": {
"globalaccount_id": "g-account-id",
"subaccount_id": "sub-id",
"user_id": "[email protected]"
},
"parameters": {
"name": "testing-cluster"
}
}`)
opID := suite.DecodeOperationID(response)
suite.processProvisioningByOperationID(opID)
suite.WaitForOperationState(opID, domain.Succeeded)

//then we create two bindings
response = suite.CallAPI(http.MethodPut, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bindingID1),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "7d55d31d-35ae-4438-bf13-6ffdfa107d9f"
}`)
require.Equal(t, http.StatusCreated, response.StatusCode)

response = suite.CallAPI("PUT", fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s", iid, bindingID2),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "7d55d31d-35ae-4438-bf13-6ffdfa107d9f"
}`)
require.Equal(t, http.StatusCreated, response.StatusCode)

//when we suspend Service instance - OSB context update
response = suite.CallAPI(http.MethodPatch, fmt.Sprintf("oauth/cf-eu10/v2/service_instances/%s?accepts_incomplete=true", iid),
`{
"service_id": "47c9dcbf-ff30-448e-ab36-d3bad66ba281",
"plan_id": "7d55d31d-35ae-4438-bf13-6ffdfa107d9f",
"context": {
"globalaccount_id": "g-account-id",
"user_id": "[email protected]",
"active": false
}
}`)
assert.Equal(t, http.StatusOK, response.StatusCode)
suspensionOpID := suite.WaitForLastOperation(iid, domain.InProgress)

suite.FinishDeprovisioningOperationByProvisioner(suspensionOpID)
suite.WaitForOperationState(suspensionOpID, domain.Succeeded)

// when we remove bindings we just return OK
response = suite.CallAPI(http.MethodDelete, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s?plan_id=361c511f-f939-4621-b228-d0fb79a1fe15&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid, bindingID1), "")
assert.Equal(t, http.StatusOK, response.StatusCode)
response = suite.CallAPI(http.MethodDelete, fmt.Sprintf("oauth/v2/service_instances/%s/service_bindings/%s?plan_id=361c511f-f939-4621-b228-d0fb79a1fe15&service_id=47c9dcbf-ff30-448e-ab36-d3bad66ba281", iid, bindingID2), "")
assert.Equal(t, http.StatusOK, response.StatusCode)
}
17 changes: 16 additions & 1 deletion cmd/broker/broker_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
"testing"
"time"

"github.com/kyma-project/kyma-environment-broker/internal/storage/dberr"

imv1 "github.com/kyma-project/infrastructure-manager/api/v1"

"github.com/kyma-project/kyma-environment-broker/internal/kubeconfig"
Expand Down Expand Up @@ -407,7 +409,7 @@ func (s *BrokerSuiteTest) WaitForOperationState(operationID string, state domain
}
return op.State == state, nil
})
assert.NoError(s.t, err, "timeout waiting for the operation expected state %s != %s. The existing operation %+v", state, op.State, op)
assert.NoError(s.t, err, "timeout waiting for the operation expected state %s. The existing operation %+v", state, op)
}

func (s *BrokerSuiteTest) GetOperation(operationID string) *internal.Operation {
Expand All @@ -431,6 +433,19 @@ func (s *BrokerSuiteTest) WaitForLastOperation(iid string, state domain.LastOper
return op.ID
}

func (s *BrokerSuiteTest) WaitForInstanceRemoval(iid string) {
err := s.poller.Invoke(func() (done bool, err error) {
_, err = s.db.Instances().GetByID(iid)
return dberr.IsNotFound(err), nil
})
assert.NoError(s.t, err, "timeout waiting for the instance %s to be removed", iid)
}

func (s *BrokerSuiteTest) AssertBindingRemoval(iid string, bindingID string) {
_, err := s.db.Bindings().Get(iid, bindingID)
assert.True(s.t, dberr.IsNotFound(err), "bindings should be removed")
}

func (s *BrokerSuiteTest) LastOperation(iid string) *internal.Operation {
op, _ := s.db.Operations().GetLastOperation(iid)
return op
Expand Down
6 changes: 3 additions & 3 deletions cmd/broker/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,9 +447,9 @@ func createAPI(router *mux.Router, servicesConfig broker.ServicesConfig, planVal
planDefaults, logs, cfg.KymaDashboardConfig, kcBuilder, convergedCloudRegionProvider, kcpK8sClient),
GetInstanceEndpoint: broker.NewGetInstance(cfg.Broker, db.Instances(), db.Operations(), kcBuilder, logs),
LastOperationEndpoint: broker.NewLastOperation(db.Operations(), db.InstancesArchived(), logs),
BindEndpoint: broker.NewBind(cfg.Broker.Binding, db.Instances(), db.Bindings(), logs, clientProvider, kubeconfigProvider),
UnbindEndpoint: broker.NewUnbind(logs, db.Bindings(), db.Instances(), brokerBindings.NewServiceAccountBindingsManager(clientProvider, kubeconfigProvider)),
GetBindingEndpoint: broker.NewGetBinding(logs, db.Bindings()),
BindEndpoint: broker.NewBind(cfg.Broker.Binding, db, logs, clientProvider, kubeconfigProvider),
UnbindEndpoint: broker.NewUnbind(logs, db, brokerBindings.NewServiceAccountBindingsManager(clientProvider, kubeconfigProvider)),
GetBindingEndpoint: broker.NewGetBinding(logs, db),
LastBindingOperationEndpoint: broker.NewLastBindingOperation(logs),
}

Expand Down
6 changes: 3 additions & 3 deletions internal/broker/bind_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,10 @@ type Credentials struct {
Kubeconfig string `json:"kubeconfig"`
}

func NewBind(cfg BindingConfig, instanceStorage storage.Instances, bindingsStorage storage.Bindings, log logrus.FieldLogger, clientProvider broker.ClientProvider, kubeconfigProvider broker.KubeconfigProvider) *BindEndpoint {
func NewBind(cfg BindingConfig, db storage.BrokerStorage, log logrus.FieldLogger, clientProvider broker.ClientProvider, kubeconfigProvider broker.KubeconfigProvider) *BindEndpoint {
return &BindEndpoint{config: cfg,
instancesStorage: instanceStorage,
bindingsStorage: bindingsStorage,
instancesStorage: db.Instances(),
bindingsStorage: db.Bindings(),
log: log.WithField("service", "BindEndpoint"),
serviceAccountBindingManager: broker.NewServiceAccountBindingsManager(clientProvider, kubeconfigProvider),
}
Expand Down
24 changes: 12 additions & 12 deletions internal/broker/bind_create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,13 @@ func TestCreateSecondBindingWithTheSameIdButDifferentParams(t *testing.T) {
MaxBindingsCount: 10,
}
binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
st := storage.NewMemoryStorage()
err := st.Instances().Insert(instance)
brokerStorage := storage.NewMemoryStorage()
err := brokerStorage.Instances().Insert(instance)
assert.NoError(t, err)
err = st.Bindings().Insert(&binding)
err = brokerStorage.Bindings().Insert(&binding)
assert.NoError(t, err)

svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
svc := NewBind(*bindingCfg, brokerStorage, logrus.New(), nil, nil)
params := BindingParams{
ExpirationSeconds: 601,
}
Expand Down Expand Up @@ -123,13 +123,13 @@ func TestCreateSecondBindingWithTheSameIdAndParams(t *testing.T) {
MaxBindingsCount: 10,
}
binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
st := storage.NewMemoryStorage()
err := st.Instances().Insert(instance)
brokerStorage := storage.NewMemoryStorage()
err := brokerStorage.Instances().Insert(instance)
assert.NoError(t, err)
err = st.Bindings().Insert(&binding)
err = brokerStorage.Bindings().Insert(&binding)
assert.NoError(t, err)

svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
svc := NewBind(*bindingCfg, brokerStorage, logrus.New(), nil, nil)
params := BindingParams{
ExpirationSeconds: 600,
}
Expand Down Expand Up @@ -164,13 +164,13 @@ func TestCreateSecondBindingWithTheSameIdAndParamsNotExplicitlyDefined(t *testin
MaxBindingsCount: 10,
}
binding := fixture.FixBindingWithInstanceID(bindingID, instanceID)
st := storage.NewMemoryStorage()
err := st.Instances().Insert(instance)
brokerStorage := storage.NewMemoryStorage()
err := brokerStorage.Instances().Insert(instance)
assert.NoError(t, err)
err = st.Bindings().Insert(&binding)
err = brokerStorage.Bindings().Insert(&binding)
assert.NoError(t, err)

svc := NewBind(*bindingCfg, st.Instances(), st.Bindings(), logrus.New(), nil, nil)
svc := NewBind(*bindingCfg, brokerStorage, logrus.New(), nil, nil)

// when
resp, err := svc.Bind(context.Background(), instanceID, bindingID, domain.BindDetails{}, false)
Expand Down
Loading
Loading