🐛 add support for standalone virtualworkspace server #2407

hardys · 2022-11-24T15:22:10Z

Summary

This is another change decoupled from the shard-standalone-vw branch from @ncdc

Related issue(s)

Follow up to ##2297 and #2303 which also merged changes from the WIP branch

Fixes #2254
Fixes #2056

hardys · 2022-11-24T15:23:27Z

Some of the e2e tests are failing hence marking this as WIP until resolved, any help with that or initial review feedback is welcome :)

pkg/server/controllers.go

pkg/server/handler.go

pkg/server/virtual.go

test/e2e/virtual/workspaces/virtual_workspace_test.go

hardys · 2022-11-30T15:46:39Z

Thanks for the review feedback @ncdc - I pushed an update which fixes most (but not yet all) of the e2e failures, will leave this as WIP until all tests are green and the review feedback is addressed 👍

cmd/sharded-test-server/virtual.go

pkg/server/controllers.go

stevekuznetsov

Seems mostly reasonable, but boy is that an enormous test setup

test/e2e/virtual/workspaces/virtual_workspace_test.go

hardys · 2022-12-13T18:11:13Z

@ncdc quick update on this as I'm still struggling somewhat to get the e2e tests all working in standalone-vw mode

AFAICS some tests (for example TestRootWorkspaces/a_user_sees_his_own_workspaces_at_the_root,_but_no_other_workspaces are failing because they hit the ForeverTestTimeout of 30s.

Locally testing (with sharded-test-server configured with --shard-run-virtual-workspaces=false) e.g

GOOS=linux GOARCH=amd64 go test -v -failfast -parallel=1 -count=1 ./test/e2e/virtual/workspaces/... -run TestRootWorkspaces/a_user_sees_his_own_workspaces_at_the_root,_but_no_other_workspaces -args --use-default-kcp-server --root-shard-kubeconfig=$PWD/.kcp-0/admin.kubeconfig

We see that the test sometimes passes, and if I increase the test timeout it can pass reliably - in some test runs locally I'm seeing a delay of around 50 seconds waiting for the user1-workspace to appear (this is on baremetal so not resource constrained, although I do see high CPU usage).

In the shard logs we can see the proxied requests, and there are some bootstrapping authz errors which I'm thinking may be related?

I1213 17:56:41.503077 2707520 handler.go:138] "proxying virtual workspace request" inURL="/services/workspaces/root/apis/tenancy.kcp.dev/v1beta1/workspaces" outURL="https://10.19.133.66:7444/services/workspaces/root/apis/tenancy.kcp.dev/v1beta1/workspaces"
I1213 17:56:41.506245 2707520 httplog.go:131] "HTTP" verb="POST" URI="/clusters/root/apis/authorization.k8s.io/v1/subjectaccessreviews" latency="1.454168ms" userAgent="virtual-workspaces/v0.0.0 (linux/amd64) kubernetes/$Format/workspaces-virtual-workspace" audit-ID="5cd9b73b-6526-4798-ae35-3d85c4231534" srcIP="[::1]:35636" resp=201
I1213 17:56:41.507868 2707520 decorator.go:75] "bootstrap policy reason: "
I1213 17:56:41.507930 2707520 decorator.go:75] "local policy for cluster \"root\" reason: "
I1213 17:56:41.507940 2707520 decorator.go:75] "delegated bootstrap.authorization.kcp.dev authorizer reason: access denied\ndelegated local.authorization.kcp.dev authorizer reason: access denied"
I1213 17:56:41.507947 2707520 decorator.go:75] "delegated maxpermissionpolicy.authorization.kcp.dev authorizer reason: access denied"
I1213 17:56:41.507952 2707520 decorator.go:75] "delegated systemcrd.authorization.kcp.dev authorizer reason: access denied"
I1213 17:56:41.507958 2707520 decorator.go:75] "delegated content.authorization.kcp.dev authorizer reason: access denied"

If you have any suggestions where to look next that would be welcome, currently I'm not very sure why there is such a long delay in the workspace creation.

ncdc · 2022-12-13T18:15:36Z

@hardys this particular test will go away as part of the workspace refactoring. Do you have a list of all the tests you're having problems with, and we can see if any of them still need addressing?

hardys · 2022-12-13T18:27:11Z

@hardys this particular test will go away as part of the workspace refactoring. Do you have a list of all the tests you're having problems with, and we can see if any of them still need addressing?

@ncdc I've mostly been testing e2e-sharded-minimal locally so I don't yet have an exhaustive list, but I'm seeing TestInProcessWorkspacesVirtualWorkspaces/In-process_virtual_workspace_apiserver/create_a_sub-workspace_and_verify_that_only_users_with_right_permissions_can_access_workspaces_inside_it also fail (similar to above, sometimes it passes)

I'm interested to understand why the long delay though, with the in-process VW the workspace list succeeds pretty much immediately, any idea why is it taking nearly a minute in standalone VW mode?

ncdc · 2022-12-13T18:30:30Z

Not offhand. Maybe something to do with the workspace authz cache (which will be going away as part of the workspace refactor)

hardys · 2022-12-13T18:32:58Z

Not offhand. Maybe something to do with the workspace authz cache (which will be going away as part of the workspace refactor)

Ack thanks, sounds like I should probably pause work on this and rebase after the workspace refactor lands :)

s-urbaniak · 2023-01-02T10:19:54Z

@hardys let me know if you need help after the rebase, from the logs it looks like the requesting user for the SAR passes all authorizers but the local and bootstrap policy one for the root workspace. Unfortunately kubernetes RBAC does not give any reason in case of denials, so the local and bootstrap policy authorizer return empty string.

hardys · 2023-01-16T14:37:52Z

/test e2e-shared

Looks like #1878

Co-authored-by: Andy Goldstein <andy.goldstein@redhat.com>

Align with the shard WaitForReady pattern ref kcp-dev#2303 as a step towards enabling multiple VW servers when there are multiple shards

Avoid calculating the port in multiple places, and also use JoinHostByPort instead of Sprintf

hardys · 2023-01-19T17:37:26Z

/hold cancel

@ncdc @stevekuznetsov this is ready for another review when you get some time, thanks!

openshift-ci · 2023-01-19T17:57:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc

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~~ [ncdc]

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

In kcp-dev#2407 we introduced this validation, but it doesn't handle the case where the VW server is not standalone, but you still want to specify --shard-virtual-workspace-url This scenario is likely when deploying a sharded environment, since you'll want the VW traffic to go via the front-proxy regardless of whether the VW server is standalone or not.

Partially reverts logic introduced in kcp-dev#2407 so we don't unconditionally use the external URL - when non-standalone VW mode is used we still need the old behavior, or the loopback client can't access the VW server. This breaks shard bootstrapping (evidently not in our e2e scenarios, but in a real deployment where probes mean the proxy isn't ready until the shard bootstrap is complete)

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 24, 2022

openshift-ci bot requested review from qiujian16 and stevekuznetsov November 24, 2022 15:22

hardys changed the title ~~[WIP] 🐛 sharded-test-server: add support for standalone virtualworkspace server~~ [WIP] 🐛 add support for standalone virtualworkspace server Nov 24, 2022

hardys force-pushed the shard-standalone-vw2 branch from 2347ae9 to 3cf20ac Compare November 24, 2022 15:24

ncdc reviewed Nov 29, 2022

View reviewed changes

pkg/server/controllers.go Outdated Show resolved Hide resolved

pkg/server/handler.go Outdated Show resolved Hide resolved

pkg/server/virtual.go Outdated Show resolved Hide resolved

test/e2e/virtual/workspaces/virtual_workspace_test.go Outdated Show resolved Hide resolved

hardys force-pushed the shard-standalone-vw2 branch 2 times, most recently from 2f0a93a to c60b0e6 Compare November 30, 2022 15:42

hardys commented Nov 30, 2022

View reviewed changes

cmd/sharded-test-server/virtual.go Show resolved Hide resolved

ncdc reviewed Nov 30, 2022

View reviewed changes

cmd/sharded-test-server/virtual.go Outdated Show resolved Hide resolved

hardys force-pushed the shard-standalone-vw2 branch from c60b0e6 to 7303446 Compare December 1, 2022 15:18

hardys commented Dec 1, 2022

View reviewed changes

pkg/server/controllers.go Outdated Show resolved Hide resolved

hardys force-pushed the shard-standalone-vw2 branch 2 times, most recently from 90e3c6b to 13fd4fd Compare December 5, 2022 18:10

stevekuznetsov reviewed Dec 5, 2022

View reviewed changes

test/e2e/virtual/workspaces/virtual_workspace_test.go Outdated Show resolved Hide resolved

test/e2e/virtual/workspaces/virtual_workspace_test.go Outdated Show resolved Hide resolved

test/e2e/virtual/workspaces/virtual_workspace_test.go Outdated Show resolved Hide resolved

hardys force-pushed the shard-standalone-vw2 branch 2 times, most recently from e2c1812 to c4fa992 Compare December 7, 2022 09:24

hardys force-pushed the shard-standalone-vw2 branch from c4fa992 to 27a1c3e Compare December 13, 2022 17:59

hardys force-pushed the shard-standalone-vw2 branch from 27a1c3e to 0d97d5c Compare December 13, 2022 18:46

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2022

hardys force-pushed the shard-standalone-vw2 branch from 0d97d5c to 4d8d130 Compare January 5, 2023 10:54

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2023

This was referenced Jan 11, 2023

🐛 e2e: PrivateKcpServer enable audit-policy #2591

Merged

🌱 e2e: TestAuditLogs remove duplicate audit-log-path #2610

Merged

hardys force-pushed the shard-standalone-vw2 branch from cb13f1c to 57b01aa Compare January 16, 2023 11:42

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2023

hardys force-pushed the shard-standalone-vw2 branch from 57b01aa to a99f0a9 Compare January 16, 2023 13:51

stevekuznetsov mentioned this pull request Jan 16, 2023

🌱 run replication tests on a shared kcp instance #2620

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2023

Steven Hardy and others added 7 commits January 19, 2023 12:07

server: support standalone vw

a016b67

Co-authored-by: Andy Goldstein <andy.goldstein@redhat.com>

cmd/virtual-workspaces: create client for shard

7fab357

Co-authored-by: Andy Goldstein <andy.goldstein@redhat.com>

sharded-test-server: support standalone vw

b331d9a

Co-authored-by: Andy Goldstein <andy.goldstein@redhat.com>

e2e-sharded enable standalone vw

6efd47f

Co-authored-by: Andy Goldstein <andy.goldstein@redhat.com>

sharded-test-server: wait for vw readiness

6037c7a

sharded-test-server: refactor vw to start/wait

4981b46

Align with the shard WaitForReady pattern ref kcp-dev#2303 as a step towards enabling multiple VW servers when there are multiple shards

sharded-test-server: add virtualWorkspacePort

b32ddf0

Avoid calculating the port in multiple places, and also use JoinHostByPort instead of Sprintf

hardys force-pushed the shard-standalone-vw2 branch from a99f0a9 to b32ddf0 Compare January 19, 2023 12:21

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2023

ncdc approved these changes Jan 19, 2023

View reviewed changes

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

openshift-merge-robot merged commit 43b6fc2 into kcp-dev:main Jan 19, 2023

hardys mentioned this pull request Jan 20, 2023

🐛 server: relax ShardVirtualWorkspace flag validation #2659

Merged

hardys mentioned this pull request Jan 23, 2023

🐛 server: revert non-standalone VW URL #2667

Merged

hardys mentioned this pull request Jan 25, 2023

[WIP] 🐛 sharded-test-server: Add external-hostname flag #2672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 add support for standalone virtualworkspace server #2407

🐛 add support for standalone virtualworkspace server #2407

hardys commented Nov 24, 2022 •

edited

Loading

hardys commented Nov 24, 2022

hardys commented Nov 30, 2022

stevekuznetsov left a comment

hardys commented Dec 13, 2022

ncdc commented Dec 13, 2022

hardys commented Dec 13, 2022

ncdc commented Dec 13, 2022

hardys commented Dec 13, 2022

s-urbaniak commented Jan 2, 2023 •

edited

Loading

hardys commented Jan 16, 2023

hardys commented Jan 19, 2023

openshift-ci bot commented Jan 19, 2023

🐛 add support for standalone virtualworkspace server #2407

🐛 add support for standalone virtualworkspace server #2407

Conversation

hardys commented Nov 24, 2022 • edited Loading

Summary

Related issue(s)

hardys commented Nov 24, 2022

hardys commented Nov 30, 2022

stevekuznetsov left a comment

Choose a reason for hiding this comment

hardys commented Dec 13, 2022

ncdc commented Dec 13, 2022

hardys commented Dec 13, 2022

ncdc commented Dec 13, 2022

hardys commented Dec 13, 2022

s-urbaniak commented Jan 2, 2023 • edited Loading

hardys commented Jan 16, 2023

hardys commented Jan 19, 2023

openshift-ci bot commented Jan 19, 2023

hardys commented Nov 24, 2022 •

edited

Loading

s-urbaniak commented Jan 2, 2023 •

edited

Loading