✨ Admission for APIExportEndpointSlice #2560

fgiloux · 2023-01-06T09:59:55Z

Summary

check for binding authorization against the referenced APIExport.

Signed-off-by: Frederic Giloux [email protected]

Related issue(s)

Contributes to #2332

fgiloux · 2023-01-06T10:02:32Z

There are a few checks that seem redundant as the APIExport reference is marked as mandatory and immutable. I would welcome a second opinion on whether they should still be kept (like for APIBinding) or removed.

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

p0lyn0mial · 2023-01-09T07:55:23Z

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

+
+	var oldSlice *apisv1alpha1.APIExportEndpointSlice
+	if a.GetOperation() == admission.Update {
+		u, ok := a.GetOldObject().(*unstructured.Unstructured)


what is the purpose of this block since all it does is to assign value to oldSlice variable

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

p0lyn0mial · 2023-01-10T14:45:36Z

pkg/admission/apibinding/apibinding_admission.go

@@ -322,3 +305,23 @@ func (o *apiBindingAdmission) SetKcpInformers(informers kcpinformers.SharedInfor
 		indexers.ByLogicalClusterPathAndName: indexers.IndexByLogicalClusterPathAndName,
 	})
 }
+
+func CheckAPIExportAccess(ctx context.Context, user user.Info, apiExportName string, authz authorizer.Authorizer) error {


I'd move it to pkg/admission/helpers or maybe even pkg/authorization

I would keep it under admission. Due to the user info I am not expecting it to be useful somewhere else. I am not found of "helpers" packages... I am not finding the name helpful :-)
I will put it under pkg/admission/authorization. Let me know if you have concerns with it.

pkg/admission/apiexportendpointslice/validation.go

p0lyn0mial · 2023-01-11T09:02:31Z

pkg/admission/authorization/apiexport.go

+limitations under the License.
+*/
+
+package authorization


pkg/admission/authorization looks strange, no?

I can rename it to permissions if you prefer but that's basically the same idea

p0lyn0mial · 2023-01-11T10:11:28Z

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

+	o.apiExportIndexer = informers.Apis().V1alpha1().APIExports().Informer().GetIndexer()
+
+	indexers.AddIfNotPresentOrDie(informers.Tenancy().V1alpha1().WorkspaceTypes().Informer().GetIndexer(), cache.Indexers{
+		indexers.ByLogicalClusterPathAndName: indexers.IndexByLogicalClusterPathAndName,


why do we need this indexer?

Good point. I am removing it. It is also in the apibinding admission and I could not see any use of it there either. It could be fixed there in a subsequent PR

p0lyn0mial · 2023-01-11T10:12:29Z

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

+	o.SetReadyFunc(func() bool {
+		return apiExportsReady()
+	})
+	o.apiExportLister = informers.Apis().V1alpha1().APIExports().Lister()


where is this field used?

Good point. I am removing it. It is also in the apibinding admission and I could not see any use of it there either. It could be fixed there in a subsequent PR

p0lyn0mial · 2023-01-11T10:13:09Z

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

+// ValidateInitialization ensures the required injected fields are set.
+func (o *apiExportEndpointSliceAdmission) ValidateInitialization() error {
+	if o.deepSARClient == nil {
+		return fmt.Errorf(PluginName + " plugin needs a Kubernetes ClusterInterface")


wrong msg ? it needs a DeepSARClient

a DeepSARClient is a Kubernetes ClusterInterface

Yeah but the error is less useful if it's less specific?

e.g. there is an injector for just the client, and one very specifically for SetDeepSARClient

p0lyn0mial · 2023-01-11T11:03:12Z

/lgtm

p0lyn0mial · 2023-01-11T11:03:29Z

/assign @sttts for approval

openshift-ci · 2023-01-11T11:04:36Z

@p0lyn0mial: GitHub didn't allow me to assign the following users: for, approval.

Note that only kcp-dev members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @sttts for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go

sttts · 2023-01-11T13:15:30Z

/lgtm
/approve

openshift-ci · 2023-01-11T13:15:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]
~~pkg/admission/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fgiloux · 2023-01-11T15:31:06Z

/retest

fgiloux · 2023-01-11T19:47:11Z

/retest

sttts · 2023-01-12T12:15:58Z

/lgtm

fgiloux · 2023-01-12T15:08:52Z

I needed to rebase as the signature of delegated.NewDelegatedAuthorizer had changed. lgtm needed again. Sorry :-(

stevekuznetsov · 2023-01-12T15:11:20Z

Mind fixing #2560 (comment) while you're at it?

…ainst the referenced APIExport. Signed-off-by: Frederic Giloux <[email protected]>

sttts · 2023-01-13T08:24:44Z

/lgtm

openshift-ci bot requested review from davidfestal and sttts January 6, 2023 09:59

openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 6, 2023

p0lyn0mial reviewed Jan 9, 2023

View reviewed changes

fgiloux force-pushed the slice-permissions branch from db84695 to d8e1a2d Compare January 10, 2023 13:05

p0lyn0mial reviewed Jan 10, 2023

View reviewed changes

fgiloux force-pushed the slice-permissions branch from d8e1a2d to 8b02a34 Compare January 10, 2023 22:15

p0lyn0mial reviewed Jan 11, 2023

View reviewed changes

fgiloux force-pushed the slice-permissions branch 2 times, most recently from 6a37518 to feae98c Compare January 11, 2023 10:37

openshift-ci bot assigned sttts and p0lyn0mial Jan 11, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023

sttts reviewed Jan 11, 2023

View reviewed changes

pkg/admission/apiexportendpointslice/apiexportendpointslice_admission.go Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023

fgiloux force-pushed the slice-permissions branch from feae98c to 104797e Compare January 11, 2023 14:51

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023

fgiloux force-pushed the slice-permissions branch from 104797e to f89328b Compare January 11, 2023 21:06

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023

fgiloux force-pushed the slice-permissions branch from f89328b to 6603d5a Compare January 12, 2023 14:16

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2023

Admission for APIExportEndpointSlice, check for bind authorization ag…

0a230b9

…ainst the referenced APIExport. Signed-off-by: Frederic Giloux <[email protected]>

fgiloux force-pushed the slice-permissions branch from 6603d5a to 0a230b9 Compare January 12, 2023 15:23

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2023

openshift-merge-robot merged commit f9a3cd8 into kcp-dev:main Jan 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Admission for APIExportEndpointSlice #2560

✨ Admission for APIExportEndpointSlice #2560

fgiloux commented Jan 6, 2023

fgiloux commented Jan 6, 2023

p0lyn0mial Jan 9, 2023

p0lyn0mial Jan 10, 2023

fgiloux Jan 10, 2023

p0lyn0mial Jan 11, 2023

fgiloux Jan 11, 2023

p0lyn0mial Jan 11, 2023

fgiloux Jan 11, 2023 •

edited

Loading

p0lyn0mial Jan 11, 2023

fgiloux Jan 11, 2023

p0lyn0mial Jan 11, 2023

fgiloux Jan 11, 2023

stevekuznetsov Jan 12, 2023

stevekuznetsov Jan 12, 2023

fgiloux Jan 12, 2023

p0lyn0mial commented Jan 11, 2023

p0lyn0mial commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

sttts commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

fgiloux commented Jan 11, 2023

fgiloux commented Jan 11, 2023

sttts commented Jan 12, 2023

fgiloux commented Jan 12, 2023

stevekuznetsov commented Jan 12, 2023

sttts commented Jan 13, 2023

✨ Admission for APIExportEndpointSlice #2560

✨ Admission for APIExportEndpointSlice #2560

Conversation

fgiloux commented Jan 6, 2023

Summary

Related issue(s)

fgiloux commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgiloux Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial commented Jan 11, 2023

p0lyn0mial commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

sttts commented Jan 11, 2023

openshift-ci bot commented Jan 11, 2023

fgiloux commented Jan 11, 2023

fgiloux commented Jan 11, 2023

sttts commented Jan 12, 2023

fgiloux commented Jan 12, 2023

stevekuznetsov commented Jan 12, 2023

sttts commented Jan 13, 2023

fgiloux Jan 11, 2023 •

edited

Loading