Skip to content

Commit 3b83bce

Browse files
mjudeikismjudeikis
committed
Address review comments.
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: @SAP mangirdas.judeikis@sap.com
1 parent 1b19d69 commit 3b83bce

15 files changed

Lines changed: 110 additions & 64 deletions

File tree

‎backend/controllers/clusterbinding/clusterbinding_reconcile.go‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
"context"
2121
"fmt"
2222
"reflect"
23+
"sort"
2324
"time"
2425

2526
corev1 "k8s.io/api/core/v1"
@@ -150,23 +151,34 @@ func (r *reconciler) ensureRBACClusterRole(ctx context.Context, client client.Cl
150151
},
151152
}
152153
for _, export := range exports {
154+
// Collect unique GroupResources and sort for stable rule ordering.
155+
grSet := map[string]kubebindv1alpha2.GroupResource{}
153156
for _, res := range export.Spec.Resources {
154-
name := res.Resource + "." + res.Group
157+
key := res.ResourceGroupName()
158+
grSet[key] = kubebindv1alpha2.GroupResource{Group: res.Group, Resource: res.Resource}
159+
}
160+
keys := make([]string, 0, len(grSet))
161+
for k := range grSet {
162+
keys = append(keys, k)
163+
}
164+
sort.Strings(keys)
165+
for _, k := range keys {
166+
gr := grSet[k]
167+
name := gr.Resource + "." + gr.Group
155168
schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, name)
156169
if err != nil {
157-
return fmt.Errorf("failed to get BoundSchema %w", err)
170+
return fmt.Errorf("failed to get BoundSchema %q: %w", name, err)
158171
}
159-
160172
expected.Rules = append(expected.Rules,
161173
rbacv1.PolicyRule{
162174
APIGroups: []string{schema.Spec.Group},
163175
Resources: []string{schema.Spec.Names.Plural},
164-
Verbs: []string{"get", "list", "watch", "update", "patch", "delete", "create"},
176+
Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"},
165177
},
166178
rbacv1.PolicyRule{
167179
APIGroups: []string{kubebindv1alpha2.GroupName},
168180
Resources: []string{"boundschemas"},
169-
Verbs: []string{"get", "list", "watch", "update", "patch"},
181+
Verbs: []string{"get", "list", "watch"},
170182
},
171183
)
172184
}

‎backend/controllers/serviceexport/serviceexport_controller.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,9 +67,9 @@ func NewAPIServiceExportReconciler(
6767
manager: mgr,
6868
opts: opts,
6969
reconciler: reconciler{
70-
getBoundSchema: func(ctx context.Context, cache cache.Cache, name string) (*kubebindv1alpha2.BoundSchema, error) {
70+
getBoundSchema: func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error) {
7171
var schema kubebindv1alpha2.BoundSchema
72-
key := types.NamespacedName{Name: name}
72+
key := types.NamespacedName{Namespace: namespace, Name: name}
7373
if err := cache.Get(ctx, key, &schema); err != nil {
7474
return nil, err
7575
}
@@ -92,6 +92,8 @@ func NewAPIServiceExportReconciler(
9292
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiserviceexports,verbs=get;list;watch;create;update;patch;delete
9393
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiserviceexports/status,verbs=get;update;patch
9494
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiserviceexports/finalizers,verbs=update
95+
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch
96+
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas/status,verbs=get;update;patch
9597

9698
// Reconcile is part of the main kubernetes reconciliation loop which aims to
9799
// move the current state of the cluster closer to the desired state.
@@ -128,8 +130,16 @@ func (r *APIServiceExportReconciler) Reconcile(ctx context.Context, req mcreconc
128130
return ctrl.Result{}, err
129131
}
130132

131-
// Update status if it has changed
132-
if !equality.Semantic.DeepEqual(original, apiServiceExport) {
133+
// Update annotatations changed (hash)
134+
if !equality.Semantic.DeepEqual(original.Annotations, apiServiceExport.Annotations) {
135+
err := client.Update(ctx, apiServiceExport)
136+
if err != nil {
137+
return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err)
138+
}
139+
logger.Info("APIServiceExport hash updated", "namespace", apiServiceExport.Namespace, "name", apiServiceExport.Name)
140+
return ctrl.Result{Requeue: true}, nil
141+
}
142+
if !equality.Semantic.DeepEqual(original.Status, apiServiceExport.Status) {
133143
err := client.Status().Update(ctx, apiServiceExport)
134144
if err != nil {
135145
return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err)

‎backend/controllers/serviceexport/serviceexport_reconcile.go‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,41 +27,42 @@ import (
2727

2828
kubebindv1alpha2 "github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2"
2929
"github.com/kube-bind/kube-bind/sdk/apis/kubebind/v1alpha2/helpers"
30+
conditionsapi "github.com/kube-bind/kube-bind/sdk/apis/third_party/conditions/apis/conditions/v1alpha1"
3031
"github.com/kube-bind/kube-bind/sdk/apis/third_party/conditions/util/conditions"
3132
)
3233

3334
type reconciler struct {
34-
getBoundSchema func(ctx context.Context, cache cache.Cache, name string) (*kubebindv1alpha2.BoundSchema, error)
35+
getBoundSchema func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error)
3536
deleteServiceExport func(ctx context.Context, client client.Client, namespace, name string) error
3637
}
3738

3839
func (r *reconciler) reconcile(ctx context.Context, cache cache.Cache, export *kubebindv1alpha2.APIServiceExport) error {
3940
var errs []error
4041

41-
if specChanged, err := r.ensureSchema(ctx, cache, export); err != nil {
42+
if err := r.ensureSchema(ctx, cache, export); err != nil {
4243
errs = append(errs, err)
43-
} else if specChanged {
44-
// TODO: This should be separate controller for apiresourceschemas.
45-
// This is wrong place now.
46-
// r.requeue(export)
47-
return nil
4844
}
4945

5046
return utilerrors.NewAggregate(errs)
5147
}
5248

53-
func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export *kubebindv1alpha2.APIServiceExport) (specChanged bool, err error) {
49+
func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export *kubebindv1alpha2.APIServiceExport) error {
5450
logger := klog.FromContext(ctx)
5551
schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources))
5652

5753
for _, res := range export.Spec.Resources {
58-
name := res.Resource + "." + res.Group
59-
schema, err := r.getBoundSchema(ctx, cache, name)
54+
schema, err := r.getBoundSchema(ctx, cache, export.Namespace, res.ResourceGroupName())
6055
if err != nil {
6156
if errors.IsNotFound(err) {
62-
continue
57+
conditions.MarkFalse(
58+
export,
59+
kubebindv1alpha2.APIServiceExportConditionProviderInSync,
60+
"BoundSchemaMissing",
61+
conditionsapi.ConditionSeverityError,
62+
"BoundSchemas are not available: %v", schema.Name)
63+
return nil
6364
}
64-
return false, err
65+
return err
6566
}
6667

6768
schemas = append(schemas, schema)
@@ -76,10 +77,10 @@ func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export
7677
export.Annotations = map[string]string{}
7778
}
7879
export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] = hash
79-
return true, nil
80+
return nil
8081
}
8182

8283
conditions.MarkTrue(export, kubebindv1alpha2.APIServiceExportConditionProviderInSync)
8384

84-
return false, nil
85+
return nil
8586
}

‎backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go‎

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,10 @@ func (r *reconciler) reconcile(ctx context.Context, cl client.Client, cache cach
6161
return err
6262
}
6363

64+
// TODO(mjudeikis): we could potentially add finallizer to APIServiceExport above or "adopt" boundschemas
65+
// with owner references once export is created.
66+
// https://github.com/kube-bind/kube-bind/issues/297
67+
6468
conditions.SetSummary(req)
6569

6670
return nil
@@ -69,10 +73,6 @@ func (r *reconciler) reconcile(ctx context.Context, cl client.Client, cache cach
6973
func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, cache cache.Cache, req *kubebindv1alpha2.APIServiceExportRequest) error {
7074
// Ensure all bound schemas exist
7175
for _, res := range req.Spec.Resources {
72-
if len(res.Versions) == 0 {
73-
continue
74-
}
75-
7676
parts := strings.SplitN(r.schemaSource, ".", 3)
7777
if len(parts) != 3 { // We check this in validation, but just in case.
7878
return fmt.Errorf("invalid schema source: %q", r.schemaSource)
@@ -106,23 +106,25 @@ func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, c
106106
}
107107

108108
for _, item := range list.Items {
109-
spec := item.Object["spec"]
110-
if spec == nil {
111-
return fmt.Errorf("invalid schema %s/%s: no spec found", item.GetNamespace(), item.GetName())
109+
obj := item.UnstructuredContent()
110+
spec, ok := obj["spec"].(map[string]interface{})
111+
if !ok {
112+
klog.FromContext(ctx).Error(nil, "Skipping invalid schema: missing spec", "ns", item.GetNamespace(), "name", item.GetName())
113+
continue
112114
}
113-
group := spec.(map[string]interface{})["group"]
114-
names := spec.(map[string]interface{})["names"]
115-
if group == nil || names == nil {
116-
return fmt.Errorf("invalid schema %s/%s: no group or names found", item.GetNamespace(), item.GetName())
115+
g, _ := spec["group"].(string)
116+
names, _ := spec["names"].(map[string]interface{})
117+
p, _ := names["plural"].(string)
118+
if g == "" || p == "" {
119+
klog.FromContext(ctx).Error(nil, "Skipping invalid schema: missing group or names.plural", "ns", item.GetNamespace(), "name", item.GetName())
120+
continue
117121
}
118-
plural := names.(map[string]interface{})["plural"]
119-
120-
if group == res.Group && plural == res.Resource {
122+
if g == res.Group && p == res.Resource {
121123
boundSchema, err := helpers.UnstructuredToBoundSchema(item)
122124
if err != nil {
123125
return err
124126
}
125-
boundSchema.Name = res.Resource + "." + res.Group
127+
boundSchema.Name = res.ResourceGroupName()
126128
boundSchema.Namespace = req.Namespace
127129
boundSchema.Spec.InformerScope = r.informerScope
128130
boundSchema.ResourceVersion = ""
@@ -154,7 +156,7 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
154156
var scope apiextensionsv1.ResourceScope
155157
if req.Status.Phase == kubebindv1alpha2.APIServiceExportRequestPhasePending {
156158
for _, res := range req.Spec.Resources {
157-
name := res.Resource + "." + res.Group
159+
name := res.ResourceGroupName()
158160
boundSchema, err := r.getBoundSchema(ctx, cache, req.Namespace, name)
159161
if err != nil {
160162
if apierrors.IsNotFound(err) {
@@ -177,8 +179,14 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
177179
schemas = append(schemas, boundSchema)
178180
}
179181

180-
if _, err := r.getServiceExport(ctx, cache, req.Namespace, req.Name); err != nil && !apierrors.IsNotFound(err) {
181-
return err
182+
if _, err := r.getServiceExport(ctx, cache, req.Namespace, req.Name); err != nil {
183+
if !apierrors.IsNotFound(err) {
184+
return err
185+
}
186+
} else {
187+
// already exists; nothing to do
188+
conditions.MarkTrue(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady)
189+
return nil
182190
}
183191

184192
hash := helpers.BoundSchemasSpecHash(schemas)
@@ -210,6 +218,9 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
210218

211219
logger.V(1).Info("Creating APIServiceExport", "name", export.Name, "namespace", export.Namespace)
212220
if err := r.createServiceExport(ctx, cl, export); err != nil {
221+
if apierrors.IsAlreadyExists(err) {
222+
return nil
223+
}
213224
return err
214225
}
215226

‎backend/options/options.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ var schemaSourceAliases = map[string]string{
122122
CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests
123123
"customresourcedefinitions": CustomResourceDefinitionSource.String(),
124124
"apiresourceschemas": KCPAPIResourceSchemaSource.String(),
125+
KCPAPIResourceSchemaSource.String(): KCPAPIResourceSchemaSource.String(),
125126
}
126127

127128
func (options *Options) AddFlags(fs *pflag.FlagSet) {

‎cli/pkg/kubectl/bind-apiservice/plugin/servicebindings.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func (b *BindAPIServiceOptions) createAPIServiceBindings(ctx context.Context, co
4646

4747
var bindings []*kubebindv1alpha2.APIServiceBinding
4848
for _, resource := range request.Spec.Resources {
49-
name := resource.Resource + "." + resource.Group
49+
name := resource.ResourceGroupName()
5050
existing, err := bindClient.KubeBindV1alpha2().APIServiceBindings().Get(ctx, name, metav1.GetOptions{})
5151
if err != nil && !apierrors.IsNotFound(err) {
5252
return nil, err
@@ -58,7 +58,7 @@ func (b *BindAPIServiceOptions) createAPIServiceBindings(ctx context.Context, co
5858
bindings = append(bindings, existing)
5959

6060
// checking CRD to match the binding
61-
crd, err := apiextensionsClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, resource.Resource+"."+resource.Group, metav1.GetOptions{})
61+
crd, err := apiextensionsClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, resource.ResourceGroupName(), metav1.GetOptions{})
6262
if err != nil && !apierrors.IsNotFound(err) {
6363
return nil, err
6464
} else if err == nil {
@@ -78,7 +78,7 @@ func (b *BindAPIServiceOptions) createAPIServiceBindings(ctx context.Context, co
7878
}
7979
created, err := bindClient.KubeBindV1alpha2().APIServiceBindings().Create(ctx, &kubebindv1alpha2.APIServiceBinding{
8080
ObjectMeta: metav1.ObjectMeta{
81-
Name: resource.Resource + "." + resource.Group,
81+
Name: resource.ResourceGroupName(),
8282
Namespace: "kube-bind",
8383
},
8484
Spec: kubebindv1alpha2.APIServiceBindingSpec{

‎pkg/indexers/serviceexport.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ func IndexServiceExportByBoundSchema(obj client.Object) []string {
4545

4646
names := make([]string, 0, len(export.Spec.Resources))
4747
for _, res := range export.Spec.Resources {
48-
name := res.Resource + "." + res.Group
49-
names = append(names, name)
48+
names = append(names, res.ResourceGroupName())
5049
}
5150

5251
return names

‎pkg/indexers/serviceexportrequest.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func IndexServiceExportRequestByGroupResource(obj client.Object) []string {
3636
}
3737
keys := []string{}
3838
for _, gr := range apiServiceExportRequest.Spec.Resources {
39-
keys = append(keys, gr.Resource+"."+gr.Group)
39+
keys = append(keys, gr.ResourceGroupName())
4040
}
4141
return keys
4242
}
@@ -50,7 +50,7 @@ func IndexServiceExportRequestByServiceExport(obj client.Object) []string {
5050
}
5151
keys := []string{}
5252
for _, gr := range apiServiceExportRequest.Spec.Resources {
53-
keys = append(keys, apiServiceExportRequest.Namespace+"/"+gr.Resource+"."+gr.Group)
53+
keys = append(keys, apiServiceExportRequest.Namespace+"/"+gr.ResourceGroupName())
5454
}
5555
return keys
5656
}

‎pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -267,12 +267,11 @@ func (r *reconciler) ensurePrettyName(ctx context.Context, binding *kubebindv1al
267267
func (r *reconciler) getSchemasFromExport(ctx context.Context, export *kubebindv1alpha2.APIServiceExport) ([]*kubebindv1alpha2.BoundSchema, error) {
268268
schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources))
269269

270-
for _, ref := range export.Spec.Resources {
271-
name := ref.Resource + "." + ref.Group
272-
schema, err := r.getBoundSchema(ctx, name)
270+
for _, res := range export.Spec.Resources {
271+
schema, err := r.getBoundSchema(ctx, res.ResourceGroupName())
273272
if err != nil {
274273
return nil, fmt.Errorf("failed to get Schema %s: %w",
275-
name, err)
274+
res.ResourceGroupName(), err)
276275
}
277276

278277
schemas = append(schemas, schema)

‎pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ func (r *reconciler) ensureControllers(ctx context.Context, name string, export
130130
processedSchemas := make(map[string]bool)
131131

132132
for _, res := range export.Spec.Resources {
133-
name := res.Resource + "." + res.Group
133+
name := res.ResourceGroupName()
134134
// Fetch the APIResourceSchema
135135
schema, err := r.getRemoteBoundSchema(ctx, name)
136136
if err != nil {
@@ -317,8 +317,7 @@ func (r *reconciler) ensureCRDConditionsCopiedToBoundSchema(ctx context.Context,
317317
var errs []error
318318
allValid := true // assume all BoundAPIResourceSchemas are valid
319319
for _, res := range export.Spec.Resources {
320-
name := res.Resource + "." + res.Group
321-
boundSchema, err := r.getRemoteBoundSchema(ctx, name)
320+
boundSchema, err := r.getRemoteBoundSchema(ctx, res.ResourceGroupName())
322321
if err != nil {
323322
if errors.IsNotFound(err) {
324323
continue

0 commit comments

Comments
 (0)