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

Jump to bottom

🌱 *: remove tenancy.kcp.dev/v1alpha1.ClusterWorkspace #2543

Merged
merged 8 commits into from
Jan 9, 2023
Merged

🌱 *: remove tenancy.kcp.dev/v1alpha1.ClusterWorkspace #2543

merged 8 commits into from
Jan 9, 2023

Conversation

stevekuznetsov
Copy link
Contributor

@stevekuznetsov stevekuznetsov commented Jan 4, 2023 •
edited by sttts
Loading

Your PR description is really short.
Your PR description is really short.
Your PR description is really short.
Your PR description is really short.
Your PR description is really short.

@openshift-ci openshift-ci bot requested review from csams and davidfestal January 4, 2023 15:35
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jan 4, 2023
@stevekuznetsov
Copy link
Contributor Author

Hm, tests are flaky even locally. No idea why that might be - any chance it's due to these mechanical changes @ncdc ?

@stevekuznetsov
Copy link
Contributor Author

/assign @ncdc @sttts

@openshift-ci openshift-ci bot assigned ncdc and sttts Jan 4, 2023
stevekuznetsov
stevekuznetsov commented Jan 4, 2023
View reviewed changes
go.mod Outdated
@@ -9,6 +9,7 @@ require (
github.com/bombsimon/logrusr/v3 v3.0.0
github.com/coredns/caddy v1.1.1
github.com/coredns/coredns v1.9.3
github.com/dave/dst v0.27.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will drop the auto-refactor file & go.mod diffs when ready to merge.

@stevekuznetsov
Copy link
Contributor Author

/retest

@stevekuznetsov stevekuznetsov mentioned this pull request Jan 5, 2023
Merged
34 tasks
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2023
@stevekuznetsov stevekuznetsov changed the title 🌱 *: remove tenancy.kcp.dev/v1alpha1 🌱 *: remove tenancy.kcp.dev/v1alpha1.ClusterWorkspace Jan 6, 2023
@stevekuznetsov
Copy link
Contributor Author

/retest

@stevekuznetsov
Copy link
Contributor Author 

    --- FAIL: TestAuthorizer/without_org_access,_a_deep_SAR_with_user-1_against_org2_succeeds_even_without_org_access_for_user-1 (0.01s)
        authorizer_test.go:274: try to list ConfigMap as user-1 in "240qoqib7huzjdr8:workspace1" without access, should fail
        authorizer_test.go:274: 
            	Error Trace:	authorizer_test.go:239
            	            				authorizer_test.go:274
            	Error:      	An error is expected but got nil.
            	Test:       	TestAuthorizer/without_org_access,_a_deep_SAR_with_user-1_against_org2_succeeds_even_without_org_access_for_user-1
            	Messages:   	user-1 should not be able to list configmaps in "240qoqib7huzjdr8:workspace1"
    --- FAIL: TestAuthorizer/as_org_member,_workspace1_admin_user-1_cannot_access_workspace2,_not_even_discovery (0.00s)
        authorizer_test.go:274: 
            	Error Trace:	authorizer_test.go:160
            	            				authorizer_test.go:274
            	Error:      	An error is expected but got nil.
            	Test:       	TestAuthorizer/as_org_member,_workspace1_admin_user-1_cannot_access_workspace2,_not_even_discovery
            	Messages:   	user-1 should not be able to list server resources in a different workspace
    --- FAIL: TestAuthorizer/without_org_access,_org1_workspace1_admin_user-1_cannot_access_org2,_not_even_discovery (0.01s)
        authorizer_test.go:274: 
            	Error Trace:	authorizer_test.go:151
            	            				authorizer_test.go:274
            	Error:      	An error is expected but got nil.
            	Test:       	TestAuthorizer/without_org_access,_org1_workspace1_admin_user-1_cannot_access_org2,_not_even_discovery
            	Messages:   	user-1 should not be able to list configmaps in a different org

🤔

sttts
sttts reviewed Jan 6, 2023
View reviewed changes
refactor.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2023
@stevekuznetsov
test/e2e: update workspace options to use label selector
a133b77 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
pkg/apis: use tenancyv1alpha1.WorkspaceTypeReference
1a87489 
There's no real utility to having two different types for this reference
and having the same data inside of it regardless.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
*: automated refactor v1alpha1.ClusterWorkspace -> v1beta1.Workspace
35b2267 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
teat/e2e: fixup a workspace location literal
16a9ade 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
*: remove ClusterWorkspace projection
d67c63c 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
*: removce tenancyv1alpha1.ClusterWorkspace types
ea4dd9d 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
test/e2e: improve end-to-end authorizer tests
bc2399d 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov
test/e2e: create privileged workspaces as shard admin
e1320de 
In order to set the required-groups annotation, we must be the shard
admin. However, by being that powerful, we side-step admission and any
workspaces created with that privilege do not have owner annotations,
which are required for workspace initialization. Therefore, when we use
that privilege we also need to hard-code a workspace owner annotation
ourselves as well.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2023
@sttts
Copy link
Member

sttts commented Jan 9, 2023

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 9, 2023

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit 16e5542 into kcp-dev:main Jan 9, 2023
sttts
sttts reviewed Jan 9, 2023
View reviewed changes
pkg/server/options/authorization.go
localAuth, localResolver := authz.NewLocalAuthorizer(informer)
localAuth = authz.NewDecorator("local.authorization.kcp.io", localAuth).AddAuditLogging().AddAnonymization().AddReasonAnnotation()

// everything below - skipped for Deep SAR
Copy link
Member

Choose a reason for hiding this comment

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

@s-urbaniak this shouldn't be the case. maxPermissionPolicyAuth and systemCRDAuth should both be applied for deep authz.

Copy link
Contributor

Choose a reason for hiding this comment

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

hrmpf, sorry, explained that wrong, my bad, will follow-up with a PR tomorrow 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

skipping max perm authorizer seems to be necessary to fix #2384 (fixed in #2385). what are we missing?

@sttts sttts mentioned this pull request Jan 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@s-urbaniak s-urbaniak s-urbaniak left review comments

@csams csams Awaiting requested review from csams

@davidfestal davidfestal Awaiting requested review from davidfestal

Assignees

@ncdc ncdc

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/workspaces kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stevekuznetsov @sttts @s-urbaniak @ncdc @openshift-merge-robot