Add a controller in the cluster keeping in sync routes and hosts file #1793

guillaumerose · 2020-12-22T10:21:29Z

The daemon has an API available in the VM: curl http://gateway/hosts/add or remove.
A controller is running the cluster and calls the daemon API.

For Windows, in the future, the daemon will forward request to the adminhelper daemon. Currently, a UAC prompt is shown.

openshift-ci-robot · 2020-12-22T10:21:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guillaumerose

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

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

gbraad · 2020-12-23T05:15:14Z

Makefile

@@ -88,12 +88,19 @@ $(BUILD_DIR)/macos-amd64/crc: $(SOURCES)
 $(BUILD_DIR)/linux-amd64/crc: $(SOURCES)
 	GOOS=linux GOARCH=amd64 go build -ldflags="$(LDFLAGS)" -o $(BUILD_DIR)/linux-amd64/crc ./cmd/crc

+$(BUILD_DIR)/linux-amd64/routes-controller: $(SOURCES)
+	CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -ldflags="$(LDFLAGS)" -o $(BUILD_DIR)/linux-amd64/routes-controller ./cmd/routes-controller


#1793 (comment)

./cmd/routes-controller this source code likely has to become a repo on it's own as this is part of a container.

gbraad · 2020-12-23T05:15:51Z

~~This 'routes-controller' is a separate binary?~~ What is it's function?
Please describe this more clearly in the description of the PR

~~The problem I see is that this becomes another binary we need to maintain the lifecycle of. How will this work? Who maintains this? Why can't the/a (slightly more generic) daemon handle this?~~ It seems this is part of a container run with podman

OK, so the routes-controller ends up in a container hosted in the VM... please make this part of the description. Technical details are important for a review.

The container build defintion need to be separated into their own repo. Which would also mean the routes-controller needs to be extracted.

gbraad · 2020-12-23T05:18:12Z

Makefile

 $(BUILD_DIR)/windows-amd64/crc.exe: $(SOURCES)
 	GOARCH=amd64 GOOS=windows go build -ldflags="$(LDFLAGS)" -o $(BUILD_DIR)/windows-amd64/crc.exe ./cmd/crc

 $(HOST_BUILD_DIR)/crc-embedder: $(SOURCES)
 	go build --tags="build" -ldflags="$(LDFLAGS)" -o $(HOST_BUILD_DIR)/crc-embedder ./cmd/crc-embedder

+.PHONY: routes-controller
+routes-controller: $(BUILD_DIR)/linux-amd64/routes-controller
+	podman build -t quay.io/gurose/routes-controller:v1 -f cmd/routes-controller/Dockerfile $(BUILD_DIR)/linux-amd64/


quay.io/crcont/ ?

cmd/routes-controller/Dockerfile containers need to live in their own repos as part of the security review.

cfergeau · 2021-01-06T13:37:47Z

Could the controller be running on the host since we have the crc daemon which is running in the background anyway? It seems it would be overall easier, no new binary/container, no new VM<->host API, ...

cfergeau · 2021-01-06T13:55:13Z

cmd/crc/cmd/daemon.go

@@ -159,6 +159,10 @@ func adminHelperMux() *http.ServeMux {
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			return
 		}
+		if runtime.GOOS == "windows" { // Disable it on Windows, it avois tons of UAC prompts.


'avoids' (twice)

guillaumerose · 2021-01-11T13:24:39Z

I have 4 choices in fact:

Use this controller + API
Make crc start run in the daemon so that I can know when the VM is started and kubeconfig is here. Then I can launch the controller
Add events to the network stack so that, when a new VM connects, I try to run the controller
Poll the kubeconfig file. When it appears, run the controller.

I guess the good solution is to make crc start the client of the daemon.

cfergeau · 2021-01-19T09:25:10Z

I have 4 choices in fact:

* Use this controller + API

* Make crc start run in the daemon so that I can know when the VM is started and kubeconfig is here. Then I can launch the controller

* Add events to the network stack so that, when a new VM connects, I try to run the controller

* Poll the kubeconfig file. When it appears, run the controller.

I guess the good solution is to make crc start the client of the daemon.

What I had in mind was to rework machine.Start() a bit:

        // Post-VM start
        vmState, err := host.Driver.GetState()
        if err != nil {                                                                                                                   
                return nil, errors.Wrap(err, "Error getting the state")                                                                  
        }
        if vmState != state.Running {
                return nil, errors.Wrap(err, "CodeReady Containers VM is not running")
        }
                
        instanceIP, err := getIP(host, client.useVSock())
        if err != nil {
                return nil, errors.Wrap(err, "Error getting the IP")
        }
        sshRunner, err := crcssh.CreateRunner(instanceIP, getSSHPort(client.useVSock()), crcBundleMetadata.GetSSHKeyPath(), constants.GetPrivateKeyPath(), constants.GetRsaPrivateKeyPath())
        if err != nil {
                return nil, errors.Wrap(err, "Error creating the ssh client")
        }
        defer sshRunner.Close()
        
        logging.Debug("Waiting until ssh is available")
        if err := cluster.WaitForSSH(sshRunner); err != nil {
                return nil, errors.Wrap(err, "Failed to connect to the CRC VM with SSH -- host might be unreachable")
        }
        logging.Info("CodeReady Containers VM is running")

The kubeconfig file could be copied to ~/.crc/machines/crc around that time, rather than being done later as being done now. And the host DNS configuration (addOpenShiftHosts / runPostStartForOS) could be also be done after checking the VM is up, but before waiting for SSH. In the vsock case, addOpenShiftHosts would talk to the daemon, starting it if necessary.

guillaumerose · 2021-01-22T10:45:56Z

After a long discussion, it seems that embedding this controller in the daemon is really complicated as machine.Start can be called from the crc CLI and from the daemon itself.
I propose we start with this controller in a pod and then, when crc CLI will always be a client of the daemon, move the controller to the daemon.

guillaumerose · 2021-01-25T11:36:16Z

I extracted the controller code to https://github.com/code-ready/routes-controller and modified everything to get it working on Windows.

UAC prompts only appear two times at crc start time and then each time a route is created/updated/deleted.

cc @gbraad

gbraad · 2021-01-25T11:58:13Z

So a UAC prompt is given twice on start, and each time on a route? That sounds like a lot (too much).

guillaumerose · 2021-01-25T14:24:12Z

twice on start, and each time on a route

Yes twice (clean + add the default ones) and then, when you add/update/delete a route, you get a prompt.
Compared to now, this is just one eventual prompt if you run oc new-app.

It will be 0 when the adminhelper will be a daemon.

For now:

I can disable this for Windows and let the user do it by hand,
or keep as it is.

gbraad · 2021-01-25T16:11:54Z

It will be 0 when the adminhelper will be a daemon.

That is what I am looking for. Can you outline the needed steps for this in a new issue or on the admin-helper issue?
Some estimation or timeline is appreciated. Especially to conclude if this should be merged or wait for to coincide with another possible follow-up.

guillaumerose · 2021-01-25T19:03:20Z

This is already good to take. It makes the cluster work fully. The debate for the daemon will be more intense for sure. Let's start with this.

cfergeau

Looks good, though probably best to land 'admin-helper as a daemon' first.

cfergeau · 2021-03-01T16:53:44Z

cmd/crc/cmd/daemon.go

@@ -133,6 +146,44 @@ func run(configuration *types.Configuration, endpoints []string) error {
 	return <-errCh
 }

+// This API is only exposed in the virtual network. Any process can reach it by connecting to gateway.crc.testing.
+func internalAPIMux() *http.ServeMux {


internal is very non descriptive. hostsAPIMux() since this is handling requests under the /hosts path?

I renamed to gatewayAPIMux to indicate it's only gateway.crc.testing.

cfergeau · 2021-03-01T16:54:25Z

cmd/crc/cmd/daemon.go

+			http.Error(w, err.Error(), http.StatusBadRequest)
+			return
+		}
+		if err := adminhelper.RemoveFromHostsFile(req...); err != nil {


Better to avoid the code c&p.

I don't get this. What do you mean?

The code handling /hosts/add and /hosts/remove is the same except for the line calling adminhelper.AddToHostsFile / adminhelper.RemoveFromHostsFile, dunno if it's possible to factor this somehow without making the code too complicated?

Yes good point, done

cmd/crc/cmd/daemon.go

cfergeau · 2021-03-19T14:32:59Z

pkg/crc/machine/start.go

+			Containers: []v1.Container{
+				{
+					Name:            "routes-controller",
+					Image:           "quay.io/crcont/routes-controller:6effc2f0304d3a11c1e22c278b10630e834d0220",


one more container image we need to remember to maintain :)

pkg/crc/adminhelper/hosts.go

cfergeau · 2021-03-19T14:40:28Z

pkg/crc/adminhelper/hosts_windows.go

@@ -6,7 +6,12 @@ import (
 	"github.com/code-ready/crc/pkg/os/windows/powershell"
 )

-func execute(args ...string) error {
+func executePrivileged(args ...string) error {
 	_, _, err := powershell.ExecuteAsAdmin("modifying hosts file", strings.Join(append([]string{goodhostPath}, args...), " "))
 	return err
 }


Probably better to land first the admin-helper daemon work?

admin-helper daemon still require some work: a preflight to install it, define the protocol, etc.

With this PR, we solve it for macOS and Linux. For Windows, the experience is not too bad. There is just one UAC prompt per new route.

This network stack could be GA on macOS/Linux after this PR (+ some minor enhancements, better errors, etc.).
I think we have the feature parity with the traditional network stack.

This network stack could be GA on macOS/Linux after this PR (+ some minor enhancements, better errors, etc.).
I think we have the feature parity with the traditional network stack.

daemon autostart is missing on linux (and on macos setups without the tray running). And if I'm not mistaken this also does not handle routes using non-standard ports? I guess autostart falls under the 'minor enhancements category')? I don't mind the non-standard thing for now, I agree we can push for wider testing after this.

does not handle routes using non-standard ports

I tried to find examples or code around that but I couldn't find it. The only route I see like this would be the one for Kubernetes API.
Router container only exposes port 80 and 443, not others. I don't really understand how an other port can be used.

I thought our own doc had such examples ( https://code-ready.github.io/crc/#deploying-sample-application-with-odo_gsg ) but after being deployed, the app is exposed on port 80/443, not on 8080 as I expected.
https://docs.okd.io/latest/networking/configuring_ingress_cluster_traffic/configuring-ingress-cluster-traffic-load-balancer.html may be related to this, but is not working with the default networking anyway.

pkg/crc/services/dns/dns.go

guillaumerose · 2021-04-15T07:54:55Z

/retest

guillaumerose · 2021-04-15T09:46:27Z

/retest

guillaumerose · 2021-04-15T12:40:55Z

/retest

guillaumerose · 2021-04-16T05:39:49Z

/retest

cfergeau

Still a few minor comments, but overall looks good to me.

cfergeau · 2021-04-16T11:34:32Z

pkg/crc/adminhelper/hosts.go

-	return execute(append([]string{"add", instanceIP}, hostnames...)...)
+	var filtered []string
+	for _, hostname := range hostnames {
+		stdout, err := execute("contains", instanceIP, hostname)


Have you considered using github.com/code-ready/admin-helper/pkg/hosts directly? contains implementation is:

func contains(args []string) error { hosts, err := hosts.New() if err != nil { return err } if hosts.Contains(args[0], args[1]) { fmt.Println("true") } else { fmt.Println("false") } return nil }

which is the same amount of code, and would be more efficient.

Yes done. I wanted to avoid to vendor admin-helper in crc. We just need to remind to bump it in 2 places.

pkg/crc/adminhelper/hosts.go

cmd/crc/cmd/daemon.go

praveenkumar · 2021-04-19T06:24:27Z

pkg/crc/machine/start.go

+	if err != nil {
+		return err
+	}
+	if err := sshRunner.CopyData(bin, "/tmp/routes-controller.json", 0444); err != nil {


@guillaumerose this is only pod definition so if somehow this pod crash then this route stuff will break, should we have deployment as Kind instead pod so that pod recreation happen if pod crashes?

Deployment are useful version management. For crash, there is the RestartPolicy on the pod directly. It is "Always" by default so we should be good.

I tried to kill the process by hand. It is restarted.
I tried to start/stop/start the VM. The process is here.

@guillaumerose For me it is not happening.

13:07 $ oc get pods -n openshift-ingress NAME READY STATUS RESTARTS AGE router-default-7476bbd498-g9r7k 1/1 Running 1 13d routes-controller 1/1 Running 0 30m 13:08 $ oc delete pod routes-controller -n openshift-ingress pod "routes-controller" deleted 13:11 $ oc get pods -n openshift-ingress NAME READY STATUS RESTARTS AGE router-default-7476bbd498-g9r7k 1/1 Running 1 13d

Yes sure, I mean kill with sudo kill -9 pid.
If the user does this, why bother him and recreate the pod ?

Since it is part of openshift-ingress and if a user just delete all the pods under this namespace, all pods except this one come up without any issue, Also can we make it as static pod if we really want it to be running always?

I tried to add in static pod directory but it doesn't work:

Apr 19 12:19:17 crc-xl2km-master-0 hyperkube[2154]: E0419 12:19:17.473167 2154 kubelet.go:1671] Failed creating a mirror pod for "routes-controller-crc-xl2km-master-0_openshift-ingress(fd89789fbe33ba7075234cb104ec8e39)": pods "routes-controller-crc-xl2km-master-0" is forbidden: a mirror pod may not reference service accounts

if a user just delete all the pods under this namespace

It should not happen or he really wants to do this?

This is not blocker for this PR so let's have some follow up issue in case we hit it.

A controller running in the VM will be able to add and remote hosts file entries. The API is reachable within the virtual network only at http://gateway.crc.testing.

It will keep hosts file in sync with routes. It talks to the http server running at gateway.crc.testing:80.

These routes are the default routes. They are in every OpenShift cluster.

Hosts file on Windows also contains crc entries. crc cleanup should remove them.

Each time the routes controller reconciles, a UAC prompt appear because admin-helper need to check if the entry is present or not. With this new unprivileged check, the entry is added only if needed. This saves couple of UAC prompts on Windows.

vendor/github.com/dimchansky/utfbom/.travis.yml is breaking this script. git is replacing automatically CRLF by LF and this script cacthes the difference.

guillaumerose · 2021-04-19T12:24:29Z

retest this please

praveenkumar · 2021-04-20T12:19:05Z

/lgtm

The feature was added in crc by crc-org/crc#1793 The image digest needs to be sync between both projects.

The feature was added in crc by crc-org/crc#1793 The image digest needs to be sync between both projects. https://github.com/code-ready/routes-controller/releases/tag/v0.0.1

openshift-ci-robot requested review from cfergeau and praveenkumar December 22, 2020 10:21

openshift-ci-robot added the approved label Dec 22, 2020

guillaumerose force-pushed the routescontroller branch from 63f2810 to dc6742d Compare December 22, 2020 12:51

gbraad reviewed Dec 23, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase label Dec 25, 2020

cfergeau reviewed Jan 6, 2021

View reviewed changes

guillaumerose force-pushed the routescontroller branch from dc6742d to 44621cd Compare January 25, 2021 10:01

openshift-ci-robot removed the needs-rebase label Jan 25, 2021

cfergeau reviewed Mar 19, 2021

View reviewed changes

gbraad mentioned this pull request Apr 9, 2021

[BUG] OCP Routes don't work using userland network stack #2193

Closed

openshift-ci-robot added the needs-rebase label Apr 11, 2021

openshift-ci bot removed the needs-rebase label Apr 14, 2021

guillaumerose force-pushed the routescontroller branch 2 times, most recently from c4dc2f6 to aa4047c Compare April 14, 2021 19:21

cfergeau reviewed Apr 16, 2021

View reviewed changes

guillaumerose mentioned this pull request Apr 19, 2021

Limit the number and size of entries crc-org/admin-helper#14

Open

praveenkumar reviewed Apr 19, 2021

View reviewed changes

guillaumerose added 6 commits April 19, 2021 08:49

Expose the hosts file API inside the VM

8169dbb

A controller running in the VM will be able to add and remote hosts file entries. The API is reachable within the virtual network only at http://gateway.crc.testing.

Add routes controller pod to the cluster

fe399ea

It will keep hosts file in sync with routes. It talks to the http server running at gateway.crc.testing:80.

Add missing routes in the hosts file

0b59c1b

These routes are the default routes. They are in every OpenShift cluster.

cleanup: also clean hosts file on Windows

2f1807b

Hosts file on Windows also contains crc entries. crc cleanup should remove them.

Refactor to directly pass arguments to 'execute' without an extra array

cff500d

guillaumerose force-pushed the routescontroller branch from aa4047c to cff500d Compare April 19, 2021 07:07

vendorcheck: ignore CRLF/LF differences

da28a0b

vendor/github.com/dimchansky/utfbom/.travis.yml is breaking this script. git is replacing automatically CRLF by LF and this script cacthes the difference.

openshift-ci-robot assigned praveenkumar Apr 20, 2021

openshift-ci-robot added the lgtm label Apr 20, 2021

guillaumerose merged commit 088d645 into crc-org:master Apr 20, 2021

guillaumerose added a commit to guillaumerose/snc that referenced this pull request Apr 21, 2021

Preload routes controller image

97e07e7

The feature was added in crc by crc-org/crc#1793 The image digest needs to be sync between both projects.

guillaumerose mentioned this pull request Apr 21, 2021

Preload routes controller image crc-org/snc#398

Merged

Add a controller in the cluster keeping in sync routes and hosts file #1793

Add a controller in the cluster keeping in sync routes and hosts file #1793

Conversation

guillaumerose commented Dec 22, 2020 • edited Loading

openshift-ci-robot commented Dec 22, 2020

gbraad Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau commented Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

guillaumerose commented Jan 11, 2021

cfergeau commented Jan 19, 2021

guillaumerose commented Jan 22, 2021

guillaumerose commented Jan 25, 2021

gbraad commented Jan 25, 2021

guillaumerose commented Jan 25, 2021

gbraad commented Jan 25, 2021

guillaumerose commented Jan 25, 2021

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumerose commented Apr 15, 2021

guillaumerose commented Apr 15, 2021

guillaumerose commented Apr 15, 2021

guillaumerose commented Apr 16, 2021

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaumerose commented Apr 19, 2021

praveenkumar commented Apr 20, 2021

guillaumerose commented Dec 22, 2020 •

edited

Loading

gbraad Dec 23, 2020 •

edited

Loading

gbraad commented Dec 23, 2020 •

edited

Loading

cfergeau commented Jan 6, 2021 •

edited

Loading