Skip to content

Commit c6e1f2b

Browse files
AndresNicoAndresNico
authored
fix[214]: Update ServiceInstance controller to accept servicePlan.id only (#215)
* Update ServiceInstance controller to also accept servicePlan.id only * Disable gocyclo check * Refactor into serviceplanresolver
1 parent 73f0431 commit c6e1f2b

5 files changed

Lines changed: 301 additions & 18 deletions

File tree

‎internal/clients/fake/serviceplan.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package fake
2+
3+
import (
4+
"context"
5+
6+
"github.com/cloudfoundry/go-cfclient/v3/client"
7+
"github.com/cloudfoundry/go-cfclient/v3/resource"
8+
"github.com/stretchr/testify/mock"
9+
)
10+
11+
type MockServicePlan struct {
12+
mock.Mock
13+
}
14+
15+
func (m *MockServicePlan) Get(ctx context.Context, guid string) (*resource.ServicePlan, error) {
16+
args := m.Called(guid)
17+
return args.Get(0).(*resource.ServicePlan), args.Error(1)
18+
}
19+
20+
func (m *MockServicePlan) Single(ctx context.Context, opts *client.ServicePlanListOptions) (*resource.ServicePlan, error) {
21+
args := m.Called()
22+
return args.Get(0).(*resource.ServicePlan), args.Error(1)
23+
}

‎internal/clients/serviceinstance/serviceinstance.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,11 +71,12 @@ func (c *Client) pollJobComplete(ctx context.Context, job string) error {
7171
type Client struct {
7272
ServiceInstance
7373
Job
74+
ServicePlanResolver
7475
}
7576

7677
// NewClient creates a new client instance from a cfclient.ServiceInstance instance.
7778
func NewClient(cf *client.Client) *Client {
78-
return &Client{cf.ServiceInstances, cf.Jobs}
79+
return &Client{cf.ServiceInstances, cf.Jobs, cf.ServicePlans}
7980
}
8081

8182
// GetByIDOrSpec retrieves external resource by GUID or by matching CR's ForProvider spec

‎internal/clients/serviceinstance/serviceplanresolver.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package serviceinstance
2+
3+
import (
4+
"context"
5+
6+
"github.com/cloudfoundry/go-cfclient/v3/client"
7+
"github.com/cloudfoundry/go-cfclient/v3/resource"
8+
"github.com/crossplane/crossplane-runtime/pkg/errors"
9+
k8s "sigs.k8s.io/controller-runtime/pkg/client"
10+
11+
"github.com/SAP/crossplane-provider-cloudfoundry/apis/resources/v1alpha1"
12+
)
13+
14+
const (
15+
errMissingServicePlan = "managed resource service instance requires a service plan"
16+
)
17+
18+
type ServicePlan interface {
19+
Get(ctx context.Context, guid string) (*resource.ServicePlan, error)
20+
Single(ctx context.Context, opts *client.ServicePlanListOptions) (*resource.ServicePlan, error)
21+
}
22+
23+
type ServicePlanResolver interface {
24+
ServicePlan
25+
}
26+
27+
// We either populate/update the service plan ID with the external resource GUID
28+
// based on the specified offering and plan or we use the provided ID directly.
29+
func (c *Client) ResolveServicePlan(ctx context.Context, kube k8s.Client, cr *v1alpha1.ServiceInstance) error {
30+
if cr.Spec.ForProvider.ServicePlan.Offering != nil && cr.Spec.ForProvider.ServicePlan.Plan != nil {
31+
return c.resolvePlanID(ctx, kube, cr)
32+
}
33+
34+
if cr.Spec.ForProvider.ServicePlan.ID != nil {
35+
return c.validatePlanID(ctx, cr)
36+
}
37+
38+
return errors.New(errMissingServicePlan)
39+
}
40+
41+
// Populate/Update service plan ID based on offering and plan
42+
func (c *Client) resolvePlanID(ctx context.Context, kube k8s.Client, cr *v1alpha1.ServiceInstance) error {
43+
opt := client.NewServicePlanListOptions()
44+
opt.ServiceOfferingNames.EqualTo(*cr.Spec.ForProvider.ServicePlan.Offering)
45+
opt.Names.EqualTo(*cr.Spec.ForProvider.ServicePlan.Plan)
46+
47+
res, err := c.ServicePlanResolver.Single(ctx, opt)
48+
if err != nil {
49+
return errors.Wrapf(err, "cannot initialize service plan using serviceName/servicePlanName: %s:%s", *cr.Spec.ForProvider.ServicePlan.Offering, *cr.Spec.ForProvider.ServicePlan.Plan)
50+
}
51+
52+
cr.Spec.ForProvider.ServicePlan.ID = &res.GUID
53+
return kube.Update(ctx, cr)
54+
}
55+
56+
// Verify whether service plan ID is valid
57+
func (c *Client) validatePlanID(ctx context.Context, cr *v1alpha1.ServiceInstance) error {
58+
_, err := c.ServicePlanResolver.Get(ctx, *cr.Spec.ForProvider.ServicePlan.ID)
59+
if err != nil {
60+
return errors.Wrapf(err, "cannot initialize service plan using ID: %s", *cr.Spec.ForProvider.ServicePlan.ID)
61+
}
62+
return nil
63+
}

‎internal/clients/serviceinstance/serviceplanresolver_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
package serviceinstance
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"testing"
7+
8+
"github.com/cloudfoundry/go-cfclient/v3/client"
9+
"github.com/cloudfoundry/go-cfclient/v3/resource"
10+
"github.com/crossplane/crossplane-runtime/pkg/errors"
11+
"github.com/crossplane/crossplane-runtime/pkg/test"
12+
"github.com/google/go-cmp/cmp"
13+
k8s "sigs.k8s.io/controller-runtime/pkg/client"
14+
15+
"github.com/SAP/crossplane-provider-cloudfoundry/apis/resources/v1alpha1"
16+
"github.com/SAP/crossplane-provider-cloudfoundry/internal/clients/fake"
17+
)
18+
19+
var (
20+
guid = "2d8b0d04-d537-4e4e-8c6f-f09ca0e7f56f"
21+
offering = "test-offering"
22+
plan = "test-plan"
23+
validateErrMsg = fmt.Sprintf("error executing GET request for %s: cfclient error (CF-ResourceNotFound|10010): Service plan not found", guid)
24+
25+
errValidate = errors.New(validateErrMsg)
26+
errExactlyOneResultNotReturned = client.ErrExactlyOneResultNotReturned
27+
)
28+
29+
type modifier func(*v1alpha1.ServiceInstance)
30+
31+
func withServicePlan(servicePlan v1alpha1.ServicePlanParameters) modifier {
32+
return func(r *v1alpha1.ServiceInstance) {
33+
r.Spec.ForProvider.ServicePlan = &servicePlan
34+
}
35+
}
36+
37+
func serviceInstance(m ...modifier) *v1alpha1.ServiceInstance {
38+
r := &v1alpha1.ServiceInstance{}
39+
for _, rm := range m {
40+
rm(r)
41+
}
42+
return r
43+
}
44+
45+
func TestResolveServicePlan(t *testing.T) {
46+
type service func() ServicePlan
47+
type args struct {
48+
ctx context.Context
49+
cr *v1alpha1.ServiceInstance
50+
}
51+
type want struct {
52+
err error
53+
}
54+
55+
cases := map[string]struct {
56+
args args
57+
want want
58+
service service
59+
kube k8s.Client
60+
}{
61+
"None": {
62+
args: args{
63+
ctx: context.Background(),
64+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{})),
65+
},
66+
want: want{
67+
err: errors.New(errMissingServicePlan),
68+
},
69+
service: func() ServicePlan {
70+
m := &fake.MockServicePlan{}
71+
return m
72+
},
73+
},
74+
"OnlyId": {
75+
args: args{
76+
ctx: context.Background(),
77+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{ID: &guid})),
78+
},
79+
want: want{
80+
err: nil,
81+
},
82+
service: func() ServicePlan {
83+
m := &fake.MockServicePlan{}
84+
m.On("Get", guid).Return(&resource.ServicePlan{
85+
Resource: resource.Resource{
86+
GUID: guid,
87+
},
88+
}, nil)
89+
return m
90+
},
91+
},
92+
"OfferingAndPlan": {
93+
args: args{
94+
ctx: context.Background(),
95+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{Offering: &offering, Plan: &plan})),
96+
},
97+
want: want{
98+
err: nil,
99+
},
100+
service: func() ServicePlan {
101+
m := &fake.MockServicePlan{}
102+
m.On("Single").Return(&resource.ServicePlan{
103+
Resource: resource.Resource{
104+
GUID: guid,
105+
},
106+
}, nil)
107+
return m
108+
},
109+
},
110+
"OnlyOffering": {
111+
args: args{
112+
ctx: context.Background(),
113+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{Offering: &offering})),
114+
},
115+
want: want{
116+
err: errors.New(errMissingServicePlan),
117+
},
118+
service: func() ServicePlan {
119+
m := &fake.MockServicePlan{}
120+
m.On("Single").Return(&resource.ServicePlan{}, errors.New("failed"))
121+
return m
122+
},
123+
},
124+
"OnlyPlan": {
125+
args: args{
126+
ctx: context.Background(),
127+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{Plan: &plan})),
128+
},
129+
want: want{
130+
err: errors.New(errMissingServicePlan),
131+
},
132+
service: func() ServicePlan {
133+
m := &fake.MockServicePlan{}
134+
m.On("Single").Return(&resource.ServicePlan{}, errors.New("failed"))
135+
return m
136+
},
137+
},
138+
"All": {
139+
args: args{
140+
ctx: context.Background(),
141+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{ID: &guid, Plan: &plan, Offering: &offering})),
142+
},
143+
want: want{
144+
err: nil,
145+
},
146+
service: func() ServicePlan {
147+
m := &fake.MockServicePlan{}
148+
m.On("Single").Return(&resource.ServicePlan{
149+
Resource: resource.Resource{
150+
GUID: guid,
151+
},
152+
}, nil)
153+
return m
154+
},
155+
},
156+
"ErrorResolvePlanID": {
157+
args: args{
158+
ctx: context.Background(),
159+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{Plan: &plan, Offering: &offering})),
160+
},
161+
want: want{
162+
err: errors.Wrapf(errExactlyOneResultNotReturned, "cannot initialize service plan using serviceName/servicePlanName: %s:%s", offering, plan),
163+
},
164+
service: func() ServicePlan {
165+
m := &fake.MockServicePlan{}
166+
m.On("Single").Return(&resource.ServicePlan{}, errExactlyOneResultNotReturned)
167+
return m
168+
},
169+
},
170+
"ErrorValidatePlanID": {
171+
args: args{
172+
ctx: context.Background(),
173+
cr: serviceInstance(withServicePlan(v1alpha1.ServicePlanParameters{ID: &guid})),
174+
},
175+
want: want{
176+
err: errors.Wrapf(errValidate, "cannot initialize service plan using ID: %s", guid),
177+
},
178+
service: func() ServicePlan {
179+
m := &fake.MockServicePlan{}
180+
m.On("Get", guid).Return(&resource.ServicePlan{}, errValidate)
181+
return m
182+
},
183+
},
184+
}
185+
for n, tc := range cases {
186+
t.Run(n, func(t *testing.T) {
187+
c := &Client{
188+
ServicePlanResolver: tc.service(),
189+
}
190+
kube := &test.MockClient{
191+
MockUpdate: test.NewMockUpdateFn(nil),
192+
}
193+
err := c.ResolveServicePlan(tc.args.ctx, kube, tc.args.cr)
194+
195+
switch {
196+
case tc.want.err == nil && err != nil:
197+
t.Fatalf("ResolveServicePlan(...): unexpected error: %v", err)
198+
199+
case tc.want.err != nil && err == nil:
200+
t.Fatalf("ResolveServicePlan(...): expected error %v but got nil", tc.want.err)
201+
202+
case tc.want.err != nil && err != nil:
203+
if diff := cmp.Diff(tc.want.err.Error(), err.Error()); diff != "" {
204+
t.Errorf("ResolveServicePlan(...): -want, +got:\n%s", diff)
205+
}
206+
}
207+
})
208+
}
209+
}

‎internal/controller/serviceinstance/controller.go‎

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"crypto/sha256"
77
"time"
88

9-
"github.com/cloudfoundry/go-cfclient/v3/client"
109
xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"
1110
"github.com/crossplane/crossplane-runtime/pkg/connection"
1211
"github.com/crossplane/crossplane-runtime/pkg/controller"
@@ -391,28 +390,16 @@ func (s servicePlanInitializer) Initialize(ctx context.Context, mg resource.Mana
391390
}
392391

393392
if cr.Spec.ForProvider.ServicePlan != nil {
394-
// When ServicePlan is set we populate the service
395-
// plan ID based on the external resource GUID.
393+
// When ServicePlan is set we either populate/update the service plan ID with the external resource GUID
394+
// based on the specified offering and plan or we use the provided ID directly.
396395
cf, err := clients.ClientFnBuilder(ctx, s.kube)(mg)
397396
if err != nil {
398397
return errors.Wrapf(err, errNewClient)
399398
}
400399

401-
opt := client.NewServicePlanListOptions()
402-
if cr.Spec.ForProvider.ServicePlan.Offering != nil {
403-
opt.ServiceOfferingNames.EqualTo(*cr.Spec.ForProvider.ServicePlan.Offering)
404-
}
405-
if cr.Spec.ForProvider.ServicePlan.Plan != nil {
406-
opt.Names.EqualTo(*cr.Spec.ForProvider.ServicePlan.Plan)
407-
}
408-
sp, err := cf.ServicePlans.Single(ctx, opt)
409-
if err != nil {
410-
return errors.Wrapf(err, "Cannot initialize service plan using serviceName/servicePlanName: %s:%s`", *cr.Spec.ForProvider.ServicePlan.Offering, *cr.Spec.ForProvider.ServicePlan.Plan)
411-
}
412-
413-
cr.Spec.ForProvider.ServicePlan.ID = &sp.GUID
400+
client := serviceinstance.NewClient(cf)
401+
return client.ResolveServicePlan(ctx, s.kube, cr)
414402

415-
return s.kube.Update(ctx, cr)
416403
}
417404

418405
// Service plan is not set

0 commit comments

Comments
 (0)