Initial CNI installer & plugin #1304

curtbushko · 2022-06-24T02:58:01Z

This PR get the initial structure of the installer and plugin in place

Changes proposed in this PR:

The installer is a subcommand under ./control-plane/subcommand/install-cni
The plugin is a standalone binary (it has to be) under ./control-plane/cni
There are changes to the Dockerfile, Makefiles, .circleci and scripts under ./control-plane/build-support in order to get the cni plugin building alongside the control-plane binary.
The installer is working in my manual tests but it needs more tests and some more logic. I have added bundles of TODOs in places that I know that I need to change.
The plugin is working but it does nothing at this time. It also contains some TODOs.

Helm chart, more unit tests, more logic, acceptance tests are to come in followup PRs.

How I've tested this PR:

Manual testing in my own KinD clusters. It requires some setup. Please wait until I have the helm chart and acceptance test to run things.

How I expect reviewers to test this PR:

👀

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

curtbushko · 2022-06-24T02:58:59Z

.circleci/config.yml

      # save dev build to CircleCI
      - store_artifacts:
          path: ./control-plane/pkg/bin
+      - store_artifacts:
+          path: ./control-plane/cni/pkg/bin


These needed to be added so that the docker image would build

curtbushko · 2022-06-24T03:00:56Z

control-plane/Dockerfile

@@ -42,6 +43,7 @@ RUN addgroup ${BIN_NAME} && \
    adduser -S -G ${BIN_NAME} 100

 COPY pkg/bin/linux_${ARCH}/${BIN_NAME} /bin
+COPY cni/pkg/bin/linux_${ARCH}/${CNI_BIN_NAME} /bin


The plugin is under ./control-plane/cni but the binary ends up under that directory. I didn't want to mess with build-local.sh too much and move the plugin binary around, to say ./control-plane/bin, so I left the binary in the cni directory.

curtbushko · 2022-06-24T03:07:45Z

control-plane/cni/config/config.go

+	Kubeconfig string `json:"kubeconfig" mapstructure:"kubeconfig"`
+	// LogLevl is the logging level. Can be set as a cli flag.
+	LogLevel string `json:"log_level" mapstructure:"log_level"`
+}


This struct is used by both the installer and plugin. It is a representation of the config file that is written to the host.

curtbushko · 2022-06-24T03:24:57Z

control-plane/cni/go.mod

+)
+
+replace github.com/hashicorp/consul/sdk v0.9.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50
+


I did a trick here where I copied in the go.mod/go.sum from control-plane, added the libcni plugin and ran go mod tidy against it hoping that the package versions would lineup between control-plane and cni. I was hoping that it might speed up the builds.

curtbushko · 2022-06-24T03:28:06Z

control-plane/cni/main.go

+)
+
+const (
+	keyCNIStatus    = "consul.hashicorp.com/cni-status"


Should these be moved to where the other annotations under ./control-plane/connect-inject/annotations.go?

(I kind of didn't want to pull in all of the control-plane package and its dependencies if I didn't need it...)

I agree with not pulling in the deps

curtbushko · 2022-06-24T03:29:41Z

control-plane/cni/main.go

+	// Kubeconfig file name. Can be set as a cli flag.
+	Kubeconfig string `json:"kubeconfig"`
+	// LogLevl is the logging level. Can be set as a cli flag.
+	LogLevel string `json:"log_level"`


You will notice that enabled is not in this list of variables. enabled is only used in the helm install and is not written to the cni config file. The installer and plugin do not need the enabled variable for anything.

curtbushko · 2022-06-24T03:31:04Z

control-plane/cni/main.go

+}
+
+// parseConfig parses the supplied configuration (and prevResult) from stdin.
+func parseConfig(stdin []byte) (*PluginConf, error) {


This is standard libncni code and is part of their sample. Libcni does a lot of the work for you when it comes to dealing with all of the structure around CNI plugins. Parsing the inputs being one of them...

curtbushko · 2022-06-24T03:35:04Z

control-plane/cni/main.go

+	if podNamespace == "" && podName == "" {
+		return fmt.Errorf("not running in a pod, namespace and pod should have values")
+	}
+


This logging goes to stdout but that is stdout on the node itself. Calico logs the output to /var/log on the filesystem and we might want to do something similar. I am worried about log rotation and filling up the file system of the node though...

I have seen Multus get messages into the pod description and I wonder if it is possible to send logs to the pod logs. We do have access to the kube api. I think it would be a neat little feature and would standout as a nice usability feature.

curtbushko · 2022-06-24T03:38:49Z

control-plane/cni/main.go

+	})
+
+	// Check to see if the plugin is a chained plugin
+	if cfg.PrevResult == nil {


Chained plugins pass along their result from their runs. We are not doing anything that needs passing on so we just forward on the previous result. We are designed to be the final plugin in the chain but 🤷.

Tidbit: Kindnet, KinD's CNI plugin, is not a proper plugin and it does not pass along its previous result. This is why we can not run our consul-cni plugin directly on KinD... it would just throw errors. It took me a while to figure this out as I thought for a while that the problem was with my plugin... For KinD we need to disable Kindnet and install another plugin before ours, like Calico.

I think it might be valuable to change the error and/or the comments to reflect what you said about "We are designed to be the final plugin in the chain."

I added 'final' in the message.

curtbushko · 2022-06-24T03:41:12Z

control-plane/cni/main.go

+	result := prevResult
+	logger.Debug("consul-cni previous result", "result", result)
+
+	ctx := context.Background()


In the installer we are generating a kubeconfig file. What is here right now is to prove out that we can generate a proper kubeconfig file and use that kubeconfig to communicate with the kubernetes api. Setting an annotation is easy proof...

curtbushko · 2022-06-24T03:42:52Z

control-plane/go.mod

@@ -46,6 +46,7 @@ require (
 	github.com/beorn7/perks v1.0.1 // indirect
 	github.com/bgentry/speakeasy v0.1.0 // indirect
 	github.com/cespare/xxhash/v2 v2.1.1 // indirect
+	github.com/containernetworking/cni v1.1.1 // indirect


Libcni and our cni package (for shared config struct). Libcni has a couple extra dependencies like ginko...

curtbushko · 2022-06-24T03:47:56Z

control-plane/subcommand/install-cni/command.go

+// TODO: Add description that explains the difference between CNIConfig and installConfig
+
+// installConfig are the values by the installer when running inside a container.
+type installConfig struct {


installConfig is used because the installer is running inside a container and it is doing things to the host node. With the daemonset, the host system will be mounted into the container as /host/opt/cni/bin or /host/etc/cni/net.d.

ishustava · 2022-06-27T15:25:37Z

@curtbushko could you provide testing instructions? Since we don't have acceptance tests yet, I think we'd need to manually test it before we merge into main.

curtbushko · 2022-06-27T15:29:17Z

@ishustava That is my mistake. It is supposed to go into the cni feature branch. I have set it to merge into cni.

The manual testing is very tedious which is why I will do the helm chart (w/bats) plus acceptance tests before I force anyone to run anything.

lkysow

Looking good! I think the structure is there, I'm not sure how much refactoring we should do at this point though because I think the code will need to change a decent amount with making it a long-running command and with actually running the iptables stuff so feel free to disregard comments that won't be applicable later.

I'll leave it unapproved for now to see your responses and then can approve knowing that not everything will be addressed.

control-plane/subcommand/install-cni/testdata/ZZZ-consul-cni-kubeconfig.golden

lkysow · 2022-06-28T20:18:49Z

control-plane/subcommand/install-cni/command.go

+)
+
+const (
+	defaultName            = "consul-cni"


default name of what?

It is the name of the plugin and the name that get placed in the config file. I supposed this should probably go into ./control-plane/cni/config.go since that is where the config struct is.

okay then maybe call this defaultPluginName?

although is it a default or just the actual name

It is the actual name, nobody can change it.

so maybe pluginName?

control-plane/subcommand/install-cni/command.go

lkysow · 2022-06-28T22:03:41Z

control-plane/cni/main.go

+)
+
+const (
+	keyCNIStatus    = "consul.hashicorp.com/cni-status"


I agree with not pulling in the deps

control-plane/cni/main.go

control-plane/subcommand/install-cni/kubeconfig.go

lkysow · 2022-06-29T21:24:19Z

control-plane/subcommand/install-cni/testdata/ZZZ-consul-cni-kubeconfig.golden

+clusters:
+- cluster:
+    certificate-authority: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0
+    server: https://172.30.0.1:443


so should this have https://[172.30.0.1]:443?

I think it works either way. I have added a todo to double check when I am writing acceptance tests.

If other folks are using [] then I think safer to keep it like that.

Done. Added to the tests and code.

jmurret

Looks good so far. I had no real changes. Just a couple of comments.

jmurret · 2022-06-29T22:47:53Z

control-plane/cni/main.go

+	})
+
+	// Check to see if the plugin is a chained plugin
+	if cfg.PrevResult == nil {


I think it might be valuable to change the error and/or the comments to reflect what you said about "We are designed to be the final plugin in the chain."

jmurret · 2022-06-29T22:55:08Z

control-plane/subcommand/install-cni/command.go

+		LogLevel:   c.flagLogLevel,
+	}
+
+	// TODO: Validate config, especially log level


besides checking for nil on flagCNIBinDir, flagCNINetDir, and flagCNIBinSourceDir, I would also validate that they being with a / since installConfig struct instantiation relies on it several lines down.

This is a good call! I will added it to my validation todo.

Get structure in place and CNI installer & plugin building

… plugin error to plugin

* Get structure in place and CNI installer & plugin building

curtbushko commented Jun 24, 2022

View reviewed changes

curtbushko requested review from ishustava, a team and jmurret and removed request for a team June 24, 2022 03:57

curtbushko force-pushed the curtbushko/cni-initial branch 2 times, most recently from 12c6a30 to 515982e Compare June 27, 2022 14:35

curtbushko changed the base branch from main to cni June 27, 2022 15:29

lkysow reviewed Jun 28, 2022

View reviewed changes

curtbushko force-pushed the curtbushko/cni-initial branch 2 times, most recently from 627a381 to 905d2f0 Compare June 28, 2022 22:24

curtbushko requested a review from lkysow June 29, 2022 21:16

lkysow reviewed Jun 29, 2022

View reviewed changes

control-plane/subcommand/install-cni/kubeconfig.go Show resolved Hide resolved

lkysow reviewed Jun 29, 2022

View reviewed changes

jmurret approved these changes Jun 29, 2022

View reviewed changes

curtbushko requested a review from lkysow June 30, 2022 14:06

lkysow approved these changes Jun 30, 2022

View reviewed changes

curtbushko added 8 commits June 30, 2022 22:51

Get structure in place and CNI installer & plugin building

4ed61c3

Get structure in place and CNI installer & plugin building

Addressing PR comments

758cdd3

clean up appendCNIConfig and add some comments to make it clearer

ef8b739

use constant for file name and refactor kubeconfig to use structs

a5604f8

missed some dots from linting

4b1f49a

Add brackets to kubeconfig, update kubeconfig test and add more final…

093afd1

… plugin error to plugin

change defaultName/defaultType to pluginName/pluginType for clarity

2448a45

differnence between go 1.18 and 1.17?

13c91a9

curtbushko force-pushed the curtbushko/cni-initial branch from 82bfd14 to 13c91a9 Compare July 1, 2022 02:52

curtbushko merged commit 725cadf into cni Jul 1, 2022

curtbushko deleted the curtbushko/cni-initial branch July 1, 2022 04:51

curtbushko added a commit that referenced this pull request Jul 12, 2022

Initial CNI installer & plugin (#1304)

53560ba

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Jul 14, 2022

Initial CNI installer & plugin (#1304)

a04f49b

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Jul 15, 2022

Initial CNI installer & plugin (#1304)

8735c40

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Jul 25, 2022

Initial CNI installer & plugin (#1304)

7edca35

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Jul 25, 2022

Initial CNI installer & plugin (#1304)

50a9fb3

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 19, 2022

Initial CNI installer & plugin (#1304)

4923503

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 19, 2022

Initial CNI installer & plugin (#1304)

ed29361

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 23, 2022

Initial CNI installer & plugin (#1304)

e6cb439

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 23, 2022

Initial CNI installer & plugin (#1304)

587388d

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 24, 2022

Initial CNI installer & plugin (#1304)

bb3ceac

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 24, 2022

Initial CNI installer & plugin (#1304)

8aa0e97

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 24, 2022

Initial CNI installer & plugin (#1304)

595c49c

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 24, 2022

Initial CNI installer & plugin (#1304)

71c2e5e

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 25, 2022

Initial CNI installer & plugin (#1304)

80abe16

* Get structure in place and CNI installer & plugin building

curtbushko added a commit that referenced this pull request Aug 26, 2022

Initial CNI installer & plugin (#1304)

6e75d77

* Get structure in place and CNI installer & plugin building

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial CNI installer & plugin #1304

Initial CNI installer & plugin #1304

curtbushko commented Jun 24, 2022 •

edited

Loading

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022 •

edited

Loading

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

lkysow Jun 28, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

jmurret Jun 29, 2022

curtbushko Jun 30, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

curtbushko Jun 24, 2022

ishustava commented Jun 27, 2022

curtbushko commented Jun 27, 2022 •

edited

Loading

lkysow left a comment

lkysow Jun 28, 2022

curtbushko Jun 29, 2022

lkysow Jun 29, 2022

lkysow Jun 29, 2022

curtbushko Jun 30, 2022

lkysow Jun 30, 2022

lkysow Jun 28, 2022

lkysow Jun 29, 2022

curtbushko Jun 29, 2022

lkysow Jun 29, 2022

curtbushko Jun 30, 2022

jmurret left a comment

jmurret Jun 29, 2022

jmurret Jun 29, 2022

curtbushko Jun 30, 2022

		)

		replace github.com/hashicorp/consul/sdk v0.9.0 => github.com/hashicorp/consul/sdk v0.4.1-0.20220531155537-364758ef2f50

Initial CNI installer & plugin #1304

Initial CNI installer & plugin #1304

Conversation

curtbushko commented Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

curtbushko Jun 24, 2022 • edited Loading

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

Choose a reason for hiding this comment

ishustava commented Jun 27, 2022

curtbushko commented Jun 27, 2022 • edited Loading

lkysow 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

jmurret 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

curtbushko commented Jun 24, 2022 •

edited

Loading

curtbushko Jun 24, 2022 •

edited

Loading

curtbushko commented Jun 27, 2022 •

edited

Loading