✨ CABPK can now read file contents from a Secret #3083

alexeldeib · 2020-05-21T23:23:38Z

What this PR does / why we need it:
related to #1739

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

revised from #3038 after offline discussion

still needs some manual testing

alexeldeib · 2020-05-21T23:24:35Z

/hold

for some additional manual testing

alexeldeib · 2020-05-22T00:51:41Z

/test pull-cluster-api-e2e

vincepri · 2020-05-22T01:09:45Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

+func Convert_v1alpha3_File_To_v1alpha2_File(in *kubeadmbootstrapv1alpha3.File, out *File, s apiconversion.Scope) error {
+	// We don't implement manual conversion for types using contentFrom
+	return autoConvert_v1alpha3_File_To_v1alpha2_File(in, out, s)
+}


We should still restore it when we go v1alpha3 -> v1alpha2 -> v1alpha3, there is an example in other files, or I can help tomorrow

bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go

vincepri · 2020-05-22T01:11:13Z

bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go

+	// +optional
+	Secret *SecretFileSource `json:"secret,omitempty"`


This shouldn't be optional I think? If one specifies a FileSource in file.contentFrom: {} it should fail validation saying that secret is required

Since we only have one I was thinking if in the future we change this from required to optional it'd be breaking? But I think you mentioned today it's fine, and I concur that makes sense. Will make required.

The webhook does validate the stricter scenario already, I think you're correct regardless.

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

vincepri · 2020-05-22T01:12:54Z

/test pull-cluster-api-test

vincepri · 2020-05-22T01:13:12Z

Only reviewed ~40% for now, will continue later

Co-authored-by: Vince Prignano <[email protected]> Signed-off-by: Alexander Eldeib <[email protected]>

alexeldeib · 2020-05-22T02:50:13Z

I tried to reorder commits so all the functional stuff is in one because it's fairly small compared to total changes. Moved the rest into separate Go/yaml commits, hope that's easier to review.

alexeldeib · 2020-05-22T02:51:54Z

How this looks for Azure now, if anyone is interested:

apiVersion: bootstrap.cluster.x-k8s.io/v1alpha3
kind: KubeadmConfigTemplate
metadata:
  name: ${CLUSTER_NAME}-md-0
spec:
  template:
    spec:
      files:
      - contentFrom:
          secret:
            name: azure-secret
            key: azure.json
        owner: root:root
        path: /etc/kubernetes/azure.json
        permissions: "0644"
      joinConfiguration:
        nodeRegistration:
          kubeletExtraArgs:
            cloud-config: /etc/kubernetes/azure.json
            cloud-provider: azure
          name: '{{ ds.meta_data["local_hostname"] }}'
---
apiVersion: v1
kind: Secret
metadata:
  name: azure-secret
data:
  "azure.json": |
    ${AZURE_JSON_B64}

definitely nicer than the previous example IMO

alexeldeib · 2020-05-22T02:57:12Z

bootstrap/kubeadm/api/v1alpha2/conversion.go

@@ -39,6 +39,7 @@ func (src *KubeadmConfig) ConvertTo(dstRaw conversion.Hub) error {
 	dst.Status.DataSecretName = restored.Status.DataSecretName
 	dst.Spec.Verbosity = restored.Spec.Verbosity
 	dst.Spec.UseExperimentalRetryJoin = restored.Spec.UseExperimentalRetryJoin
+	dst.Spec.Files = restored.Spec.Files


I think this is what we want? Otherwise we have to match files based on some property field to match array items?

We should only restore the files that have ContentFrom populated (non-nil)

Is this because the restored version might be stale where conversion was possible?

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.

I'm not sure I follow what this is doing... we're setting dst.Spec.Files to restored.Spec.Files and then appending some of the restored files to dst.Spec.Files? Isn't that going to result in duplicates?

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

alexeldeib · 2020-05-22T03:00:29Z

/hold cancel

bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go

vincepri · 2020-05-22T15:12:56Z

bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_webhook.go

+	return nil
+}
+
+func (c *KubeadmConfig) validate() error {


Suggested change

func (c *KubeadmConfig) validate() error {

func (c *KubeadmConfigSpec) validate() error {

If we add this method to KubeadmConfigSpec instead of KubeadmConfig we should be able to call it from the webhooks defined under controlplane/kubeadm as well.

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

bootstrap/kubeadm/config/webhook/webhookcainjection_patch.yaml

alexeldeib · 2020-05-22T22:57:52Z

/retest

alexeldeib · 2020-05-22T23:24:27Z

/retest

Signed-off-by: Alexander Eldeib <[email protected]> Co-authored-by: Vince Prignano <[email protected]>

vincepri

One other thing that came to mind when I was reading the conversion is that today files has no checks on whether the path for each added file is actually unique.

Would that cause a configuration error?

vincepri · 2020-05-25T16:03:08Z

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

+			in.ContentFrom = nil
+			in.Content = string(data)
+			collected[i] = in


Just to double check, this logic is only changing the content of the struct at runtime, but it won't be stored in the KubeadmConfig at the end when we patch.

That's the intent. I could probably make this a little bit more explicit by newing up a slice, copying, and returning? Plus comments

vincepri · 2020-05-25T16:06:00Z

/milestone v0.3.7

alexeldeib · 2020-05-25T22:29:06Z

Files not validating path uniqueness is a problem, yes. This PR matches the existing behavior in files which is effectively "last file wins". I can validate uniqueness on path and adjust the conversion logic to account?

The main effect of this right now is loss of conversion fidelity: conversion from v1a3 to v1a2, then adding a raw content file with the same path as a contentFrom file and then converting back to v1a3 would prefer the contentFrom file, even though the raw content file is newer.

alexeldeib · 2020-05-25T22:59:34Z

We should probably also validate that via webhooks at create time

fabriziopandini

@alexeldeib I like very much this new approach
Only one minor not from my side, otherwise lgtm

bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go

vincepri · 2020-05-26T14:08:12Z

@alexeldeib I'm fine doing the validation in a different PR / later

vincepri · 2020-05-26T14:08:43Z

/retitle ✨ CABPK can now read file contents from a Secret

fabriziopandini · 2020-05-26T14:31:20Z

/approve

k8s-ci-robot · 2020-05-26T14:31:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alexeldeib · 2020-05-26T16:09:04Z

@vincepri I'm happy either way. Might make more sense to snapshot as-is and iterate, but the additional code change isn't too big. I'll have a chance to wrap up the additional changes today/tomorrow.

@fabriziopandini thanks for review! 👍

vincepri

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 21, 2020

k8s-ci-robot requested review from fabriziopandini and justinsb May 21, 2020 23:23

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2020

alexeldeib force-pushed the ace/from branch 4 times, most recently from 87a85b7 to 039ea98 Compare May 22, 2020 00:18

vincepri reviewed May 22, 2020

View reviewed changes

alexeldeib force-pushed the ace/from branch 2 times, most recently from 5c977e8 to d006bc0 Compare May 22, 2020 02:46

alexeldeib and others added 3 commits May 21, 2020 19:47

✨ implement api, controllers, and webhook

4f353d3

Co-authored-by: Vince Prignano <[email protected]> Signed-off-by: Alexander Eldeib <[email protected]>

⚒️ add Go tests

3d1345d

⚒️ update generated CRDs

574fd86

alexeldeib force-pushed the ace/from branch from d006bc0 to 574fd86 Compare May 22, 2020 02:48

alexeldeib commented May 22, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 22, 2020

vincepri reviewed May 22, 2020

View reviewed changes

alexeldeib force-pushed the ace/from branch 2 times, most recently from 89831eb to 5c3a0f2 Compare May 22, 2020 22:42

alexeldeib force-pushed the ace/from branch from 5c3a0f2 to d06b3b4 Compare May 22, 2020 22:58

alexeldeib force-pushed the ace/from branch 4 times, most recently from 8f5f288 to 2cff638 Compare May 22, 2020 23:46

⚒️ simplify file resolution, style

9b7df76

Signed-off-by: Alexander Eldeib <[email protected]> Co-authored-by: Vince Prignano <[email protected]>

alexeldeib force-pushed the ace/from branch from 2cff638 to 9b7df76 Compare May 23, 2020 01:37

vincepri reviewed May 25, 2020

View reviewed changes

k8s-ci-robot added this to the v0.3.7 milestone May 25, 2020

fabriziopandini reviewed May 26, 2020

View reviewed changes

bootstrap/kubeadm/api/v1alpha3/kubeadmbootstrapconfig_types.go Show resolved Hide resolved

k8s-ci-robot changed the title ~~✨ enable capbk to read secrets~~ ✨ CABPK can now read file contents from a Secret May 26, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 26, 2020

vincepri approved these changes May 26, 2020

View reviewed changes

k8s-ci-robot assigned vincepri May 26, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2020

k8s-ci-robot merged commit f0a0230 into kubernetes-sigs:master May 26, 2020

alexeldeib mentioned this pull request May 26, 2020

⚒️ fix CAPBK file conversion, add validating webhook #3100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ CABPK can now read file contents from a Secret #3083

✨ CABPK can now read file contents from a Secret #3083

alexeldeib commented May 21, 2020 •

edited

Loading

alexeldeib commented May 21, 2020

alexeldeib commented May 22, 2020

vincepri May 22, 2020

alexeldeib May 22, 2020

vincepri May 22, 2020

alexeldeib May 22, 2020

vincepri commented May 22, 2020

vincepri commented May 22, 2020

alexeldeib commented May 22, 2020

alexeldeib commented May 22, 2020

alexeldeib May 22, 2020

vincepri May 22, 2020

alexeldeib May 22, 2020

vincepri May 25, 2020

CecileRobertMichon May 26, 2020

alexeldeib May 26, 2020

alexeldeib commented May 22, 2020

vincepri May 22, 2020

alexeldeib May 22, 2020

alexeldeib commented May 22, 2020

alexeldeib commented May 22, 2020

vincepri left a comment

vincepri May 25, 2020

alexeldeib May 25, 2020

vincepri commented May 25, 2020

alexeldeib commented May 25, 2020 •

edited

Loading

alexeldeib commented May 25, 2020

fabriziopandini left a comment

vincepri commented May 26, 2020

vincepri commented May 26, 2020

fabriziopandini commented May 26, 2020

k8s-ci-robot commented May 26, 2020

alexeldeib commented May 26, 2020 •

edited

Loading

vincepri left a comment

		// +optional
		Secret *SecretFileSource `json:"secret,omitempty"`

	func (c *KubeadmConfig) validate() error {
	func (c *KubeadmConfigSpec) validate() error {

✨ CABPK can now read file contents from a Secret #3083

✨ CABPK can now read file contents from a Secret #3083

Conversation

alexeldeib commented May 21, 2020 • edited Loading

alexeldeib commented May 21, 2020

alexeldeib commented May 22, 2020

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

vincepri commented May 22, 2020

vincepri commented May 22, 2020

alexeldeib commented May 22, 2020

alexeldeib commented May 22, 2020

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

alexeldeib commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented May 22, 2020

alexeldeib commented May 22, 2020

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented May 25, 2020

alexeldeib commented May 25, 2020 • edited Loading

alexeldeib commented May 25, 2020

fabriziopandini left a comment

Choose a reason for hiding this comment

vincepri commented May 26, 2020

vincepri commented May 26, 2020

fabriziopandini commented May 26, 2020

k8s-ci-robot commented May 26, 2020

alexeldeib commented May 26, 2020 • edited Loading

vincepri left a comment

Choose a reason for hiding this comment

alexeldeib commented May 21, 2020 •

edited

Loading

alexeldeib commented May 25, 2020 •

edited

Loading

alexeldeib commented May 26, 2020 •

edited

Loading