From d06b3b43737ad8ab260d9867c27df8aad29fd4b2 Mon Sep 17 00:00:00 2001 From: Alexander Eldeib Date: Fri, 22 May 2020 15:24:56 -0700 Subject: [PATCH] :hammer_and_pick: simplify file resolution, style Signed-off-by: Alexander Eldeib Co-authored-by: Vince Prignano --- .../kubeadm/api/v1alpha2/conversion_test.go | 2 +- .../v1alpha3/kubeadmbootstrapconfig_types.go | 2 +- .../api/v1alpha3/kubeadmconfig_webhook.go | 14 ++-- .../api/v1alpha3/zz_generated.deepcopy.go | 8 +- .../webhook/webhookcainjection_patch.yaml | 8 -- .../controllers/kubeadmconfig_controller.go | 76 +++++-------------- .../kubeadmconfig_controller_test.go | 4 +- 7 files changed, 33 insertions(+), 81 deletions(-) diff --git a/bootstrap/kubeadm/api/v1alpha2/conversion_test.go b/bootstrap/kubeadm/api/v1alpha2/conversion_test.go index 44f0a23846f5..76940f494dad 100644 --- a/bootstrap/kubeadm/api/v1alpha2/conversion_test.go +++ b/bootstrap/kubeadm/api/v1alpha2/conversion_test.go @@ -39,7 +39,7 @@ func TestConvertKubeadmConfig(t *testing.T) { Files: []v1alpha3.File{ { ContentFrom: &v1alpha3.FileSource{ - Secret: &v1alpha3.SecretFileSource{ + Secret: v1alpha3.SecretFileSource{ Name: "foo", Key: "bar", }, diff --git a/bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go b/bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go index 1877ebcb53d9..5fcf4d200c9e 100644 --- a/bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go +++ b/bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go @@ -185,7 +185,7 @@ type File struct { // sources of data for target systems should add them here. type FileSource struct { // Secret represents a secret that should populate this file. - Secret *SecretFileSource `json:"secret"` + Secret SecretFileSource `json:"secret"` } // Adapts a Secret into a FileSource. diff --git a/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go b/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go index 0bb1f30308b8..c8e95a9063a5 100644 --- a/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go +++ b/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go @@ -45,12 +45,12 @@ var _ webhook.Validator = &KubeadmConfig{} // ValidateCreate implements webhook.Validator so a webhook will be registered for the type func (c *KubeadmConfig) ValidateCreate() error { - return c.validate() + return c.Spec.validate(c.Name) } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type func (c *KubeadmConfig) ValidateUpdate(old runtime.Object) error { - return c.validate() + return c.Spec.validate(c.Name) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type @@ -58,11 +58,11 @@ func (c *KubeadmConfig) ValidateDelete() error { return nil } -func (c *KubeadmConfig) validate() error { +func (c *KubeadmConfigSpec) validate(name string) error { var allErrs field.ErrorList - for i := range c.Spec.Files { - file := c.Spec.Files[i] + for i := range c.Files { + file := c.Files[i] if file.Content != "" && file.ContentFrom != nil { allErrs = append( allErrs, @@ -76,7 +76,7 @@ func (c *KubeadmConfig) validate() error { // n.b.: if we ever add types besides Secret as a ContentFrom // Source, we must add webhook validation here for one of the // sources being non-nil. - if file.ContentFrom != nil && file.ContentFrom.Secret != nil { + if file.ContentFrom != nil { if file.ContentFrom.Secret.Name == "" { allErrs = append( allErrs, @@ -103,5 +103,5 @@ func (c *KubeadmConfig) validate() error { if len(allErrs) == 0 { return nil } - return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmConfig").GroupKind(), c.Name, allErrs) + return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmConfig").GroupKind(), name, allErrs) } diff --git a/bootstrap/kubeadm/api/v1alpha3/zz_generated.deepcopy.go b/bootstrap/kubeadm/api/v1alpha3/zz_generated.deepcopy.go index fdcfe797b439..72c1e952f200 100644 --- a/bootstrap/kubeadm/api/v1alpha3/zz_generated.deepcopy.go +++ b/bootstrap/kubeadm/api/v1alpha3/zz_generated.deepcopy.go @@ -31,7 +31,7 @@ func (in *File) DeepCopyInto(out *File) { if in.ContentFrom != nil { in, out := &in.ContentFrom, &out.ContentFrom *out = new(FileSource) - (*in).DeepCopyInto(*out) + **out = **in } } @@ -48,11 +48,7 @@ func (in *File) DeepCopy() *File { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *FileSource) DeepCopyInto(out *FileSource) { *out = *in - if in.Secret != nil { - in, out := &in.Secret, &out.Secret - *out = new(SecretFileSource) - **out = **in - } + out.Secret = in.Secret } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FileSource. diff --git a/bootstrap/kubeadm/config/webhook/webhookcainjection_patch.yaml b/bootstrap/kubeadm/config/webhook/webhookcainjection_patch.yaml index 275782aa1b48..e838a4bf1d25 100644 --- a/bootstrap/kubeadm/config/webhook/webhookcainjection_patch.yaml +++ b/bootstrap/kubeadm/config/webhook/webhookcainjection_patch.yaml @@ -1,11 +1,3 @@ -# This patch add annotation to admission webhook config and -# the variables $(CERTIFICATE_NAMESPACE) and $(CERTIFICATE_NAME) will be substituted by kustomize. -# apiVersion: admissionregistration.k8s.io/v1beta1 -# kind: MutatingWebhookConfiguration -# metadata: -# name: mutating-webhook-configuration -# annotations: -# cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME) --- apiVersion: admissionregistration.k8s.io/v1beta1 kind: ValidatingWebhookConfiguration diff --git a/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go b/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go index 9369719f1333..2bf16e6f6a49 100644 --- a/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go +++ b/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go @@ -366,16 +366,12 @@ func (r *KubeadmConfigReconciler) handleClusterNotInitialized(ctx context.Contex verbosityFlag = fmt.Sprintf("--v %s", strconv.Itoa(int(*scope.Config.Spec.Verbosity))) } - files := append(certificates.AsFiles(), scope.Config.Spec.Files...) - - additionalFiles, err := r.resolveFiles(ctx, scope.Config) + files, err := r.resolveFiles(ctx, scope.Config, certificates.AsFiles()...) if err != nil { scope.Error(err, "Failed to resolve files") return ctrl.Result{}, err } - files = append(files, additionalFiles...) - cloudInitData, err := cloudinit.NewInitControlPlane(&cloudinit.ControlPlaneInput{ BaseUserData: cloudinit.BaseUserData{ AdditionalFiles: files, @@ -444,14 +440,12 @@ func (r *KubeadmConfigReconciler) joinWorker(ctx context.Context, scope *Scope) verbosityFlag = fmt.Sprintf("--v %s", strconv.Itoa(int(*scope.Config.Spec.Verbosity))) } - additionalFiles, err := r.resolveFiles(ctx, scope.Config) + files, err := r.resolveFiles(ctx, scope.Config) if err != nil { scope.Error(err, "Failed to resolve files") return ctrl.Result{}, err } - var files = append(scope.Config.Spec.Files, additionalFiles...) - cloudJoinData, err := cloudinit.NewNode(&cloudinit.NodeInput{ BaseUserData: cloudinit.BaseUserData{ AdditionalFiles: files, @@ -518,21 +512,15 @@ func (r *KubeadmConfigReconciler) joinControlplane(ctx context.Context, scope *S verbosityFlag := "" if scope.Config.Spec.Verbosity != nil { - verbosityFlag = - - fmt.Sprintf("--v %s", strconv.Itoa(int(*scope.Config.Spec.Verbosity))) + verbosityFlag = fmt.Sprintf("--v %s", strconv.Itoa(int(*scope.Config.Spec.Verbosity))) } - var files = append(certificates.AsFiles(), scope.Config.Spec.Files...) - - additionalFiles, err := r.resolveFiles(ctx, scope.Config) + files, err := r.resolveFiles(ctx, scope.Config, certificates.AsFiles()...) if err != nil { scope.Error(err, "Failed to resolve files") return ctrl.Result{}, err } - files = append(files, additionalFiles...) - cloudJoinData, err := cloudinit.NewJoinControlPlane(&cloudinit.ControlPlaneJoinInput{ JoinConfiguration: joinData, Certificates: certificates, @@ -561,64 +549,40 @@ func (r *KubeadmConfigReconciler) joinControlplane(ctx context.Context, scope *S // resolveFiles maps .Spec.Files into cloudinit.Files, resolving any object references // along the way. -func (r *KubeadmConfigReconciler) resolveFiles(ctx context.Context, cfg *bootstrapv1.KubeadmConfig) ([]bootstrapv1.File, error) { - var converted []bootstrapv1.File +func (r *KubeadmConfigReconciler) resolveFiles(ctx context.Context, cfg *bootstrapv1.KubeadmConfig, merge ...bootstrapv1.File) ([]bootstrapv1.File, error) { + collected := append(cfg.Spec.Files, merge...) for i := range cfg.Spec.Files { in := cfg.Spec.Files[i] - switch { - case in.ContentFrom != nil: - file, err := r.resolveContent(ctx, cfg.Namespace, in) + if in.ContentFrom != nil { + data, err := r.resolveSecretFileContent(ctx, cfg.Namespace, in) if err != nil { return nil, errors.Wrapf(err, "failed to resolve file source") } - converted = append(converted, file) - case in.Content != "": - converted = append(converted, in) - default: - return nil, fmt.Errorf("could not find content to populate file: %s", in.Path) + in.ContentFrom = nil + in.Content = string(data) + cfg.Spec.Files[i] = in } } - return converted, nil -} - -func (r *KubeadmConfigReconciler) resolveContent(ctx context.Context, ns string, source bootstrapv1.File) (bootstrapv1.File, error) { - switch { - case source.ContentFrom.Secret != nil: - return r.resolveSecretFileContent(ctx, ns, source) - default: - return bootstrapv1.File{}, errors.New("could not find non-nil source for file content") - } + return collected, nil } // resolveSecretFileContent returns file content fetched from a referenced secret object. -func (r *KubeadmConfigReconciler) resolveSecretFileContent(ctx context.Context, ns string, source bootstrapv1.File) (bootstrapv1.File, error) { - var secret corev1.Secret - nn := types.NamespacedName{Namespace: ns, Name: source.ContentFrom.Secret.Name} - if err := r.Client.Get(ctx, nn, &secret); err != nil { +func (r *KubeadmConfigReconciler) resolveSecretFileContent(ctx context.Context, ns string, source bootstrapv1.File) ([]byte, error) { + secret := &corev1.Secret{} + key := types.NamespacedName{Namespace: ns, Name: source.ContentFrom.Secret.Name} + if err := r.Client.Get(ctx, key, secret); err != nil { if apierrors.IsNotFound(err) { - return bootstrapv1.File{}, errors.Wrapf(err, "secret not found: %s", nn) + return nil, errors.Wrapf(err, "secret not found: %s", key) } - return bootstrapv1.File{}, errors.Wrapf(err, "getting Secret %s", nn) + return nil, errors.Wrapf(err, "failed to retrieve Secret %q", key) } - - return secretToFile(&secret, source) -} - -func secretToFile(secret *corev1.Secret, source bootstrapv1.File) (bootstrapv1.File, error) { data, ok := secret.Data[source.ContentFrom.Secret.Key] if !ok { - return bootstrapv1.File{}, fmt.Errorf("secret references non-existent secret key: %s", source.ContentFrom.Secret.Key) + return nil, errors.Errorf("secret references non-existent secret key: %q", source.ContentFrom.Secret.Key) } - - return bootstrapv1.File{ - Owner: source.Owner, - Permissions: source.Permissions, - Encoding: source.Encoding, - Content: string(data), - Path: source.Path, - }, nil + return data, nil } // ClusterToKubeadmConfigs is a handler.ToRequestsFunc to be used to enqeue diff --git a/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go b/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go index ccec4394c233..3cf99156c7c3 100644 --- a/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go +++ b/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go @@ -492,7 +492,7 @@ func TestKubeadmConfigReconcile_SecretSource(t *testing.T) { "defaults": { input: bootstrapv1.File{ ContentFrom: &bootstrapv1.FileSource{ - Secret: &bootstrapv1.SecretFileSource{ + Secret: bootstrapv1.SecretFileSource{ Name: secretName, Key: secretDataKey, }, @@ -509,7 +509,7 @@ func TestKubeadmConfigReconcile_SecretSource(t *testing.T) { "failure": { input: bootstrapv1.File{ ContentFrom: &bootstrapv1.FileSource{ - Secret: &bootstrapv1.SecretFileSource{ + Secret: bootstrapv1.SecretFileSource{ Name: secretName, Key: missingKey, },