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

kubeproxy operator #35

Merged
merged 14 commits into from
Jul 2, 2020
Merged

Conversation

johnsonj
Copy link
Contributor

@johnsonj johnsonj commented Nov 18, 2019

TODO:

  • boilerplate
  • operator can run external to the cluster
  • make the operator run in the cluster
  • golden file tests

w/ kubebuilder @ 5f27a7241d:

export KUBEBUILDER_ENABLE_PLUGINS=1
kubebuilder init --fetch-deps=false --domain=x-k8s.io --license=apache2
kubebuilder create api --pattern addon --controller=true --example=false --group=addons --kind=KubeProxy --make=false --namespaced=true --resource=true --version=v1alpha
This changes implements the basic operator functionality that can
bootstrap a new cluster with kubeproxy. The operator currently
runs external to the cluster (on your local machine). For an example,
see the ./create-cluster.sh script that uses kinder.

The change adds 'ClusterCIDR' to the KubeProxy object. It would be
interesting to see if we can detect this value when running within the
cluster.
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 18, 2019
@timothysc timothysc removed their request for review December 10, 2019 20:23
@timothysc
Copy link
Member

cc @neolit123

Status KubeProxyStatus `json:"status,omitempty"`
}

var _ addonv1alpha1.CommonObject = &KubeProxy{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to comment to WIP PR.
Is it better to add var _ addonv1alpha1.Patchable = &KubeProxy{} ?
Because of KubeProxy has PatchSpec interface.
As like here.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 24, 2020
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 29, 2020
@neolit123
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Apr 29, 2020
@johnsonj
Copy link
Contributor Author

/remove-lifecycle frozen
/retitle kubeproxy operator

@k8s-ci-robot k8s-ci-robot removed the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Jun 10, 2020
@k8s-ci-robot k8s-ci-robot changed the title WIP: kubeproxy-operator kubeproxy operator Jun 10, 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 10, 2020
@johnsonj
Copy link
Contributor Author

Thanks to @somtochiama this is 'feature complete'! There's follow up work (eg clean up go.mod, update to latest controller-runtime, integrate with prow tests in this repo) but I think that work should be done in this repo and not this PR.

@neolit123
Copy link
Member

hello,

this is 'feature complete'!

is there a roadmap for this operator?
ideally long term we would like to use it a number of SIG Cluster Lifecycle tools including kubeadm, cluster-api.

cc @rosti @fabriziopandini

@johnsonj
Copy link
Contributor Author

@neolit123 - there is no identified roadmap. This is just establishing that things work. There's definitely work to do to make this ready for kubeadm/cluster-api.

We can go through the same exercise as coredns operator: proposal, discussion in the cluster addons meeting minutes, then action (#40)! We do need an owner though. (not sure if @somtochiama is considering this?)

@neolit123
Copy link
Member

thanks for the details @johnsonj we just spoke with @rajansandeep and i proposed that he joins the kubeadm office hours so that we can discuss the coredns operator. arguably it is more urgent than the kube-proxy operator, so perhaps we can establish a pattern from it.

@johnsonj
Copy link
Contributor Author

sounds great @neolit123 !

@somtochiama
Copy link
Member

We can go through the same exercise as coredns operator: proposal, discussion in the cluster addons meeting minutes, then action (#40)! We do need an owner though. (not sure if @somtochiama is considering this?)

I am definitely interested

@johnsonj
Copy link
Contributor Author

/assign @stealthybox

RUN curl -fsSL https://dl.k8s.io/release/v1.17.4/bin/linux/amd64/kubectl > /usr/bin/kubectl
RUN chmod a+rx /usr/bin/kubectl
# Build the manager binary
FROM golang:1.12.5 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

We can update to use golang 1.13

kubeproxy/go.mod Outdated
@@ -0,0 +1,13 @@
module addon-operators/kubeproxy

go 1.12
Copy link
Contributor

Choose a reason for hiding this comment

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

We can update to use golang 1.13

@@ -0,0 +1,40 @@
#!/bin/sh
Copy link
Contributor

@rajansandeep rajansandeep Jun 11, 2020

Choose a reason for hiding this comment

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

This seems like a really useful script that can be used to establish e2e/smoketests for all operators.
Should this be moved outside to a test/or hack/ dir?
#63 might give some more context

Copy link
Member

Choose a reason for hiding this comment

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

The script is for creating a kinder cluster without kubeproxy. I could make a PR for one that creates a normal kind or kinder cluster with kube-proxy already installed. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

The script is for creating a kinder cluster without kubeproxy.

Yes, this is exactly what's desired. My thinking is once this script creates the cluster, the existing smoketest.go can run the tests on it.

Copy link
Member

Choose a reason for hiding this comment

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

The script is for creating a kinder cluster without kubeproxy.

Yes, this is exactly what's desired. My thinking is once this script creates the cluster, the existing smoketest.go can run the tests on it.

Oooh, I see what you mean. @johnsonj wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally makes sense to me. We'll always need our own kube-up.sh, even in a world where these operators are default we need a way to test with our versions!

Now the question I'd pose is: Copy the shell script, or reimplement it in Go?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the shell script should suffice

Copy link
Contributor

@rajansandeep rajansandeep left a comment

Choose a reason for hiding this comment

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

I have a couple of nits that caught my eye.

Copy link

@rosti rosti left a comment

Choose a reason for hiding this comment

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

Thanks @johnsonj !
I guess that the only big concern for me here is the reliance on a deprecated feature (kube-proxy's command line flags) and the lack of component config for kube-proxy itself.

}
}

func injectFlags(ctx context.Context, object declarative.DeclarativeObject, s string) (string, error) {
Copy link

Choose a reason for hiding this comment

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

Almost all command line flags of kube-proxy are now deprecated (see here). Hence, I would advocate for the use of the kube-proxy component config here.

cc @mtaufen @stealthybox

port = "443"
}
env := map[string]string{"KUBERNETES_SERVICE_HOST": master, "KUBERNETES_SERVICE_PORT": port}
if err := SetEnvironmentVariables(o, env); err != nil {
Copy link

Choose a reason for hiding this comment

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

I don't think that this is the best way for a long term solution. I would recommend having a field in KubeProxySpec that explicitly sets that value. But it's fine to use this approach as long as the value in the CRD is not set.

## Running in a cluster

1. Create a kinder cluster
Ensure kinder is installed. [Installation docs](https://github.com/kubernetes/kubeadm/blob/master/kinder/README.md)
Copy link
Member

Choose a reason for hiding this comment

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

just a FYI, from the kinder docs:

The exported logic in kinder packages can be a subject of change at any point in time, thus using kinder as a library is unsupported.

this message is rather incomplete.

one caveat is that kinder has no support guarantees for usage external to the kubeadm e2e tests even for its CLI. this means that if we have to change something in kinder we would likely do it without a deprecation policy and me might break consumers.

but overall using it for testing the operators seems fine and if one day kubeadm uses the operators we would likely run tests on such a setup using kinder.

@justinsb
Copy link
Contributor

justinsb commented Jul 2, 2020

Looking at this PR, it sounds like there is generally agreement on the idea, with a desire to make additional changes (moving to componentconfig, splitting out e2e, etc), and it looks like there are several people making those changes sort of concurrently, including PRs to the PR.

As such, I think it's best to merge and then we can all make any additional changes as additional PRs (it is new code, so we're not breaking anyone!)

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnsonj, justinsb

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 merged commit e190793 into kubernetes-sigs:master Jul 2, 2020
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet