-
Notifications
You must be signed in to change notification settings - Fork 269
[WIP] Add support for chained plugins. #179
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package network | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/pkg/errors" | ||
|
|
||
| operv1 "github.com/openshift/api/operator/v1" | ||
| ) | ||
|
|
||
| const CNINetworkName = "openshift" | ||
|
|
||
| // Whatever network is the default network should use this as the target CNI | ||
| // configuration file. | ||
| const CNIConfigPath = "/etc/cni/net.d/80-openshift.conflist" | ||
|
|
||
| // validateChainedPlugins ensures that all supplied chained plugins are some | ||
| // kind of reasonable cni configuration | ||
| func validateChainedPlugins(conf *operv1.NetworkSpec) []error { | ||
| out := []error{} | ||
|
|
||
| if len(conf.DefaultNetwork.ChainedPlugins) > 0 && conf.DefaultNetwork.Type != operv1.NetworkTypeOpenShiftSDN { | ||
| out = append(out, errors.Errorf("network type %s does not support chained plugins", conf.DefaultNetwork.Type)) | ||
| } | ||
|
|
||
| for i, entry := range conf.DefaultNetwork.ChainedPlugins { | ||
| if entry.RawCNIConfig == "" { | ||
| out = append(out, errors.Errorf("invalid CNI plugin entry Spec.DefaultNetwork.ChainedPlugin[%d]: rawCNIConfig must be specified", i)) | ||
| continue | ||
| } | ||
| var m map[string]interface{} | ||
| if err := json.Unmarshal([]byte(entry.RawCNIConfig), &m); err != nil { | ||
| out = append(out, errors.Wrapf(err, "invalid json in Spec.DefaultNetwork.ChainedPlugin[%d].RawCNIConfig", i)) | ||
| continue | ||
| } | ||
|
|
||
| if _, ok := m["type"]; !ok { | ||
| out = append(out, errors.Errorf("invalid CNI plugin entry in Spec.DefaultNetwork.ChainedPlugin[%d].RawCNIConfig: must have 'type' key", i)) | ||
| } | ||
| } | ||
|
|
||
| return out | ||
| } | ||
|
|
||
| // makeCNIConfig merges the primary plugin configuration (as JSON) with any chained | ||
| // plugin's configurations. | ||
| func makeCNIConfig(conf *operv1.NetworkSpec, netName, cniVersion, primaryPluginConfig string) string { | ||
| pluginConfigs := []string{primaryPluginConfig} | ||
| for _, entry := range conf.DefaultNetwork.ChainedPlugins { | ||
| pluginConfigs = append(pluginConfigs, entry.RawCNIConfig) | ||
| } | ||
|
|
||
| configFile := fmt.Sprintf(` | ||
| { | ||
| "name": "%s", | ||
| "cniVersion": "%s", | ||
| "plugins": [ | ||
| %s | ||
| ] | ||
| }`, | ||
| netName, cniVersion, strings.Join(pluginConfigs, ",\n\t")) | ||
|
|
||
| return configFile | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package network | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| operv1 "github.com/openshift/api/operator/v1" | ||
|
|
||
| . "github.com/onsi/gomega" | ||
| ) | ||
|
|
||
| func TestMakeCNIConfig(t *testing.T) { | ||
| g := NewGomegaWithT(t) | ||
| conf := operv1.NetworkSpec{} | ||
|
|
||
| out := makeCNIConfig(&conf, "test1", "0.1.2", `{"type": "only"}`) | ||
| g.Expect(out).To(MatchJSON(` | ||
| { | ||
| "cniVersion": "0.1.2", | ||
| "name": "test1", | ||
| "plugins": [{"type": "only"}] | ||
| }`)) | ||
|
|
||
| conf.DefaultNetwork.ChainedPlugins = []operv1.ChainedPluginEntry{ | ||
| {RawCNIConfig: `{"type": "foo"}`}, | ||
| {RawCNIConfig: `{"type": "bar", "a": "b", "c":{"a": "b", "c": "d"}}`}, | ||
| } | ||
|
|
||
| out = makeCNIConfig(&conf, "test2", "1.2.3", `{"type": "primary"}`) | ||
| g.Expect(out).To(MatchJSON(` | ||
| { | ||
| "cniVersion": "1.2.3", | ||
| "name": "test2", | ||
| "plugins": [ | ||
| {"type": "primary"}, | ||
| {"type": "foo"}, | ||
| {"type": "bar", | ||
| "a": "b", | ||
| "c": { | ||
| "a": "b", | ||
| "c": "d" | ||
| } | ||
| }] | ||
| }`)) | ||
|
|
||
| } | ||
|
|
||
| func TestValidateChainedPlugins(t *testing.T) { | ||
| g := NewGomegaWithT(t) | ||
|
|
||
| conf := &operv1.NetworkSpec{} | ||
| g.Expect(validateChainedPlugins(conf)).To(BeEmpty()) | ||
|
|
||
| conf.DefaultNetwork.ChainedPlugins = []operv1.ChainedPluginEntry{ | ||
| {RawCNIConfig: `{"type": "foo"}`}, | ||
| {RawCNIConfig: `{"type": "bar"}`}, | ||
| } | ||
| conf.DefaultNetwork.Type = "unknown" | ||
| g.Expect(validateChainedPlugins(conf)).To(ContainElement(MatchError( | ||
| ContainSubstring("network type unknown does not support chained plugins")))) | ||
|
|
||
| conf.DefaultNetwork.Type = "OpenShiftSDN" | ||
| g.Expect(validateChainedPlugins(conf)).To(BeEmpty()) | ||
|
|
||
| conf.DefaultNetwork.ChainedPlugins = []operv1.ChainedPluginEntry{ | ||
| {RawCNIConfig: `asdfasdf`}, | ||
| } | ||
| g.Expect(validateChainedPlugins(conf)).To(ContainElement(MatchError( | ||
| ContainSubstring("invalid json in Spec.DefaultNetwork.ChainedPlugin[0].RawCNIConfig")))) | ||
|
|
||
| conf.DefaultNetwork.ChainedPlugins = []operv1.ChainedPluginEntry{ | ||
| {RawCNIConfig: `{"type": "foo"}`}, | ||
| {RawCNIConfig: `{"name": "bar"}`}, | ||
| } | ||
| g.Expect(validateChainedPlugins(conf)).To(ContainElement(MatchError( | ||
| ContainSubstring("invalid CNI plugin entry in Spec.DefaultNetwork.ChainedPlugin[1].RawCNIConfig: must have 'type' key")))) | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ func renderOpenShiftSDN(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Un | |
| data.Data["KUBERNETES_SERVICE_HOST"] = os.Getenv("KUBERNETES_SERVICE_HOST") | ||
| data.Data["KUBERNETES_SERVICE_PORT"] = os.Getenv("KUBERNETES_SERVICE_PORT") | ||
| data.Data["Mode"] = c.Mode | ||
| data.Data["CNIPath"] = CNIConfigPath | ||
|
|
||
| operCfg, err := controllerConfig(conf) | ||
| if err != nil { | ||
|
|
@@ -52,6 +53,9 @@ func renderOpenShiftSDN(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Un | |
| } | ||
| data.Data["NodeConfig"] = nodeCfg | ||
|
|
||
| data.Data["CNIConfig"] = makeCNIConfig(conf, CNINetworkName, "0.3.1", | ||
| `{"type": "openshift-sdn"}`) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not super happy about this, can we just have the SDN itself do the combination? eg the CNO makes the template from the ChainedPlugins and the SDN inserts itself at position 0 in the list and then writes the result conflist to the final CNI configdir that CRIO looks for? eg I'd rather have the SDN plugin itself handle its config rather than encode logic about the conf/type/version/etc in the operator...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was going to say something similar before, but there is already a crazy amount of coupling between the operator and the SDN plugin itself anyway. This doesn't really make it worse. (And besides, what happens if one of the chained plugins wants to use some feature that requires using version 0.4.0 of the spec?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I thought about that for a long time, and couldn't find a solution I like. First of all, the cniVersion is a tricky coordination point no matter where you place it. It needs to be the same for all plugins, and all plugins need to support the selected versions. If we bump openshift-sdn to 0.4.0, then all chained plugins instantly must support 0.4.0. I suppose this is a problem with CNI, and we may wish to revisit that decision. Wherever we do the config munging doesn't fix this problem. As for having openshift-sdn insert itself, I decided against that for a few reasons:
I'm happy to revisit this. It's not great either way you look at it. |
||
|
|
||
| manifests, err := render.RenderDir(filepath.Join(manifestDir, "network/openshift-sdn"), &data) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to render manifests") | ||
|
|
@@ -105,7 +109,7 @@ func isOpenShiftSDNChangeSafe(prev, next *operv1.NetworkSpec) []error { | |
| } | ||
|
|
||
| func fillOpenShiftSDNDefaults(conf, previous *operv1.NetworkSpec, hostMTU int) { | ||
| // NOTE: If you change any defaults, and it's not a safe chang to roll out | ||
| // NOTE: If you change any defaults, and it's not a safe change to roll out | ||
| // to existing clusters, you MUST use the value from previous instead. | ||
| if conf.DeployKubeProxy == nil { | ||
| prox := false | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should note that this applies to all containers in the cluster.