Skip to content

feat: add kubelet http2 support#180

Merged
koordinator-bot[bot] merged 1 commit intokoordinator-sh:mainfrom
LambdaHJ:cce-kubelet-tls
Jun 8, 2022
Merged

feat: add kubelet http2 support#180
koordinator-bot[bot] merged 1 commit intokoordinator-sh:mainfrom
LambdaHJ:cce-kubelet-tls

Conversation

@LambdaHJ
Copy link
Contributor

Ⅰ. Describe what this PR does

support cce kubernetes cluster kubelet read-only port http2 scheme

Ⅱ. Does this pull request fix one issue?

fixes #150

Ⅲ. Describe how to verify it

Ⅳ. Special notes for reviews

Copy link
Member

@jasonliu747 jasonliu747 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Please follow the instruction to sign off your commit.

Copy link
Member

@saintube saintube left a comment

Choose a reason for hiding this comment

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

please don't forget to add unit tests :)

@jasonliu747
Copy link
Member

jasonliu747 commented May 30, 2022

@LambdaHJ came up with another soultion. Have you ever tried using token in /var/run/secrets/kubernetes.io/serviceaccount/token as BearerToken to make an HTTPS request?

@jasonliu747 jasonliu747 linked an issue May 31, 2022 that may be closed by this pull request
@jasonliu747
Copy link
Member

jasonliu747 commented May 31, 2022

As discussed earlier, let's use service account as token to access kubelet. And use apiserver as a failover only when kubelet doesn't work. There might be some pre-conditions to be met for token based authn/authz. For more information, please check https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet-authentication-authorization/

Koordlet needs to reach node address and kubelet port. Addresses and ports are configured in kubelet and published as part of Node object. Addresses in .status.addresses and port in .status.daemonEndpoints.kubeletEndpoint.port field (default 10250).

And we can add a command line flag called kubelet-preferred-address-type(default to InternalIP and raise an error if not one of them: [Hostname, InternalIP, ExternalIP, InternalDNS, ExternalDNS]). Instead of 0.0.0.0, koordlet would pick that node address based on the flag.

@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #180 (c561879) into main (fc8db7c) will increase coverage by 0.40%.
The diff coverage is 63.77%.

❗ Current head c561879 differs from pull request most recent head 58bafa6. Consider uploading reports for the commit 58bafa6 to get more accurate results

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
+ Coverage   58.63%   59.03%   +0.40%     
==========================================
  Files         100       99       -1     
  Lines        9222     9295      +73     
==========================================
+ Hits         5407     5487      +80     
+ Misses       3364     3347      -17     
- Partials      451      461      +10     
Flag Coverage Δ
unittests 59.03% <63.77%> (+0.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/koordlet/statesinformer/states_informer.go 37.50% <27.77%> (+1.30%) ⬆️
pkg/koordlet/statesinformer/states_pod.go 73.77% <68.00%> (+32.86%) ⬆️
pkg/koordlet/statesinformer/kubelet_stub.go 66.66% <72.09%> (+31.37%) ⬆️
pkg/koordlet/statesinformer/config.go 100.00% <100.00%> (+41.66%) ⬆️
pkg/util/node.go 54.76% <100.00%> (+25.13%) ⬆️
pkg/runtimeproxy/server/docker/utils.go 17.50% <0.00%> (-12.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc8db7c...58bafa6. Read the comment docs.

@koordinator-bot koordinator-bot bot added size/XXL and removed size/L labels Jun 6, 2022
@koordinator-bot koordinator-bot bot added size/XL and removed size/XXL labels Jun 7, 2022
@LambdaHJ LambdaHJ force-pushed the cce-kubelet-tls branch 6 times, most recently from c38fe04 to ff7e78b Compare June 7, 2022 10:55
@eahydra
Copy link
Member

eahydra commented Jun 7, 2022

/lgtm

Signed-off-by: 黄金 <heikkihuang@kugou.net>
@koordinator-bot koordinator-bot bot removed the lgtm label Jun 8, 2022
@hormes
Copy link
Member

hormes commented Jun 8, 2022

/lgtm
/approve

@koordinator-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hormes

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

The pull request process is described here

Details 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

@koordinator-bot koordinator-bot bot merged commit e6d2498 into koordinator-sh:main Jun 8, 2022
Copy link
Member

@jasonliu747 jasonliu747 left a comment

Choose a reason for hiding this comment

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

/lgtm
Very nice PR, thanks for your contribution. @LambdaHJ

eahydra pushed a commit to eahydra/koordinator that referenced this pull request Jun 17, 2022
Signed-off-by: 黄金 <heikkihuang@kugou.net>

Co-authored-by: 黄金 <heikkihuang@kugou.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

7 participants