Replace k8sapiserver startAPIServer to use Kubernetes createServerChain #121

matthewygf · 2022-02-28T12:19:16Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR tries to extend the simulator apiserver to accept CRDs, this is useful because some scheduler-plugins uses CRDs. #77

Additionally, we can directly use the Kubernetes/api-server/createServerChain method to reduce the burden of maintaining API code.

This is a PR spawned from a discussion in PR #113

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

After this extension, user can use kubectl to apply CRDs.

/label tide/merge-method-squash

… compose

k8s-ci-robot · 2022-02-28T12:19:24Z

Hi @matthewygf. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

matthewygf · 2022-02-28T12:24:25Z

/assign @sanposhiho

matthewygf · 2022-02-28T12:37:28Z

@sanposhiho , to build my branch successfully locally, I had to use "go mod vendor", and then generate the openAPI definition in kubernetes/hack/update-generated-openapi.sh to make it work.

I suspect this is why the test is failling, and I am not sure how to fix this test... would you be able to advise ... ?

sanposhiho · 2022-03-03T13:07:18Z

/ok-to-test

sanposhiho · 2022-03-03T13:49:35Z

@matthewygf
Thanks for your work!

I think we need to use go mod vendor as you said and replace this line to:

OPENAPIFILE="vendor/k8s.io/kubernetes/pkg/generated/openapi/zz_generated.openapi.go"

Then, after running make openapi on project root, we can resolve the error.

(We are using make openapi to generate openapi in submodules/kubernetes dir and move generated openapi file to our project.)

And btw, I found another bug on /hack/openapi.sh 😓 Please replace this line to:

cp pkg/generated/openapi/zz_generated.openapi.go "../../${OPENAPIFILE}"

matthewygf · 2022-03-03T20:40:24Z

@sanposhiho Thanks for the suggestion !

Your method did work ! however I thought the make kube-apiserver took too long. So I have borrowed the hack/open-api sh into my branch, which speeds up the gen open-api by a bit. (See the updated hack/openapi.sh)

The test currently fails because the env $GOPATH seems to be not set for make test. Do you know how to set the env in github action ? :')

sanposhiho · 2022-04-13T11:51:24Z

k8sapiserver/k8sapiserver.go

-		Port: 443,
+func createK8SAPIChainedServer(etcdURL, frontendURL string) (*apiserver.APIAggregator, error) {
+	serverOpts, cleanupFunc, err := createK8SAPIServerOpts(etcdURL, frontendURL)
+	defer cleanupFunc()


We shouldn't register cleanupFunc to defer here since cleanup will be executed when createK8SAPIChainedServer is finished.

Make sense, I am going to pass the cleanupFunc all the way up to the shutdown Func instead.

sanposhiho · 2022-04-13T11:56:03Z

@matthewygf
It seems that all requests to API server from web client make panic invalid memory address or nil pointer dereference
on api-server. 🤔
I'm investigating that, but no cause identified yet...

Could you check if the panic occurs on your machine as well or not?

sanposhiho · 2022-04-13T14:16:27Z

It seems that only requests from Web UI doesn't work well. So this change is related probably.
#115

sanposhiho · 2022-04-13T14:44:46Z

Can send a request via kubectl. Hmmm, only requests from web client have the issue 🤔

$ kubectl get pods  --server=http://localhost:3131      
NAME   READY   STATUS    RESTARTS   AGE
hoge   0/1     Pending   0          16s

sanposhiho · 2022-04-13T16:23:03Z

k8sapiserver/k8sapiserver.go

+/IR3qCXyThP/dbCiHrF3v1cuhBOHY8CLVg==
+-----END EC PRIVATE KEY-----`
+
+// StartAPIServer starts both the secure k8sAPIServer and proxy server to handle insecure serving, and it make panic when a error happen.
 func StartAPIServer(kubeAPIServerURL, etcdURL, frontendURL string) (*restclient.Config, func(), error) {
 	h := &APIServerHolder{Initialized: make(chan struct{})}
 	handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {


Okay, finally found work-around... It seems that user-agent causes the panic. I don't know why 🤷‍♂️🤷‍♂️🤷‍♂️
Let's overwrite this. This work-around works for me. (Other good work-around is welcome)

Suggested change

handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {

handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {

// work-around: we need to overwrite user-agent from client.

// some user-agents causes a panic on kube-apiserver.

req.Header.Set("User-Agent", "kubectl/v1.21.5 (darwin/amd64) kubernetes/aea7bba")

I0414 21:43:43.780933 1 trace.go:205] Trace[354409240]: "Delete" url:/api/v1/namespaces/default/pods,user-agent:simulator/v0.0.0 (linux/amd64) kubernetes/$Format,audit-id:7b1c1668-a971-440e-a841-eff8aeee635e,client:127.0.0.1,accept:application/json, */*,protocol:HTTP/1.1 (14-Apr-2022 21:43:42.177) (total time: 1603ms) - I am not sure how or why you have to set the user-agent😲, in the kube-apiserver trace, it seems the user-agent is always set to simulator/v0.0.0. 🤔

My requests are from WebUI, hitting the reset button.

No panic with kubectl either 🤔

How about something like this

From my investigation, we need to overwrite user-agent. Some user-agent (chrome is one of them) breaks api-server. 🤔

Let me check if I can run simulator successfully on docker container.

k8sapiserver/k8sapiserver.go

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

matthewygf · 2022-04-14T21:21:04Z

@matthewygf It seems that all requests to API server from web client make panic invalid memory address or nil pointer dereference on api-server. 🤔 I'm investigating that, but no cause identified yet...

Could you check if the panic occurs on your machine as well or not?

Hmmm, @sanposhiho I haven't got any panic when making request via webUI... 🤔 I run the simulator-server using docker compose, how do you run the simulator server ?

sanposhiho · 2022-04-15T11:09:21Z

I run the simulator-server using docker compose, how do you run the simulator server ?

Run locally with make start + yarn dev and then sent request from web ui(chrome).

sanposhiho · 2022-04-15T11:53:50Z

Okay, I also could run the simulator without panic in docker container. Hmmmmmm. 🤷‍♂️🤷‍♂️
But, in my docker container, all request's user-agent are Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.75 Safari/537.36 (the same user-agent when I run the simulator locally)
So,,, if requests have chrome user-agent (or other non-kubernetes supported user-agent), when we run the simulator locally, kube-apiserver will break and when we run the simulator in docker container, it won't. 🤔

Could you run the simulator locally with make start and yarn dev and check if api-server panic or not? @matthewygf

matthewygf · 2022-04-17T14:47:17Z

Okay, I also could run the simulator without panic in docker container. Hmmmmmm. 🤷‍♂️🤷‍♂️ But, in my docker container, all request's user-agent are Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/100.0.4896.75 Safari/537.36 (the same user-agent when I run the simulator locally) So,,, if requests have chrome user-agent (or other non-kubernetes supported user-agent), when we run the simulator locally, kube-apiserver will break and when we run the simulator in docker container, it won't. 🤔

Could you run the simulator locally with make start and yarn dev and check if api-server panic or not? @matthewygf

@sanposhiho I wasn't able to build the webui from yarn dev, (so I start frontend using docker container) but I managed to start the server locally with make start. I tested it with chrome after commented out the user agent default code and it seems to be fine 🤷‍♂️... Can you check whether using frontend with docker works ? Or using frontend with production build ? yarn build && yarn start.

Maybe another issue is the apple user agent ? My machine is a windows, and I start the simulator in wsl (windows subsystem for linux).

sanposhiho · 2022-04-18T12:34:06Z

Sorry, I didn't update master in my fork. The latest master also has this problem.
I raised an issue for this problem: #147
So, I think we can merge this branch without solving this. 👌 Thanks for investigating with me.

Can you check whether using frontend with docker works? Or using frontend with production build ? yarn build && yarn start.

Yes. When I use docker (make docker_build_and_up), it works fine. I also tried in another PC (M1 Mac Pro 2020) and could reproduce this. So this issue doesn't seem to be due to my machine.

Also, I tried "one in docker, the other locally":

run frontend locally, and run backend in docker → works fine.
run backend(+ kube-apiserver) locally and run frontend in docker → panic.

So, this problem is only happens when we run backend locally.

sanposhiho · 2022-04-18T12:38:23Z

k8sapiserver/k8sapiserver.go

+		if req.UserAgent() == "" {
+			req.Header.Set("User-Agent", restclient.DefaultKubernetesUserAgent())
+		}


Sorry, could you please revert this? I'll try to fix this issue in another PR.

reverted :)

196Ikuchil · 2022-04-18T13:10:35Z

@matthewygf @sanposhiho
Hi! Finally, #100 has merged. Please check it if you need it.

#121 (comment)

This reverts commit b06408a.

sanposhiho · 2022-04-18T14:12:16Z

@matthewygf
Thanks for your great work!! I am very happy to merge this change.
Supporting CRD through this change is a great step forward for us. 🎉

/lgtm
/approve

sanposhiho · 2022-04-18T14:12:21Z

/hold cancel

k8s-ci-robot · 2022-04-18T14:13:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matthewygf, sanposhiho

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

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

matthewygf added 10 commits February 3, 2022 15:02

Able to configure API server and port, thus expose it is possible via…

1a67a16

… compose

linters

7a2910b

update property name to be more descriptive

7d4bc50

change the parameter arg to be more descriptive

66eaa4a

Merge branch 'kubernetes-sigs:master' into master

3780613

Merge branch 'kubernetes-sigs:master' into master

0849ddc

Merge branch 'kubernetes-sigs:master' into master

cd9871f

WIP - use create server chain to start k8s api server.

419361d

WIP - remove unecessary shutdown fn

e063005

setup annoymoususer for insecure testing

85c3f55

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 28, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 28, 2022

k8s-ci-robot requested review from adtac and alculquicondor February 28, 2022 12:19

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2022

matthewygf mentioned this pull request Feb 28, 2022

Added APIExtensionServer and AggregatorServer into the simulator to accept CRDs #113

Closed

k8s-ci-robot assigned sanposhiho Feb 28, 2022

update go mod sum

0315c98

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 3, 2022

instead of building kube-api , only do openapi-gen

50752f0

sanposhiho reviewed Apr 13, 2022

View reviewed changes

k8sapiserver/k8sapiserver.go Outdated Show resolved Hide resolved

sanposhiho mentioned this pull request Apr 13, 2022

enable admission control with Priority admission plugin to support Preemption #75

Closed

sanposhiho reviewed Apr 13, 2022

View reviewed changes

k8sapiserver/k8sapiserver.go Outdated Show resolved Hide resolved

matthewygf and others added 2 commits April 14, 2022 22:12

Enable Priority Plugin for admission

56ffe65

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

Change the order of logging to ensure url is correct.

d2fe507

Co-authored-by: Kensei Nakada <handbomusic@gmail.com>

matthewygf added 2 commits April 14, 2022 22:33

pass cleanupFunc all the way to shutdown

48c2813

attemp to fix useragent issue

b06408a

sanposhiho mentioned this pull request Apr 18, 2022

kube-apiserver panics when run backend locally #147

Closed

sanposhiho reviewed Apr 18, 2022

View reviewed changes

Revert "attemp to fix useragent issue"

f201a12

This reverts commit b06408a.

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 18, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2022

k8s-ci-robot merged commit 70ab63b into kubernetes-sigs:master Apr 18, 2022

sanposhiho mentioned this pull request Apr 18, 2022

Support plugins in sigs/scheduler-plugins? #77

Closed

sanposhiho mentioned this pull request May 6, 2022

enable admission control with Priority admission plugin to support Preemption #74

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace k8sapiserver startAPIServer to use Kubernetes createServerChain #121

Replace k8sapiserver startAPIServer to use Kubernetes createServerChain #121

matthewygf commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

matthewygf commented Feb 28, 2022

matthewygf commented Feb 28, 2022

sanposhiho commented Mar 3, 2022

sanposhiho commented Mar 3, 2022

matthewygf commented Mar 3, 2022 •

edited

Loading

sanposhiho Apr 13, 2022 •

edited

Loading

matthewygf Apr 14, 2022

sanposhiho commented Apr 13, 2022 •

edited

Loading

sanposhiho commented Apr 13, 2022

sanposhiho commented Apr 13, 2022

sanposhiho Apr 13, 2022 •

edited

Loading

matthewygf Apr 14, 2022 •

edited

Loading

matthewygf Apr 14, 2022

matthewygf Apr 14, 2022

sanposhiho Apr 15, 2022

sanposhiho Apr 15, 2022

matthewygf commented Apr 14, 2022 •

edited

Loading

sanposhiho commented Apr 15, 2022 •

edited

Loading

sanposhiho commented Apr 15, 2022 •

edited

Loading

matthewygf commented Apr 17, 2022 •

edited

Loading

sanposhiho commented Apr 18, 2022

sanposhiho Apr 18, 2022 •

edited

Loading

matthewygf Apr 18, 2022

196Ikuchil commented Apr 18, 2022

sanposhiho commented Apr 18, 2022

sanposhiho commented Apr 18, 2022

k8s-ci-robot commented Apr 18, 2022

-	handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+	handler := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		// work-around: we need to overwrite user-agent from client.
+		// some user-agents causes a panic on kube-apiserver.
+		req.Header.Set("User-Agent", "kubectl/v1.21.5 (darwin/amd64) kubernetes/aea7bba")

Replace k8sapiserver startAPIServer to use Kubernetes createServerChain #121

Replace k8sapiserver startAPIServer to use Kubernetes createServerChain #121

Conversation

matthewygf commented Feb 28, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

k8s-ci-robot commented Feb 28, 2022

matthewygf commented Feb 28, 2022

matthewygf commented Feb 28, 2022

sanposhiho commented Mar 3, 2022

sanposhiho commented Mar 3, 2022

matthewygf commented Mar 3, 2022 • edited Loading

sanposhiho Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

matthewygf Apr 14, 2022

Choose a reason for hiding this comment

sanposhiho commented Apr 13, 2022 • edited Loading

sanposhiho commented Apr 13, 2022

sanposhiho commented Apr 13, 2022

sanposhiho Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

matthewygf Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

matthewygf Apr 14, 2022

Choose a reason for hiding this comment

matthewygf Apr 14, 2022

Choose a reason for hiding this comment

sanposhiho Apr 15, 2022

Choose a reason for hiding this comment

sanposhiho Apr 15, 2022

Choose a reason for hiding this comment

matthewygf commented Apr 14, 2022 • edited Loading

sanposhiho commented Apr 15, 2022 • edited Loading

sanposhiho commented Apr 15, 2022 • edited Loading

matthewygf commented Apr 17, 2022 • edited Loading

sanposhiho commented Apr 18, 2022

sanposhiho Apr 18, 2022 • edited Loading

Choose a reason for hiding this comment

matthewygf Apr 18, 2022

Choose a reason for hiding this comment

196Ikuchil commented Apr 18, 2022

sanposhiho commented Apr 18, 2022

sanposhiho commented Apr 18, 2022

k8s-ci-robot commented Apr 18, 2022

matthewygf commented Mar 3, 2022 •

edited

Loading

sanposhiho Apr 13, 2022 •

edited

Loading

sanposhiho commented Apr 13, 2022 •

edited

Loading

sanposhiho Apr 13, 2022 •

edited

Loading

matthewygf Apr 14, 2022 •

edited

Loading

matthewygf commented Apr 14, 2022 •

edited

Loading

sanposhiho commented Apr 15, 2022 •

edited

Loading

sanposhiho commented Apr 15, 2022 •

edited

Loading

matthewygf commented Apr 17, 2022 •

edited

Loading

sanposhiho Apr 18, 2022 •

edited

Loading