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

Adding kube-proxy-replacement support in cilium #6334

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

MrFreezeex
Copy link
Member

@MrFreezeex MrFreezeex commented Jun 28, 2020

Signed-off-by: Arthur Outhenin-Chalandre arthur@cri.epita.fr

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add kube-proxy-replacement support for a kube proxy free deployment with cilium https://docs.cilium.io/en/v1.8/gettingstarted/kubeproxy-free/

It still needs #5554 to fix various kube_proxy_remove bugs...

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add new variable cilium_kube_proxy_replacement

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 28, 2020
@k8s-ci-robot k8s-ci-robot requested review from EppO and floryut June 28, 2020 16:51
@k8s-ci-robot 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 Jun 28, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @MrFreezeex. 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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 28, 2020
@MrFreezeex MrFreezeex changed the title Adding kube-proxy-replacement support in cilium [WIP] Adding kube-proxy-replacement support in cilium Jun 28, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2020
@floryut
Copy link
Member

floryut commented Jun 28, 2020

/ok-to-test

@k8s-ci-robot 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 Jun 28, 2020
@MrFreezeex MrFreezeex changed the title [WIP] Adding kube-proxy-replacement support in cilium Adding kube-proxy-replacement support in cilium Jun 28, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2020
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jun 28, 2020

I have tested this PR with #5554 in strict mode and I have a working cluster without kube-proxy 🎉.

.gitlab-ci/packet.yml Outdated Show resolved Hide resolved
@MatthiasLohr
Copy link

Is there anything one can help to speed up things here?

@floryut
Copy link
Member

floryut commented Jul 13, 2020

Is there anything one can help to speed up things here?

as #5554 is now merged, could you rebase and we can see if the test succeed ?

@MrFreezeex
Copy link
Member Author

I rebased !

Side note during my test I had issues on ubuntu 18 because of the old kernel version. If this fails again It's probably because of that, but let's see how It goes...

@MrFreezeex
Copy link
Member Author

The test did failed... Probably due to the same issue I had. I changed the base distro to debian10 so It should works in the CI!

@MrFreezeex MrFreezeex force-pushed the cilium-svc-proxy branch 6 times, most recently from 8a726d3 to a9309ff Compare July 13, 2020 23:39
Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jul 14, 2020

The CI job for cilium in strict mode has succeeded https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/638214670 ! This PR should be ready to be reviewed :).

@mmack
Copy link

mmack commented Jul 29, 2020

Would like to see this merged... Can I help somehow? Test anything?

@floryut
Copy link
Member

floryut commented Jul 29, 2020

Would like to see this merged... Can I help somehow? Test anything?

There was a CI issue and the PR drop out of sight I guess.. But I restarted jobs this afternoon and was ready to lgtm :)
I'll take a look tomorrow and will try to put this in master

@floryut
Copy link
Member

floryut commented Jul 30, 2020

Well seems fine to me, agreed with the manual part as Cilium job is manual.
/lgtm

All yours @Miouge1

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2020
@Miouge1
Copy link
Contributor

Miouge1 commented Jul 30, 2020

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miouge1, MrFreezeex

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2020
@k8s-ci-robot k8s-ci-robot merged commit 3550e3c into kubernetes-sigs:master Jul 30, 2020
@mmack
Copy link

mmack commented Jul 30, 2020

ok guys, i've tested this and i think we need to set the k8s api host and port at the cilium agent's:

        - name: KUBERNETES_SERVICE_HOST
          valueFrom:
            configMapKeyRef:
              key: k8sServiceHost
              name: cilium-config
              optional: true
        - name: KUBERNETES_SERVICE_PORT
          valueFrom:
            configMapKeyRef:
              key: k8sServicePort
              name: cilium-config
              optional: true

and in the vars:

k8sServiceHost: real_node_ip or apiserver LB host
k8sServicePort: 443

because the "normal" k8s api service is not reachable without a kube-proxy beforehand....
Also see this docs: https://docs.cilium.io/en/v1.8/gettingstarted/kubeproxy-free/

@MrFreezeex
Copy link
Member Author

MrFreezeex commented Jul 30, 2020

ok guys, i've tested this and i think we need to set the k8s api host and port at the cilium agent's:

        - name: KUBERNETES_SERVICE_HOST
          valueFrom:
            configMapKeyRef:
              key: k8sServiceHost
              name: cilium-config
              optional: true
        - name: KUBERNETES_SERVICE_PORT
          valueFrom:
            configMapKeyRef:
              key: k8sServicePort
              name: cilium-config
              optional: true

and in the vars:

k8sServiceHost: real_node_ip or apiserver LB host
k8sServicePort: 443

because the "normal" k8s api service is not reachable without a kube-proxy beforehand....
Also see this docs: https://docs.cilium.io/en/v1.8/gettingstarted/kubeproxy-free/

I also saw this in the docs but I didn't encouter any problem testing in strict mode without these variables. Also the test case added in this PR pass with an healthy cluster. Do you have an example of a setup in which a kube proxy less cilium deployment is not working ?

@MrFreezeex MrFreezeex deleted the cilium-svc-proxy branch July 30, 2020 11:31
@mmack
Copy link

mmack commented Jul 30, 2020

I guess it's not a problem with a running cluster b/c kube-proxy created the svc already.

But spawn a completely new cluster, then it won't work.

@MrFreezeex
Copy link
Member Author

I guess it's not a problem with a running cluster b/c kube-proxy created the svc already.

But spawn a completely new cluster, then it won't work.

I did it with a new cluster though and the test is already doing that... What is your test setup ?

@mmack
Copy link

mmack commented Jul 30, 2020

bare metal, ubuntu 20. wireguard tunnel between nodes, therefore ip = $internal_ip and ansible_host: $externalip.

loadbalancer_apiserver is set to a ip that balances to 2 etcd nodes which run the apiserver.

It worked all with kube-proxy and cilium.

@MrFreezeex
Copy link
Member Author

bare metal, ubuntu 20. wireguard tunnel between nodes, therefore ip = $internal_ip and ansible_host: $externalip.

loadbalancer_apiserver is set to a ip that balances to 2 etcd nodes which run the apiserver.

It worked all with kube-proxy and cilium.

Trying to replicate and fix in #6473.

LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Sep 17, 2020
Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants