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

🌱 apibinding, types: remove ancestor check #2060

Merged
merged 3 commits into from
Sep 23, 2022
Merged

🌱 apibinding, types: remove ancestor check #2060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0439339
p/admission/apibinding: remove ancestor check
s-urbaniak Sep 23, 2022
39ec01d
p/a/apis/v/types_apibinding: remove ancestor constraint
s-urbaniak Sep 23, 2022
9fb617c
make codegen
s-urbaniak Sep 23, 2022
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
10 changes: 4 additions & 6 deletions config/crds/apis.kcp.dev_apibindings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,8 @@ spec:
type: string
path:
description: path is an absolute reference to a workspace,
e.g. root:org:ws. The workspace must be some ancestor or
a child of some ancestor. If it is unset, the path of the
APIBinding is used.
e.g. root:org:ws. If it is unset, the path of the APIBinding
is used.
pattern: ^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
required:
Expand Down Expand Up @@ -164,9 +163,8 @@ spec:
type: string
path:
description: path is an absolute reference to a workspace,
e.g. root:org:ws. The workspace must be some ancestor or
a child of some ancestor. If it is unset, the path of the
APIBinding is used.
e.g. root:org:ws. If it is unset, the path of the APIBinding
is used.
pattern: ^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
required:
Expand Down
5 changes: 2 additions & 3 deletions config/crds/workload.kcp.dev_synctargets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,8 @@ spec:
type: string
path:
description: path is an absolute reference to a workspace,
e.g. root:org:ws. The workspace must be some ancestor
or a child of some ancestor. If it is unset, the path
of the APIBinding is used.
e.g. root:org:ws. If it is unset, the path of the APIBinding
is used.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was no code checking this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was, this is the expected state.

pattern: ^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
required:
Expand Down
2 changes: 1 addition & 1 deletion config/root-phase0/apiexport-workload.kcp.dev.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ metadata:
name: workload.kcp.dev
spec:
latestResourceSchemas:
- v220909-c255fd13.synctargets.workload.kcp.dev
- v220923-836dfac8.synctargets.workload.kcp.dev
status: {}
7 changes: 3 additions & 4 deletions config/root-phase0/apiresourceschema-synctargets.workload.kcp.dev.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: apis.kcp.dev/v1alpha1
kind: APIResourceSchema
metadata:
creationTimestamp: null
name: v220909-c255fd13.synctargets.workload.kcp.dev
name: v220923-836dfac8.synctargets.workload.kcp.dev
spec:
group: workload.kcp.dev
names:
Expand Down Expand Up @@ -98,9 +98,8 @@ spec:
type: string
path:
description: path is an absolute reference to a workspace,
e.g. root:org:ws. The workspace must be some ancestor or
a child of some ancestor. If it is unset, the path of the
APIBinding is used.
e.g. root:org:ws. If it is unset, the path of the APIBinding
is used.
pattern: ^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
type: string
required:
Expand Down
19 changes: 2 additions & 17 deletions pkg/admission/apibinding/apibinding_admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,22 +165,7 @@ func (o *apiBindingAdmission) Validate(ctx context.Context, a admission.Attribut
}

// Verify the workspace reference.
var apiExportClusterName logicalcluster.Name
cluster, err := genericapirequest.ValidClusterFrom(ctx)
if err != nil {
return admission.NewForbidden(a, fmt.Errorf("error determining workspace: %w", err))
}
switch {
case apiBinding.Spec.Reference.Workspace.Path != "":
absoluteRef := logicalcluster.New(apiBinding.Spec.Reference.Workspace.Path)
absoluteRefParent, _ := absoluteRef.Parent()
isAncestor := cluster.Name.HasPrefix(absoluteRef)
isAncestorChild := cluster.Name != absoluteRefParent && cluster.Name.HasPrefix(absoluteRefParent)
if !isAncestor && !isAncestorChild {
return admission.NewForbidden(a, fmt.Errorf("spec.reference.workspace.path: not pointing to an ancestor or child of an ancestor of %q", cluster.Name))
}
apiExportClusterName = absoluteRef
default:
if apiBinding.Spec.Reference.Workspace.Path == "" {
return admission.NewForbidden(a, fmt.Errorf("workspace reference is missing")) // this should not happen due to validation
}

Expand All @@ -196,7 +181,7 @@ func (o *apiBindingAdmission) Validate(ctx context.Context, a admission.Attribut
}

// Access check
if err := o.checkAPIExportAccess(ctx, a.GetUserInfo(), apiExportClusterName, apiBinding.Spec.Reference.Workspace.ExportName); err != nil {
if err := o.checkAPIExportAccess(ctx, a.GetUserInfo(), logicalcluster.New(apiBinding.Spec.Reference.Workspace.Path), apiBinding.Spec.Reference.Workspace.ExportName); err != nil {
action := "create"
if a.GetOperation() == admission.Update {
action = "update"
Expand Down
15 changes: 8 additions & 7 deletions pkg/admission/apibinding/apibinding_admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,19 +237,20 @@ func TestValidate(t *testing.T) {
authzDecision: authorizer.DecisionAllow,
},
{
name: "Create: complete non-ancestor absolute workspace reference fails",
name: "Create: complete non-ancestor absolute workspace reference does not fail",
attr: createAttr(
newAPIBinding().withName("test").withAbsoluteWorkspaceReference("root:some-other-org:bla", "someExport").
withLabel(apisv1alpha1.InternalAPIBindingExportLabelKey, toSha224Base62("root:some-other-org:bla")).APIBinding,
withLabel(apisv1alpha1.InternalAPIBindingExportLabelKey, toSha224Base62("root:some-other-org:bla:someExport")).APIBinding,
),
expectedErrors: []string{"spec.reference.workspace.path: not pointing to an ancestor or child of an ancestor of \"root:org:ws\""},
authzDecision: authorizer.DecisionAllow,
},
{
name: "Create: complete child absolute workspace reference fails",
name: "Create: complete child absolute workspace reference does not fail",
attr: createAttr(
newAPIBinding().withName("test").withAbsoluteWorkspaceReference("root:org:ws:child", "someExport").APIBinding,
newAPIBinding().withName("test").withAbsoluteWorkspaceReference("root:org:ws:child", "someExport").
withLabel(apisv1alpha1.InternalAPIBindingExportLabelKey, toSha224Base62("root:org:ws:child:someExport")).APIBinding,
),
expectedErrors: []string{"spec.reference.workspace.path: not pointing to an ancestor or child of an ancestor of \"root:org:ws\""},
authzDecision: authorizer.DecisionAllow,
},
{
name: "Create: complete workspace reference fails with no authorization decision",
Expand Down Expand Up @@ -449,7 +450,7 @@ func TestValidate(t *testing.T) {
err := o.Validate(ctx, tc.attr, nil)

wantErr := len(tc.expectedErrors) > 0
require.Equal(t, wantErr, err != nil)
require.Equal(t, wantErr, err != nil, "err: %v", err)

if err != nil {
t.Logf("Got admission errors: %v", err)
Expand Down
5 changes: 2 additions & 3 deletions pkg/apis/apis/v1alpha1/types_apibinding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,8 @@ type ExportReference struct {
// WorkspaceExportReference describes an API and backing implementation that are provided by an actor in the
// specified Workspace.
type WorkspaceExportReference struct {
// path is an absolute reference to a workspace, e.g. root:org:ws. The workspace must
// be some ancestor or a child of some ancestor. If it is unset, the path of the APIBinding
// is used.
// path is an absolute reference to a workspace, e.g. root:org:ws.
// If it is unset, the path of the APIBinding is used.
// +optional
// +kubebuilder:validation:Pattern:="^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$"
Path string `json:"path,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion pkg/openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 2 additions & 3 deletions pkg/virtual/framework/internalapis/fixtures/synctargets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,8 @@ spec:
type: string
path:
description: path is an absolute reference to a workspace,
e.g. root:org:ws. The workspace must be some ancestor or
a child of some ancestor. If it is unset, the path of the
APIBinding is used.
e.g. root:org:ws. If it is unset, the path of the APIBinding
is used.
type: string
required:
- exportName
Expand Down