Skip to content

Commit dfb2e05

Browse files
kcp-ci-botkcp-ci-bot
authored
Merge pull request #3543 from xmudrii/abadmission-fix-2
Fix APIBinding admission mishandling v1alpha1 API version
2 parents a54412e + 5d32c40 commit dfb2e05

File tree

7 files changed

+599
-140
lines changed

7 files changed

+599
-140
lines changed

‎pkg/admission/apibinding/apibinding_admission.go‎

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,9 @@ import (
2222
"errors"
2323
"fmt"
2424
"io"
25-
"reflect"
2625

2726
apierrors "k8s.io/apimachinery/pkg/api/errors"
2827
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
29-
"k8s.io/apimachinery/pkg/runtime"
3028
"k8s.io/apimachinery/pkg/util/validation/field"
3129
"k8s.io/apiserver/pkg/admission"
3230
"k8s.io/apiserver/pkg/authentication/user"
@@ -93,86 +91,94 @@ func (o *apiBindingAdmission) Admit(ctx context.Context, a admission.Attributes,
9391
return apierrors.NewInternalError(err)
9492
}
9593

96-
if a.GetResource().GroupResource() == apisv1alpha1.Resource("apibindings") {
94+
// Skip if the object is not an APIBinding
95+
// (v1alpha2 doesn't mean the resource version, we're only taking the resource from that package)
96+
if a.GetResource().GroupResource() != apisv1alpha2.Resource("apibindings") {
97+
return nil
98+
}
99+
100+
// If we're working with v1alpha1 object, check the overhanging permission claims
101+
if a.GetResource().GroupVersion() == apisv1alpha1.SchemeGroupVersion {
97102
ab := &apisv1alpha1.APIBinding{}
98103
if err := validateOverhangingPermissionClaims(ctx, a, ab); err != nil {
99104
return admission.NewForbidden(a, err)
100105
}
101106
}
102107

103-
if a.GetResource().GroupResource() != apisv1alpha2.Resource("apibindings") {
104-
return nil
105-
}
106-
107108
u, ok := a.GetObject().(*unstructured.Unstructured)
108109
if !ok {
109110
return fmt.Errorf("unexpected type %T", a.GetObject())
110111
}
111112

112-
apiBinding := &apisv1alpha2.APIBinding{}
113-
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, apiBinding); err != nil {
113+
ab, err := getAPIBinding(u, a.GetResource().Version)
114+
if err != nil {
114115
return fmt.Errorf("failed to convert unstructured to APIBinding: %w", err)
115116
}
116117

117-
if apiBinding.Spec.Reference.Export == nil {
118+
if !ab.BindingReference().HasExport() {
118119
// should not happen due to validation.
119120
return nil
120121
}
121122

122-
var oldAPIBinding *apisv1alpha2.APIBinding
123+
exportPath := ab.BindingReference().ExportPath()
124+
exportName := ab.BindingReference().ExportName()
125+
126+
var oldAPIBinding apiBinding
123127
if a.GetOperation() == admission.Update {
124128
u, ok := a.GetOldObject().(*unstructured.Unstructured)
125129
if !ok {
126130
return fmt.Errorf("unexpected type %T", a.GetObject())
127131
}
128132

129-
oldAPIBinding = &apisv1alpha2.APIBinding{}
130-
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, oldAPIBinding); err != nil {
133+
oldAPIBinding, err = getAPIBinding(u, a.GetResource().Version)
134+
if err != nil {
131135
return fmt.Errorf("failed to convert unstructured to APIBinding: %w", err)
132136
}
133137
}
134138

135139
switch {
136140
case a.GetOperation() == admission.Create,
137-
a.GetOperation() == admission.Update && !reflect.DeepEqual(apiBinding.Spec.Reference, oldAPIBinding.Spec.Reference),
138-
a.GetOperation() == admission.Update && apiBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey] != oldAPIBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey]:
141+
a.GetOperation() == admission.Update && !ab.BindingReference().DeepEqual(oldAPIBinding.BindingReference()),
142+
a.GetOperation() == admission.Update && ab.Labels()[apisv1alpha1.InternalAPIBindingExportLabelKey] != oldAPIBinding.Labels()[apisv1alpha1.InternalAPIBindingExportLabelKey]:
139143

140144
// unified forbidden error that does not leak workspace existence
141145
action := "create"
142146
if a.GetOperation() == admission.Update {
143147
action = "update"
144148
}
145149
forbidden := admission.NewForbidden(a, fmt.Errorf("unable to %s APIBinding: no permission to bind to export %s", action,
146-
logicalcluster.NewPath(apiBinding.Spec.Reference.Export.Path).Join(apiBinding.Spec.Reference.Export.Name).String()))
150+
logicalcluster.NewPath(exportPath).Join(exportName).String()))
147151

148152
// get cluster name of export
149153
var exportClusterName logicalcluster.Name
150-
if apiBinding.Spec.Reference.Export.Path == "" {
154+
if exportPath == "" {
151155
exportClusterName = clusterName
152-
} else if apiBinding.Spec.Reference.Export.Path == core.RootCluster.String() {
156+
} else if exportPath == core.RootCluster.String() {
153157
// special case to allow bootstrapping
154158
exportClusterName = core.RootCluster
155159
} else {
156-
path := logicalcluster.NewPath(apiBinding.Spec.Reference.Export.Path)
157-
export, err := o.getAPIExport(path, apiBinding.Spec.Reference.Export.Name)
160+
path := logicalcluster.NewPath(exportPath)
161+
export, err := o.getAPIExport(path, exportName)
158162
if err != nil {
159163
return forbidden
160164
}
161165
exportClusterName = logicalcluster.From(export)
162166
}
163167

164168
// set labels
165-
if apiBinding.Labels == nil {
166-
apiBinding.Labels = make(map[string]string)
169+
lbls := ab.Labels()
170+
if lbls == nil {
171+
lbls = map[string]string{}
167172
}
168-
apiBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey] = permissionclaims.ToAPIBindingExportLabelValue(
173+
lbls[apisv1alpha1.InternalAPIBindingExportLabelKey] = permissionclaims.ToAPIBindingExportLabelValue(
169174
exportClusterName,
170-
apiBinding.Spec.Reference.Export.Name,
175+
exportName,
171176
)
177+
ab.SetLabels(lbls)
172178
}
173179

174180
// write back
175-
raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(apiBinding)
181+
raw, err := ab.ToUnstructured()
176182
if err != nil {
177183
return err
178184
}
@@ -189,6 +195,8 @@ func (o *apiBindingAdmission) Validate(ctx context.Context, a admission.Attribut
189195
return apierrors.NewInternalError(err)
190196
}
191197

198+
// Skip if the object is not an APIBinding
199+
// (v1alpha2 doesn't mean the resource version, we're only taking the resource from that package)
192200
if a.GetResource().GroupResource() != apisv1alpha2.Resource("apibindings") {
193201
return nil
194202
}
@@ -198,72 +206,75 @@ func (o *apiBindingAdmission) Validate(ctx context.Context, a admission.Attribut
198206
return fmt.Errorf("unexpected type %T", a.GetObject())
199207
}
200208

201-
apiBinding := &apisv1alpha2.APIBinding{}
202-
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, apiBinding); err != nil {
209+
ab, err := getAPIBinding(u, a.GetResource().Version)
210+
if err != nil {
203211
return fmt.Errorf("failed to convert unstructured to APIBinding: %w", err)
204212
}
205213

206214
// Object validation
207215
var errs field.ErrorList
208-
var oldAPIBinding *apisv1alpha2.APIBinding
216+
var oldAPIBinding apiBinding
209217
switch a.GetOperation() {
210218
case admission.Create:
211-
errs = ValidateAPIBinding(apiBinding)
219+
errs = ab.Validate()
212220
case admission.Update:
213221
u, ok = a.GetOldObject().(*unstructured.Unstructured)
214222
if !ok {
215223
return fmt.Errorf("unexpected type %T", a.GetOldObject())
216224
}
217-
oldAPIBinding = &apisv1alpha2.APIBinding{}
218-
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, oldAPIBinding); err != nil {
225+
oldAPIBinding, err = getAPIBinding(u, a.GetResource().Version)
226+
if err != nil {
219227
return fmt.Errorf("failed to convert unstructured to APIBinding: %w", err)
220228
}
221229

222-
errs = ValidateAPIBindingUpdate(oldAPIBinding, apiBinding)
230+
errs = ab.ValidateUpdate(oldAPIBinding)
223231
}
224232
if len(errs) > 0 {
225233
return admission.NewForbidden(a, fmt.Errorf("%v", errs))
226234
}
227235

236+
exportPath := ab.BindingReference().ExportPath()
237+
exportName := ab.BindingReference().ExportName()
238+
228239
switch {
229240
case a.GetOperation() == admission.Create,
230-
a.GetOperation() == admission.Update && !reflect.DeepEqual(apiBinding.Spec.Reference, oldAPIBinding.Spec.Reference),
231-
a.GetOperation() == admission.Update && apiBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey] != oldAPIBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey]:
241+
a.GetOperation() == admission.Update && !ab.BindingReference().DeepEqual(oldAPIBinding.BindingReference()),
242+
a.GetOperation() == admission.Update && ab.Labels()[apisv1alpha1.InternalAPIBindingExportLabelKey] != oldAPIBinding.Labels()[apisv1alpha1.InternalAPIBindingExportLabelKey]:
232243

233244
// unified forbidden error that does not leak workspace existence
234245
action := "create"
235246
if a.GetOperation() == admission.Update {
236247
action = "update"
237248
}
238249
forbidden := admission.NewForbidden(a, fmt.Errorf("unable to %s APIBinding: no permission to bind to export %s", action,
239-
logicalcluster.NewPath(apiBinding.Spec.Reference.Export.Path).Join(apiBinding.Spec.Reference.Export.Name).String()))
250+
logicalcluster.NewPath(exportPath).Join(exportName).String()))
240251

241252
// get cluster name of export
242253
var exportClusterName logicalcluster.Name
243-
if apiBinding.Spec.Reference.Export.Path == "" {
254+
if exportPath == "" {
244255
exportClusterName = clusterName
245-
} else if apiBinding.Spec.Reference.Export.Path == core.RootCluster.String() {
256+
} else if exportPath == core.RootCluster.String() {
246257
// special case to allow bootstrapping
247258
exportClusterName = core.RootCluster
248259
} else {
249-
path := logicalcluster.NewPath(apiBinding.Spec.Reference.Export.Path)
250-
export, err := o.getAPIExport(path, apiBinding.Spec.Reference.Export.Name)
260+
path := logicalcluster.NewPath(exportPath)
261+
export, err := o.getAPIExport(path, exportName)
251262
if err != nil {
252263
return forbidden
253264
}
254265
exportClusterName = logicalcluster.From(export)
255266
}
256267

257268
// Access check
258-
if err := o.checkAPIExportAccess(ctx, a.GetUserInfo(), exportClusterName, apiBinding.Spec.Reference.Export.Name); err != nil {
269+
if err := o.checkAPIExportAccess(ctx, a.GetUserInfo(), exportClusterName, exportName); err != nil {
259270
return forbidden
260271
}
261272

262273
// Verify the labels
263-
value := apiBinding.Labels[apisv1alpha1.InternalAPIBindingExportLabelKey]
274+
value := ab.Labels()[apisv1alpha1.InternalAPIBindingExportLabelKey]
264275
if expected := permissionclaims.ToAPIBindingExportLabelValue(
265276
exportClusterName,
266-
apiBinding.Spec.Reference.Export.Name,
277+
exportName,
267278
); value != expected {
268279
return admission.NewForbidden(a, field.Invalid(field.NewPath("metadata").Child("labels").Key(apisv1alpha1.InternalAPIBindingExportLabelKey), value, fmt.Sprintf("must be set to %q", expected)))
269280
}

0 commit comments

Comments
 (0)