Allow configuration of transport channel size in agent and server #624

cnvergence · 2024-05-06T11:20:46Z

See also:
#586

k8s-ci-robot · 2024-05-06T11:20:55Z

Welcome @cnvergence!

It looks like this is your first PR to kubernetes-sigs/apiserver-network-proxy 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/apiserver-network-proxy has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-05-06T11:20:56Z

Hi @cnvergence. 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-sigs/prow repository.

cmd/agent/app/options/options.go

jkh52 · 2024-05-09T23:13:19Z

/ok-to-test

/assign @cheftako

cmd/server/app/options/options.go

jkh52 · 2024-05-09T23:19:15Z

/assign @ipochi

cmd/agent/app/options/options.go

cheftako · 2024-05-10T17:30:12Z

cmd/server/app/options/options.go

@@ -136,6 +137,7 @@ func (o *ProxyRunOptions) Flags() *pflag.FlagSet {
 	flags.StringVar(&o.AuthenticationAudience, "authentication-audience", o.AuthenticationAudience, "Expected agent's token authentication audience (used with agent-namespace, agent-service-account, kubeconfig).")
 	flags.StringVar(&o.ProxyStrategies, "proxy-strategies", o.ProxyStrategies, "The list of proxy strategies used by the server to pick an agent/tunnel, available strategies are: default, destHost, defaultRoute.")
 	flags.StringSliceVar(&o.CipherSuites, "cipher-suites", o.CipherSuites, "The comma separated list of allowed cipher suites. Has no effect on TLS1.3. Empty means allow default list.")
+	flags.UintVar(&o.XfrChannelSize, "xfr-channel-size", o.XfrChannelSize, "The size of the channel for transferring data between the proxy server and the agent.")


We have 2 transfer channels in the server. 1) KAS -> KNP Server -> KNP Agent and 2) KNP Agent -> KNP Server -> KAS. This makes both channels be set with the same transfer channel size. Do we want to allow them to be configured to different values?

Initially, they were set to the same value, but in this case we could also make it configurable

cheftako · 2024-05-10T17:31:20Z

I like the change it seems valuable. Just a couple of minor questions it would be good to iron out first.

cnvergence · 2024-05-14T15:12:35Z

/retest

cnvergence · 2024-05-29T09:57:58Z

PTAL again :)

cheftako · 2024-05-29T16:21:22Z

pkg/server/server.go

@@ -417,7 +417,7 @@ func (s *ProxyServer) Proxy(stream client.ProxyService_ProxyServer) error {
 	streamUID := uuid.New().String()
 	klog.V(5).InfoS("Proxy request from client", "userAgent", userAgent, "serverID", s.serverID, "streamUID", streamUID)

-	recvCh := make(chan *client.Packet, xfrChannelSize)
+	recvCh := make(chan *client.Packet, s.xfrChannelSize)


This is the transfer channel for traffic coming from the KAS (frontend). However the options indicate that the size is only for the tunnel segment between the server and agent. We should either fix the flag description to mention both tunnel segments or add a separate configuration for each tunnel segment.

I have decided to fix the flag, as I didn't want to add another configuration flag, to keep it at the same value as it was originally set.

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

cnvergence · 2024-06-05T14:47:04Z

cmd/server/app/options/options.go

@@ -136,6 +137,7 @@ func (o *ProxyRunOptions) Flags() *pflag.FlagSet {
 	flags.StringVar(&o.AuthenticationAudience, "authentication-audience", o.AuthenticationAudience, "Expected agent's token authentication audience (used with agent-namespace, agent-service-account, kubeconfig).")
 	flags.StringVar(&o.ProxyStrategies, "proxy-strategies", o.ProxyStrategies, "The list of proxy strategies used by the server to pick an agent/tunnel, available strategies are: default, destHost, defaultRoute.")
 	flags.StringSliceVar(&o.CipherSuites, "cipher-suites", o.CipherSuites, "The comma separated list of allowed cipher suites. Has no effect on TLS1.3. Empty means allow default list.")
+	flags.IntVar(&o.XfrChannelSize, "xfr-channel-size", o.XfrChannelSize, "The size of the two KNP server channels used in server for transferring data. One channel is for data coming from the Kubernetes API Server, and the other one is for data coming from the KNP agent.")


@cheftako just let me know, how it does look for you now

cheftako · 2024-06-20T16:20:11Z

/lgtm
/approve
Thanks for making this change!

k8s-ci-robot · 2024-06-20T16:20:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, cnvergence

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

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 6, 2024

k8s-ci-robot requested review from elmiko and ipochi May 6, 2024 11:20

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 May 6, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 6, 2024

jkh52 reviewed May 9, 2024

View reviewed changes

cmd/agent/app/options/options.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned cheftako May 9, 2024

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 May 9, 2024

jkh52 reviewed May 9, 2024

View reviewed changes

cmd/server/app/options/options.go Outdated Show resolved Hide resolved

jkh52 reviewed May 9, 2024

View reviewed changes

cmd/server/app/options/options.go Outdated Show resolved Hide resolved

k8s-ci-robot assigned ipochi May 9, 2024

cheftako reviewed May 10, 2024

View reviewed changes

cmd/agent/app/options/options.go Outdated Show resolved Hide resolved

cheftako reviewed May 10, 2024

View reviewed changes

cmd/agent/app/options/options.go Outdated Show resolved Hide resolved

cheftako reviewed May 10, 2024

View reviewed changes

cnvergence requested review from cheftako and jkh52 May 14, 2024 20:39

cheftako reviewed May 29, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2024

cnvergence added 4 commits June 3, 2024 16:46

Allow configuration of transport channel size in agent and server

5a8e1fd

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

remove commented line

8ace398

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

review comments, test XfrChannelSize validation

d0471d8

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

restore int value

62e778d

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

fix description

31dffc5

Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>

cnvergence force-pushed the add-flag-channel-size branch from 67f090c to 31dffc5 Compare June 3, 2024 15:18

cnvergence requested a review from cheftako June 3, 2024 15:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2024

cnvergence commented Jun 5, 2024

View reviewed changes

cnvergence mentioned this pull request Jun 13, 2024

One apiserver can destabilize apiservers for all clusters in the given seed kubermatic/kubermatic#13321

Open

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2024

k8s-ci-robot merged commit fc590c6 into kubernetes-sigs:master Jun 20, 2024
16 checks passed

cnvergence deleted the add-flag-channel-size branch July 8, 2024 08:32

cnvergence mentioned this pull request Nov 8, 2024

Bump Konnectivity network proxy versions to match Kubernetes version kubermatic/kubermatic#13852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuration of transport channel size in agent and server #624

Allow configuration of transport channel size in agent and server #624

cnvergence commented May 6, 2024 •

edited

Loading

k8s-ci-robot commented May 6, 2024

k8s-ci-robot commented May 6, 2024

jkh52 commented May 9, 2024

jkh52 commented May 9, 2024

cheftako May 10, 2024

cnvergence May 14, 2024

cheftako commented May 10, 2024

cnvergence commented May 14, 2024

cnvergence commented May 29, 2024

cheftako May 29, 2024

cnvergence Jun 3, 2024

cnvergence Jun 5, 2024

cheftako commented Jun 20, 2024

k8s-ci-robot commented Jun 20, 2024

Allow configuration of transport channel size in agent and server #624

Allow configuration of transport channel size in agent and server #624

Conversation

cnvergence commented May 6, 2024 • edited Loading

k8s-ci-robot commented May 6, 2024

k8s-ci-robot commented May 6, 2024

jkh52 commented May 9, 2024

jkh52 commented May 9, 2024

cheftako May 10, 2024

Choose a reason for hiding this comment

cnvergence May 14, 2024

Choose a reason for hiding this comment

cheftako commented May 10, 2024

cnvergence commented May 14, 2024

cnvergence commented May 29, 2024

cheftako May 29, 2024

Choose a reason for hiding this comment

cnvergence Jun 3, 2024

Choose a reason for hiding this comment

cnvergence Jun 5, 2024

Choose a reason for hiding this comment

cheftako commented Jun 20, 2024

k8s-ci-robot commented Jun 20, 2024

cnvergence commented May 6, 2024 •

edited

Loading