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.

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

UDS + GRPC Support for Network Proxy #87179

Open
wants to merge 5 commits into
base: master
from

Conversation

@Jefftree
Copy link
Member

Jefftree commented Jan 14, 2020

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add HTTP Connect UDS and GRPC UDS support for network proxy. Also change the default configs to use UDS.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

KUBE_ENABLE_EGRESS_VIA_KONNECTIVITY_SERVICE=true ./cluster/kube-up.sh to set up.

I'm not familiar with the standards for API Review. Some structs in types.go were changed and if you have any suggestions, please lmk.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig api-machinery

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Jefftree
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot requested review from apelisse, deads2k and kubernetes/dep-approvers Jan 14, 2020
@Jefftree Jefftree force-pushed the Jefftree:netproxy-uds branch from 28a514f to d97823c Jan 14, 2020
@@ -28,14 +28,12 @@ spec:
hostPath:
path: /etc/srv/kubernetes/pki/konnectivity-agent
containers:
- image: gcr.io/google-containers/proxy-agent:v0.0.3
- image: gcr.io/jying-gke-dev/proxy-agent-amd64:842202cc3b819fc307b46e2b92bd8a4391bb9231

This comment has been minimized.

Copy link
@Jefftree

Jefftree Jan 14, 2020

Author Member

Temporary until latest image is pushed.

@@ -11,7 +11,7 @@ spec:
hostNetwork: true
containers:
- name: konnectivity-server-container
image: gcr.io/google-containers/proxy-server:v0.0.3
image: gcr.io/jying-gke-dev/proxy-server-amd64:842202cc3b819fc307b46e2b92bd8a4391bb9231

This comment has been minimized.

Copy link
@Jefftree

Jefftree Jan 14, 2020

Author Member

Temporary until latest image is pushed.

@Jefftree Jefftree changed the title UDS Support for Network Proxy UDS + GRPC Support for Network Proxy Jan 14, 2020
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 14, 2020

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@Jefftree Jefftree force-pushed the Jefftree:netproxy-uds branch from 3857915 to 4cd9fd0 Jan 14, 2020
@Jefftree

This comment has been minimized.

Copy link
Member Author

Jefftree commented Jan 14, 2020

@k8s-ci-robot k8s-ci-robot requested review from caesarxuchao and cheftako Jan 14, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 14, 2020

@Jefftree: GitHub didn't allow me to request PR reviews from the following users: dberkov, Sh4d1.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @caesarxuchao @cheftako @dberkov @Sh4d1

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.

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Jan 14, 2020

/assign @caesarxuchao

params+=("--cluster-ca-cert=${KONNECTIVITY_AGENT_CA_CERT_PATH}")
params+=("--cluster-cert=${KONNECTIVITY_AGENT_CERT_PATH}")
params+=("--cluster-key=${KONNECTIVITY_AGENT_KEY_PATH}")
params+=("--udsName=/etc/srv/kubernetes/konnectivity/konnectivity-server.socket")

This comment has been minimized.

Copy link
@dberkov

dberkov Jan 14, 2020

it might be worse to cut a PR in apiserver-network-proxy first for renaming udsName to uds-name

This comment has been minimized.

Copy link
@Jefftree

Jefftree Jan 14, 2020

Author Member

I'll wait for that PR and your token authentication PR to merge and update this.

Jefftree added 5 commits Jan 13, 2020
@Jefftree Jefftree force-pushed the Jefftree:netproxy-uds branch from 54b6b90 to c419307 Jan 16, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 16, 2020

@Jefftree: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-dependencies c419307 link /test pull-kubernetes-dependencies
pull-kubernetes-e2e-kind-ipv6 c419307 link /test pull-kubernetes-e2e-kind-ipv6
pull-kubernetes-e2e-gce c419307 link /test pull-kubernetes-e2e-gce
pull-kubernetes-kubemark-e2e-gce-big c419307 link /test pull-kubernetes-kubemark-e2e-gce-big

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Copy link
Member

caesarxuchao left a comment

Some nits.

name: konnectivity-agent
command: ["/proxy-agent"]
args: [
"--logtostderr=true",
"--ca-cert=/etc/srv/kubernetes/pki/konnectivity-agent/ca.crt",
"--agent-cert=/etc/srv/kubernetes/pki/konnectivity-agent/client.crt",
"--agent-key=/etc/srv/kubernetes/pki/konnectivity-agent/client.key",

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

Shall we keep the agent-cert and agent-key until the proxy agent can use tokens to validate itself? I think Dima will have a PR soon.

@@ -1645,7 +1641,6 @@ function start-etcd-servers {

# Replaces the variables in the konnectivity-server manifest file with the real values, and then
# copy the file to the manifest dir
# $1: value for variable "server_port"
# $2: value for variable "agent_port"

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

$2 should be $1 now?

path: /etc/srv/kubernetes/pki/konnectivity-server
- name: pkiagent
path: /etc/srv/kubernetes/pki
- name: konnectivity-home

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

This is for the uds, right? Maybe s/home/uds?

allErrs = append(allErrs, field.Invalid(
fldPath.Child("httpConnect"),
"direct",
"httpConnect config should be absent for direct connect"))

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

s/"direct"/"http-connect uds"

fldPath.Child("httpConnect"),
"direct",
"httpConnect config should be absent for direct connect"))
} else if connection.UDSName == "" {

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

Is the "else if" necessary?
If connection has non-nil HTTPConnect and empty UDSName, we should return both error in allErrs, right?

return contextDialer, nil
}

func createGRPCDialer(udsName string) (utilnet.DialFunc, error) {

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

createGrpcUDSDialer?

contextDialer := func(ctx context.Context, network, addr string) (net.Conn, error) {

dialOption := grpc.WithContextDialer(func(context.Context, string) (net.Conn, error) {
c, err := net.DialTimeout("unix", udsName, 0)

This comment has been minimized.

Copy link
@caesarxuchao

caesarxuchao Jan 17, 2020

Member

use net.Dial if we set the timeout to 0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.