From 7273740bdd0a57b711e4df7b61a5c6e8924dd60b Mon Sep 17 00:00:00 2001 From: Abhinav Dahiya Date: Mon, 25 Mar 2019 17:31:02 -0700 Subject: [PATCH 1/2] controller/bootstrap: use files with multiple yaml documents Users can push manifests during bootstrap that of the form: ```yaml --- ``` Especially for the installer: setting authorizes_keys [1] and setting hyperthreading [2] will push a manifest that includes multiple machineconfig objects for control-plane (master) and compute (worker) roles. Single file with multiple k8s objects separated by `---` is also a supported structure for `oc create|apply` ie. there is a high chance that users trying to push machineconfigs at install time might create such files. This commit allows bootstrap controller to read all k8s objects, even ones described above to find all the `machineconfiguration.openshift.io` Objects. [1]: https://github.com/openshift/installer/pull/1150 [2]: https://github.com/openshift/installer/pull/1392 --- pkg/controller/bootstrap/bootstrap.go | 88 ++++++++++++--- pkg/controller/bootstrap/bootstrap_test.go | 119 +++++++++++++++++++++ 2 files changed, 191 insertions(+), 16 deletions(-) create mode 100644 pkg/controller/bootstrap/bootstrap_test.go diff --git a/pkg/controller/bootstrap/bootstrap.go b/pkg/controller/bootstrap/bootstrap.go index 0814643b1c..e9df817c5a 100644 --- a/pkg/controller/bootstrap/bootstrap.go +++ b/pkg/controller/bootstrap/bootstrap.go @@ -1,7 +1,10 @@ package bootstrap import ( + "bytes" + "errors" "fmt" + "io" "io/ioutil" "os" "path/filepath" @@ -10,6 +13,7 @@ import ( "github.com/golang/glog" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + yamlutil "k8s.io/apimachinery/pkg/util/yaml" kscheme "k8s.io/client-go/kubernetes/scheme" "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1" @@ -63,28 +67,35 @@ func (b *Bootstrap) Run(destDir string) error { continue } - path := filepath.Join(b.manifestDir, info.Name()) - raw, err := ioutil.ReadFile(path) + file, err := os.Open(filepath.Join(b.manifestDir, info.Name())) if err != nil { - return err + return fmt.Errorf("error opening %s: %v", file.Name(), err) } + defer file.Close() - obji, err := runtime.Decode(scheme.Codecs.UniversalDecoder(v1.SchemeGroupVersion), raw) + manifests, err := parseManifests(file.Name(), file) if err != nil { - glog.V(4).Infof("skipping path because of error: %v", err) - // don't care - continue + return fmt.Errorf("error parsing manifests from %s: %v", file.Name(), err) } - switch obj := obji.(type) { - case *v1.MachineConfigPool: - pools = append(pools, obj) - case *v1.MachineConfig: - configs = append(configs, obj) - case *v1.ControllerConfig: - cconfig = obj - default: - glog.Infof("skipping %q %T", path, obji) + for idx, m := range manifests { + obji, err := runtime.Decode(scheme.Codecs.UniversalDecoder(v1.SchemeGroupVersion), m.Raw) + if err != nil { + glog.V(4).Infof("skipping path %q [%d] manifest because of error: %v", file.Name(), idx+1, err) + // don't care + continue + } + + switch obj := obji.(type) { + case *v1.MachineConfigPool: + pools = append(pools, obj) + case *v1.MachineConfig: + configs = append(configs, obj) + case *v1.ControllerConfig: + cconfig = obj + default: + glog.Infof("skipping %q [%d] manifest because %T", file.Name(), idx+1, obji) + } } } @@ -148,3 +159,48 @@ func getPullSecretFromSecret(sData []byte) ([]byte, error) { } return s.Data[corev1.DockerConfigJsonKey], nil } + +type manifest struct { + Raw []byte +} + +// UnmarshalJSON unmarshals bytes of single kubernetes object to manifest. +func (m *manifest) UnmarshalJSON(in []byte) error { + if m == nil { + return errors.New("Manifest: UnmarshalJSON on nil pointer") + } + + // This happens when marshalling + // + // --- (this between two `---`) + // --- + // + if bytes.Equal(in, []byte("null")) { + m.Raw = nil + return nil + } + + m.Raw = append(m.Raw[0:0], in...) + return nil +} + +// parseManifests parses a YAML or JSON document that may contain one or more +// kubernetes resources. +func parseManifests(filename string, r io.Reader) ([]manifest, error) { + d := yamlutil.NewYAMLOrJSONDecoder(r, 1024) + var manifests []manifest + for { + m := manifest{} + if err := d.Decode(&m); err != nil { + if err == io.EOF { + return manifests, nil + } + return manifests, fmt.Errorf("error parsing %q: %v", filename, err) + } + m.Raw = bytes.TrimSpace(m.Raw) + if len(m.Raw) == 0 || bytes.Equal(m.Raw, []byte("null")) { + continue + } + manifests = append(manifests, m) + } +} diff --git a/pkg/controller/bootstrap/bootstrap_test.go b/pkg/controller/bootstrap/bootstrap_test.go new file mode 100644 index 0000000000..7160e9055e --- /dev/null +++ b/pkg/controller/bootstrap/bootstrap_test.go @@ -0,0 +1,119 @@ +package bootstrap + +import ( + "reflect" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/util/diff" +) + +func TestParseManifests(t *testing.T) { + tests := []struct { + name string + raw string + want []manifest + }{{ + name: "ingress", + raw: ` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress + namespace: test-namespace +spec: + rules: + - http: + paths: + - path: /testpath + backend: + serviceName: test + servicePort: 80 +`, + want: []manifest{{ + Raw: []byte(`{"apiVersion":"extensions/v1beta1","kind":"Ingress","metadata":{"name":"test-ingress","namespace":"test-namespace"},"spec":{"rules":[{"http":{"paths":[{"backend":{"serviceName":"test","servicePort":80},"path":"/testpath"}]}}]}}`), + }}, + }, { + name: "two-resources", + raw: ` +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress + namespace: test-namespace +spec: + rules: + - http: + paths: + - path: /testpath + backend: + serviceName: test + servicePort: 80 +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: a-config + namespace: default +data: + color: "red" + multi-line: | + hello world + how are you? +`, + want: []manifest{{ + Raw: []byte(`{"apiVersion":"extensions/v1beta1","kind":"Ingress","metadata":{"name":"test-ingress","namespace":"test-namespace"},"spec":{"rules":[{"http":{"paths":[{"backend":{"serviceName":"test","servicePort":80},"path":"/testpath"}]}}]}}`), + }, { + Raw: []byte(`{"apiVersion":"v1","data":{"color":"red","multi-line":"hello world\nhow are you?\n"},"kind":"ConfigMap","metadata":{"name":"a-config","namespace":"default"}}`), + }}, + }, { + name: "two-resources-with-empty", + raw: ` +--- +apiVersion: extensions/v1beta1 +kind: Ingress +metadata: + name: test-ingress + namespace: test-namespace +spec: + rules: + - http: + paths: + - path: /testpath + backend: + serviceName: test + servicePort: 80 +--- +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: a-config + namespace: default +data: + color: "red" + multi-line: | + hello world + how are you? +--- +`, + want: []manifest{{ + Raw: []byte(`{"apiVersion":"extensions/v1beta1","kind":"Ingress","metadata":{"name":"test-ingress","namespace":"test-namespace"},"spec":{"rules":[{"http":{"paths":[{"backend":{"serviceName":"test","servicePort":80},"path":"/testpath"}]}}]}}`), + }, { + Raw: []byte(`{"apiVersion":"v1","data":{"color":"red","multi-line":"hello world\nhow are you?\n"},"kind":"ConfigMap","metadata":{"name":"a-config","namespace":"default"}}`), + }}, + }} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, err := parseManifests("dummy-file-name", strings.NewReader(test.raw)) + if err != nil { + t.Fatalf("failed to parse manifest: %v", err) + } + + if !reflect.DeepEqual(got, test.want) { + t.Fatalf("mismatch found %s", diff.ObjectDiff(got, test.want)) + } + }) + } + +} From df19bd7f4a7948b243c604a17685e74767d6fd6b Mon Sep 17 00:00:00 2001 From: Abhinav Dahiya Date: Tue, 26 Mar 2019 11:32:38 -0700 Subject: [PATCH 2/2] controller/bootstrap: differentiate between errors when decoding When decoding a manifest we get 2 kinds of errors: - this manifest does not belong to mco apigroup and hence we do not care about these failures and we need to skip. - the manifest does belong to mco apigroup, but it cannot be decoded into any of the known types, this needs to be a failure. Using IsNotRegistreredError [1] allows aus to differentiate between the 2 errors. And therefore, later kind of error is now failure. [1]: https://godoc.org/k8s.io/apimachinery/pkg/runtime#IsNotRegisteredError --- pkg/controller/bootstrap/bootstrap.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/controller/bootstrap/bootstrap.go b/pkg/controller/bootstrap/bootstrap.go index e9df817c5a..f98cb0b536 100644 --- a/pkg/controller/bootstrap/bootstrap.go +++ b/pkg/controller/bootstrap/bootstrap.go @@ -81,9 +81,12 @@ func (b *Bootstrap) Run(destDir string) error { for idx, m := range manifests { obji, err := runtime.Decode(scheme.Codecs.UniversalDecoder(v1.SchemeGroupVersion), m.Raw) if err != nil { - glog.V(4).Infof("skipping path %q [%d] manifest because of error: %v", file.Name(), idx+1, err) - // don't care - continue + if runtime.IsNotRegisteredError(err) { + // don't care + glog.V(4).Infof("skipping path %q [%d] manifest because it is not part of expected api group: %v", file.Name(), idx+1, err) + continue + } + return fmt.Errorf("error parsing %q [%d] manifest: %v", file.Name(), idx+1, err) } switch obj := obji.(type) { @@ -94,7 +97,7 @@ func (b *Bootstrap) Run(destDir string) error { case *v1.ControllerConfig: cconfig = obj default: - glog.Infof("skipping %q [%d] manifest because %T", file.Name(), idx+1, obji) + glog.Infof("skipping %q [%d] manifest because of unhandled %T", file.Name(), idx+1, obji) } } }