Skip to content

Commit fa981c5

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

24 files changed

Lines changed: 196 additions & 69 deletions

‎backend/controllers/clusterbinding/clusterbinding_reconcile.go‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@ package clusterbinding
1919
import (
2020
"context"
2121
"fmt"
22+
"maps"
2223
"reflect"
23-
"sort"
24+
"slices"
2425
"time"
2526

2627
corev1 "k8s.io/api/core/v1"
@@ -149,37 +150,36 @@ func (r *reconciler) ensureRBACClusterRole(ctx context.Context, client client.Cl
149150
},
150151
},
151152
},
152-
}
153+
Rules: []rbacv1.PolicyRule{
154+
// Always need to be able to get/list/watch the BoundSchemas
155+
// to be able to figure out what to bind.
156+
{
157+
APIGroups: []string{kubebindv1alpha2.GroupName},
158+
Resources: []string{"boundschemas"},
159+
Verbs: []string{"get", "list", "watch"},
160+
},
161+
}}
153162
for _, export := range exports {
154163
// Collect unique GroupResources and sort for stable rule ordering.
155164
grSet := map[string]kubebindv1alpha2.GroupResource{}
156165
for _, res := range export.Spec.Resources {
157166
key := res.ResourceGroupName()
158167
grSet[key] = kubebindv1alpha2.GroupResource{Group: res.Group, Resource: res.Resource}
159168
}
160-
keys := make([]string, 0, len(grSet))
161-
for k := range grSet {
162-
keys = append(keys, k)
163-
}
164-
sort.Strings(keys)
169+
keys := slices.Collect(maps.Keys(grSet))
170+
slices.Sort(keys)
165171
for _, k := range keys {
166-
gr := grSet[k]
167-
name := gr.Resource + "." + gr.Group
168-
schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, name)
172+
// k is already normalized (e.g., "pods.core" for empty group).
173+
schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, k)
169174
if err != nil {
170-
return fmt.Errorf("failed to get BoundSchema %q: %w", name, err)
175+
return fmt.Errorf("failed to get BoundSchema %q: %w", k, err)
171176
}
172177
expected.Rules = append(expected.Rules,
173178
rbacv1.PolicyRule{
174179
APIGroups: []string{schema.Spec.Group},
175180
Resources: []string{schema.Spec.Names.Plural},
176181
Verbs: []string{"get", "list", "watch", "create", "update", "patch", "delete"},
177182
},
178-
rbacv1.PolicyRule{
179-
APIGroups: []string{kubebindv1alpha2.GroupName},
180-
Resources: []string{"boundschemas"},
181-
Verbs: []string{"get", "list", "watch"},
182-
},
183183
)
184184
}
185185
}
@@ -205,7 +205,6 @@ func (r *reconciler) ensureRBACClusterRoleBinding(ctx context.Context, client cl
205205
if err != nil && !errors.IsNotFound(err) {
206206
return fmt.Errorf("failed to get ClusterRoleBinding %s: %w", name, err)
207207
}
208-
209208
if r.scope != kubebindv1alpha2.ClusterScope {
210209
if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) {
211210
return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err)

‎backend/controllers/serviceexport/serviceexport_controller.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,11 +130,13 @@ func (r *APIServiceExportReconciler) Reconcile(ctx context.Context, req mcreconc
130130
return ctrl.Result{}, err
131131
}
132132

133-
// Update annotatations changed (hash)
133+
// Update annotatations changed (hash), we need to propagate it and requeue for status changes.
134+
// This is why we compare annotations only as we don't expect any changes to spec.
135+
// Status changes are handled below.
134136
if !equality.Semantic.DeepEqual(original.Annotations, apiServiceExport.Annotations) {
135137
err := client.Update(ctx, apiServiceExport)
136138
if err != nil {
137-
return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err)
139+
return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport: %w", err)
138140
}
139141
logger.Info("APIServiceExport hash updated", "namespace", apiServiceExport.Namespace, "name", apiServiceExport.Name)
140142
return ctrl.Result{Requeue: true}, nil

‎backend/controllers/serviceexport/serviceexport_reconcile.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,15 +51,16 @@ func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export
5151
schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources))
5252

5353
for _, res := range export.Spec.Resources {
54-
schema, err := r.getBoundSchema(ctx, cache, export.Namespace, res.ResourceGroupName())
54+
name := res.ResourceGroupName()
55+
schema, err := r.getBoundSchema(ctx, cache, export.Namespace, name)
5556
if err != nil {
5657
if errors.IsNotFound(err) {
5758
conditions.MarkFalse(
5859
export,
5960
kubebindv1alpha2.APIServiceExportConditionProviderInSync,
6061
"BoundSchemaMissing",
6162
conditionsapi.ConditionSeverityError,
62-
"BoundSchemas are not available: %v", schema.Name)
63+
"BoundSchema %q is not available: %v", name, err)
6364
return nil
6465
}
6566
return err
@@ -68,7 +69,10 @@ func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export
6869
schemas = append(schemas, schema)
6970
}
7071

71-
hash := helpers.BoundSchemasSpecHash(schemas)
72+
hash, err := helpers.BoundSchemasSpecHash(schemas)
73+
if err != nil {
74+
return err
75+
}
7276

7377
if export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] != hash {
7478
// both exist, update APIServiceExport

‎backend/controllers/serviceexportrequest/serviceexportrequest_controller.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,15 @@ func (r *APIServiceExportRequestReconciler) Reconcile(ctx context.Context, req m
215215
// Run the reconciliation logic
216216
if err := r.reconciler.reconcile(ctx, client, cache, apiServiceExportRequest); err != nil {
217217
logger.Error(err, "Failed to reconcile APIServiceExportRequest")
218+
if !reflect.DeepEqual(original.Status.Phase, apiServiceExportRequest.Status.Phase) {
219+
err := client.Status().Update(ctx, apiServiceExportRequest)
220+
if err != nil {
221+
logger.Error(err, "Failed to update APIServiceExportRequest status")
222+
return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExportRequest status: %w", err)
223+
}
224+
logger.Info("APIServiceExportRequest status updated", "namespace", apiServiceExportRequest.Namespace, "name", apiServiceExportRequest.Name)
225+
}
226+
218227
return ctrl.Result{}, err
219228
}
220229

‎backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go‎

Lines changed: 38 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,12 @@ type reconciler struct {
5454

5555
func (r *reconciler) reconcile(ctx context.Context, cl client.Client, cache cache.Cache, req *kubebindv1alpha2.APIServiceExportRequest) error {
5656
if err := r.ensureBoundSchemas(ctx, cl, cache, req); err != nil {
57+
conditions.SetSummary(req)
5758
return err
5859
}
5960

6061
if err := r.ensureExports(ctx, cl, cache, req); err != nil {
62+
conditions.SetSummary(req)
6163
return err
6264
}
6365

@@ -75,7 +77,7 @@ func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, c
7577
for _, res := range req.Spec.Resources {
7678
parts := strings.SplitN(r.schemaSource, ".", 3)
7779
if len(parts) != 3 { // We check this in validation, but just in case.
78-
return fmt.Errorf("invalid schema source: %q", r.schemaSource)
80+
return fmt.Errorf("malformed schema source: %q", r.schemaSource)
7981
}
8082

8183
gvk := schema.GroupVersionKind{
@@ -107,19 +109,41 @@ func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, c
107109

108110
for _, item := range list.Items {
109111
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())
112+
group, ok, err := unstructured.NestedString(obj, "spec", "group")
113+
if !ok || err != nil || group == "" {
114+
klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing group", "ns", item.GetNamespace(), "name", item.GetName())
113115
continue
114116
}
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())
117+
plural, ok, err := unstructured.NestedString(obj, "spec", "names", "plural")
118+
if !ok || err != nil || plural == "" {
119+
klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing names.plural", "ns", item.GetNamespace(), "name", item.GetName())
120120
continue
121121
}
122-
if g == res.Group && p == res.Resource {
122+
123+
scope, ok, err := unstructured.NestedString(obj, "spec", "scope")
124+
if !ok || err != nil || scope == "" {
125+
klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing scope", "ns", item.GetNamespace(), "name", item.GetName())
126+
continue
127+
}
128+
129+
if group == res.Group && plural == res.Resource {
130+
// Important: This checks if the resource are correctly scoped. If consumer is namespaced, we can't allow this.
131+
// We terminate early to prevent triggering other controllers.
132+
if r.informerScope.String() != scope && r.informerScope != kubebindv1alpha2.ClusterScope {
133+
conditions.MarkFalse(
134+
req,
135+
kubebindv1alpha2.APIServiceExportRequestConditionExportsReady,
136+
"APIServiceExportRequestInvalid",
137+
conditionsapi.ConditionSeverityError,
138+
"APIServiceExportRequest %s is invalid: resource %s/%s has scope %q which is incompatible with backend informer scope %q",
139+
req.Name, group, plural, scope, r.informerScope,
140+
)
141+
req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed
142+
req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady)
143+
// We can't proceed with this request.
144+
return fmt.Errorf("resource %s/%s has scope %q which is incompatible with backend informer scope %q", group, plural, scope, r.informerScope)
145+
}
146+
123147
boundSchema, err := helpers.UnstructuredToBoundSchema(item)
124148
if err != nil {
125149
return err
@@ -189,7 +213,10 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
189213
return nil
190214
}
191215

192-
hash := helpers.BoundSchemasSpecHash(schemas)
216+
hash, err := helpers.BoundSchemasSpecHash(schemas)
217+
if err != nil {
218+
return err
219+
}
193220
export := &kubebindv1alpha2.APIServiceExport{
194221
ObjectMeta: metav1.ObjectMeta{
195222
Name: req.Name,

‎backend/http/handler.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -388,6 +388,12 @@ func (h *handler) handleResources(w http.ResponseWriter, r *http.Request) {
388388
scope = "-"
389389
}
390390

391+
// TODO(mjudeikis): This logic is very brittle, needs rework.
392+
// This will be improved in the permissionClaims PR.
393+
if !strings.EqualFold(h.scope.String(), scope.(string)) && h.scope != kubebindv1alpha2.ClusterScope {
394+
continue
395+
}
396+
391397
group := item.UnstructuredContent()["spec"].(map[string]interface{})["group"]
392398
if group == nil {
393399
group = "-"

‎backend/kubernetes/manager.go‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -192,24 +192,3 @@ func (m *Manager) ListDynamicResources(ctx context.Context, cluster string, gvk
192192

193193
return list, nil
194194
}
195-
196-
/* func (m *Manager) CreateBoundSchema(ctx context.Context, cluster string, name string, u *unstructured.Unstructured) error {
197-
cl, err := m.manager.GetCluster(ctx, cluster)
198-
if err != nil {
199-
return err
200-
}
201-
c := cl.GetClient()
202-
203-
boundSchema := &kubebindv1alpha2.BoundSchema{}
204-
err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), boundSchema)
205-
if err != nil {
206-
return err
207-
}
208-
209-
boundSchema.ResourceVersion = ""
210-
boundSchema.Name = name
211-
boundSchema.Spec.InformerScope = m.scope
212-
213-
return c.Create(ctx, boundSchema)
214-
}
215-
*/

‎backend/options/options.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ type ExtraOptions struct {
5656
// Defines the source of the schema for the bind screen.
5757
// Options are:
5858
// CustomResourceDefinition.v1.apiextensions.k8s.io
59-
// APIResourceSchema.v1alpha2.apis.kcp.io
59+
// APIResourceSchema.v1alpha1.apis.kcp.io
6060
SchemaSource string
6161

6262
TestingAutoSelect string

‎cli/pkg/kubectl/bind/plugin/bind.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ func (b *BindOptions) Run(ctx context.Context, urlCh chan<- string) error {
159159
}
160160

161161
if provider.APIVersion != kubebindv1alpha2.GroupVersion {
162-
return fmt.Errorf("unsupported binding provider version: %q != %q", provider.APIVersion, kubebindv1alpha2.GroupVersion)
162+
return fmt.Errorf("unsupported binding provider version %q, expected %q", provider.APIVersion, kubebindv1alpha2.GroupVersion)
163163
}
164164

165165
ns, err := kubeClient.CoreV1().Namespaces().Get(ctx, "kube-bind", metav1.GetOptions{})

‎deploy/crd/kube-bind.io_boundschemas.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ spec:
1515
listKind: BoundSchemaList
1616
plural: boundschemas
1717
shortNames:
18-
- bas
18+
- bs
1919
singular: boundschema
2020
scope: Namespaced
2121
versions:

0 commit comments

Comments
 (0)