-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ CABPK can now read file contents from a Secret #3083
Changes from 1 commit
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 | ||||
---|---|---|---|---|---|---|
|
@@ -17,11 +17,91 @@ limitations under the License. | |||||
package v1alpha3 | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
|
||||||
apierrors "k8s.io/apimachinery/pkg/api/errors" | ||||||
runtime "k8s.io/apimachinery/pkg/runtime" | ||||||
"k8s.io/apimachinery/pkg/util/validation/field" | ||||||
ctrl "sigs.k8s.io/controller-runtime" | ||||||
"sigs.k8s.io/controller-runtime/pkg/webhook" | ||||||
) | ||||||
|
||||||
var ( | ||||||
ConflictingFileSourceMsg = "only one of content of contentFrom may be specified for a single file" | ||||||
MissingFileSourceMsg = "source for file content must be specified if contenFrom is non-nil" | ||||||
MissingSecretNameMsg = "secret file source must specify non-empty secret name" | ||||||
MissingSecretKeyMsg = "secret file source must specify non-empty secret key" | ||||||
) | ||||||
|
||||||
func (r *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error { | ||||||
func (c *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error { | ||||||
return ctrl.NewWebhookManagedBy(mgr). | ||||||
For(r). | ||||||
For(c). | ||||||
Complete() | ||||||
} | ||||||
|
||||||
// +kubebuilder:webhook:verbs=create;update,path=/validate-bootstrap-cluster-x-k8s-io-v1alpha3-kubeadmconfig,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=bootstrap.cluster.x-k8s.io,resources=kubeadmconfigs,versions=v1alpha3,name=validation.kubeadmconfig.bootstrap.cluster.x-k8s.io,sideEffects=None | ||||||
|
||||||
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() | ||||||
} | ||||||
|
||||||
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type | ||||||
func (c *KubeadmConfig) ValidateUpdate(old runtime.Object) error { | ||||||
return c.validate() | ||||||
} | ||||||
|
||||||
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type | ||||||
func (c *KubeadmConfig) ValidateDelete() error { | ||||||
return nil | ||||||
} | ||||||
|
||||||
func (c *KubeadmConfig) validate() error { | ||||||
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.
Suggested change
If we add this method to 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. better? |
||||||
var allErrs field.ErrorList | ||||||
|
||||||
for i := range c.Spec.Files { | ||||||
file := c.Spec.Files[i] | ||||||
if file.Content != "" && file.ContentFrom != nil { | ||||||
allErrs = append( | ||||||
allErrs, | ||||||
field.Invalid( | ||||||
field.NewPath("spec", "files", fmt.Sprintf("%d", i)), | ||||||
file, | ||||||
ConflictingFileSourceMsg, | ||||||
), | ||||||
) | ||||||
} | ||||||
// 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.Secret.Name == "" { | ||||||
allErrs = append( | ||||||
allErrs, | ||||||
field.Invalid( | ||||||
field.NewPath("spec", "files", fmt.Sprintf("%d", i), "contentFrom", "secret", "name"), | ||||||
file, | ||||||
MissingSecretNameMsg, | ||||||
), | ||||||
) | ||||||
} | ||||||
if file.ContentFrom.Secret.Key == "" { | ||||||
allErrs = append( | ||||||
allErrs, | ||||||
field.Invalid( | ||||||
field.NewPath("spec", "files", fmt.Sprintf("%d", i), "contentFrom", "secret", "key"), | ||||||
file, | ||||||
MissingSecretKeyMsg, | ||||||
), | ||||||
) | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
if len(allErrs) == 0 { | ||||||
return nil | ||||||
} | ||||||
return apierrors.NewInvalid(GroupVersion.WithKind("KubeadmConfig").GroupKind(), c.Name, allErrs) | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ import ( | |
apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/types" | ||
kerrors "k8s.io/apimachinery/pkg/util/errors" | ||
"k8s.io/utils/pointer" | ||
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3" | ||
|
@@ -365,9 +366,19 @@ 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...) | ||
|
||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
additionalFiles, err := r.resolveFiles(ctx, scope.Config) | ||
if err != nil { | ||
scope.Error(err, "Failed to resolve files") | ||
return ctrl.Result{}, err | ||
} | ||
|
||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
files = append(files, additionalFiles...) | ||
|
||
cloudInitData, err := cloudinit.NewInitControlPlane(&cloudinit.ControlPlaneInput{ | ||
BaseUserData: cloudinit.BaseUserData{ | ||
AdditionalFiles: scope.Config.Spec.Files, | ||
AdditionalFiles: files, | ||
NTP: scope.Config.Spec.NTP, | ||
PreKubeadmCommands: scope.Config.Spec.PreKubeadmCommands, | ||
PostKubeadmCommands: scope.Config.Spec.PostKubeadmCommands, | ||
|
@@ -433,9 +444,17 @@ 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) | ||
if err != nil { | ||
scope.Error(err, "Failed to resolve files") | ||
return ctrl.Result{}, err | ||
} | ||
|
||
var files = append(scope.Config.Spec.Files, additionalFiles...) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
cloudJoinData, err := cloudinit.NewNode(&cloudinit.NodeInput{ | ||
BaseUserData: cloudinit.BaseUserData{ | ||
AdditionalFiles: scope.Config.Spec.Files, | ||
AdditionalFiles: files, | ||
NTP: scope.Config.Spec.NTP, | ||
PreKubeadmCommands: scope.Config.Spec.PreKubeadmCommands, | ||
PostKubeadmCommands: scope.Config.Spec.PostKubeadmCommands, | ||
|
@@ -499,14 +518,26 @@ 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))) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
var files = append(certificates.AsFiles(), scope.Config.Spec.Files...) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
additionalFiles, err := r.resolveFiles(ctx, scope.Config) | ||
if err != nil { | ||
scope.Error(err, "Failed to resolve files") | ||
return ctrl.Result{}, err | ||
} | ||
|
||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
files = append(files, additionalFiles...) | ||
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 noticed these files appending logic is repeated in multiple places, can we provide a utility method somewhere to detail with it? 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. how's that? |
||
|
||
cloudJoinData, err := cloudinit.NewJoinControlPlane(&cloudinit.ControlPlaneJoinInput{ | ||
JoinConfiguration: joinData, | ||
Certificates: certificates, | ||
BaseUserData: cloudinit.BaseUserData{ | ||
AdditionalFiles: scope.Config.Spec.Files, | ||
AdditionalFiles: files, | ||
NTP: scope.Config.Spec.NTP, | ||
PreKubeadmCommands: scope.Config.Spec.PreKubeadmCommands, | ||
PostKubeadmCommands: scope.Config.Spec.PostKubeadmCommands, | ||
|
@@ -528,6 +559,68 @@ func (r *KubeadmConfigReconciler) joinControlplane(ctx context.Context, scope *S | |
return ctrl.Result{}, nil | ||
} | ||
|
||
// 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 | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 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) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
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") | ||
} | ||
} | ||
|
||
// 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 | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
nn := types.NamespacedName{Namespace: ns, Name: source.ContentFrom.Secret.Name} | ||
if err := r.Client.Get(ctx, nn, &secret); err != nil { | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if apierrors.IsNotFound(err) { | ||
return bootstrapv1.File{}, errors.Wrapf(err, "secret not found: %s", nn) | ||
} | ||
return bootstrapv1.File{}, errors.Wrapf(err, "getting Secret %s", nn) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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) | ||
alexeldeib marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return bootstrapv1.File{ | ||
Owner: source.Owner, | ||
Permissions: source.Permissions, | ||
Encoding: source.Encoding, | ||
Content: string(data), | ||
Path: source.Path, | ||
}, nil | ||
} | ||
|
||
// ClusterToKubeadmConfigs is a handler.ToRequestsFunc to be used to enqeue | ||
// requests for reconciliation of KubeadmConfigs. | ||
func (r *KubeadmConfigReconciler) ClusterToKubeadmConfigs(o handler.MapObject) []ctrl.Request { | ||
|
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.
I think this is what we want? Otherwise we have to match files based on some property field to match array items?
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.
We should only restore the files that have
ContentFrom
populated (non-nil)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.
Is this because the restored version might be stale where conversion was possible?
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.
Correct, in case a v1alpha2 controller modified some of the files (even though unlikely because bootstrap should only happen once), we need to make sure to restore those that have the new schema.
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.
I'm not sure I follow what this is doing... we're setting
dst.Spec.Files
torestored.Spec.Files
and then appending some of the restored files todst.Spec.Files
? Isn't that going to result in duplicates?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.
Nice catch.
I think the first line
dst.Spec.Files = restores.Spec.Files
is leftover from my first revision, it should only be the loop. The intent is to look at restored files with contentFrom and append them. We shouldn’t append restored files with content set.Will PR shortly