✨ automatically generate azure.json #802

alexeldeib · 2020-07-17T21:00:49Z

What this PR does / why we need it:
Automatically generates azure-json for machines/machine templates. Unfortunately 1 will be generated for every machine template (or machine not associated to a template) in the current form. This means if a user manually provides the secret to override, we would create two secrets. However with this implementation there are very few scenarios a user would really need to override: the only one I really see is cloud provider rate limiting configuration, which we don't expose at all. (future work 🙂)

Added ~~Also, I haven't actually added any differentiation yet between user-provided secret not matching expected.~~

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
related to #773 but doesn't close, strictly speaking

Special notes for your reviewer:
ready for reviewz ~~added tests, think we just need docs~~ ~~Probably need to add some unit tests and/or other validation, not 100% sure this does all the right things yet. Opening up for discussion since it's already quite big.~~

FYI

squash time ~~I sorted the commits nicely for your reviewing pleasure. I'll keep them sorted as such.~~

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

automatically generate azure.json for machine templates, machines

CecileRobertMichon · 2020-07-17T21:28:22Z

/retitle [WIP] ✨ automatically generate azure.json

just marking this as WIP so it doesn't merge as per PR pr description

alexeldeib · 2020-07-17T21:29:06Z

/test pull-cluster-api-provider-azure-e2e

alexeldeib · 2020-07-17T21:36:32Z

I tried doing this a few different ways, I found it worked best to break it out into a separate controller. For MachineDeployment/MachineSet/KubeadmControlPlane, all machines come from AzureMachineTemplate clones. For these machines we can share 1 secret for all machines in the group, it's wasteful to create 1 secret per machine. We do also need to allow for creating single machines which don't belong to any of those object types -- we filter with a predicate and check that the individual machine isn't a member of any of those resource types listed above which can share common azure json. We also add the associated object as an owner of the newly created secret, to get garbage collection

curious what people think!

alexeldeib · 2020-07-17T22:20:45Z

@CecileRobertMichon thanks 👍 although this is WIP for ~~unit tests +~~ docs, I do think it's quite close to functional completion. I'd appreciate reviews as-is on the approach anyway 🙂

CecileRobertMichon · 2020-07-17T22:38:59Z

controllers/helpers.go

+
+	secret.Data = map[string][]byte{
+		"controlplane": controlPlaneConfig,
+		"node":         nodeConfig,


what are your thoughts on potentially needing to split up the azure.json for node into one for each machine deployment / machine pool in the future? Thinking of use cases where one of the configurations would change per node pool such as having multiple subnets

that's what this does already, it just always generates both controlplane and node versions for even when the MD can only be one or the other. we differentiate by referencing the desired key in the MD kubeadm config

I guess it's pretty easy to use the control plane label to tell them apart and only use one key

we don't actually plumb the per-machinepool subnet through right now because we get it from the cluster spec. But if we switched that to e.g. the machine spec, we could easily adjust the interfaces/params here to make it work.

sorry for stream of consciousness -- I think I remembered the reason I didn't differentiate.

For machines, you can tell if they're control plane, machine set, or machine deployment based. If it's not any of those (i.e., a user created a standalone machine for some reason), you want to catch that and create a single-machine azure json.

For machine templates, the cluster is the owner. We would have to go back to the cluster, get the control plane ref, fetch that object, and check if the control plane uses this machine template or not. BUT there's not really a strict guarantee that the infra ref is KCP, so we can't use our knowledge of KCP schema to look up the azure machine template spec.

The other alternative that would allow is creating azure-json per control plane machine instead of per-machine template for the control plane. That's strictly worse anyway as we end up generating N times as many secrets as required (where N == control plane replicas), vs duplicating the data one time inside the secret data map.

CecileRobertMichon

Other than this lgtm (and we should verify that it doesn't break clusterctl move, haven't enabled that scenario in e2e yet)

hack/log/redact.sh

alexeldeib · 2020-07-28T22:37:07Z

haven't tested with clusterctl move, need to try that (first time!)

CecileRobertMichon · 2020-07-28T22:56:10Z

@alexeldeib from the sounds of #840, clusterctl is probably broken with capz right now 😐

CecileRobertMichon · 2020-07-29T16:46:51Z

lgtm

/assign @devigned @nader-ziada

nader-ziada · 2020-07-29T21:21:07Z

I tried running a few scenarios and seems to work for me

/lgtm

devigned

/lgtm

CecileRobertMichon · 2020-07-29T23:39:52Z

/approve
/pony

k8s-ci-robot · 2020-07-29T23:39:57Z

@CecileRobertMichon:

In response to this:

/approve
/pony

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2020-07-29T23:40:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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~~ [CecileRobertMichon]

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

CecileRobertMichon · 2020-07-29T23:40:55Z

this is not the kind of pony energy I was going for ^

alexeldeib · 2020-07-29T23:53:59Z

/honk

k8s-ci-robot · 2020-07-29T23:54:00Z

@alexeldeib:

In response to this:

/honk

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexeldeib · 2020-07-29T23:54:13Z

now that's what i'm talking about

CecileRobertMichon · 2020-07-30T00:53:21Z

/retest

k8s-ci-robot · 2020-07-30T01:00:34Z

@alexeldeib: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`013374c`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot requested review from devigned and justaugustus July 17, 2020 21:01

k8s-ci-robot added area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jul 17, 2020

alexeldeib force-pushed the ace/cloud branch from bffda0f to 36e3d02 Compare July 17, 2020 21:05

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 17, 2020

alexeldeib force-pushed the ace/cloud branch 4 times, most recently from 7f2f201 to d6f93e8 Compare July 17, 2020 21:25

k8s-ci-robot changed the title ~~✨ automatically generate azure.json~~ [WIP] ✨ automatically generate azure.json Jul 17, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 17, 2020

alexeldeib force-pushed the ace/cloud branch 3 times, most recently from cbccbcd to 2954c26 Compare July 17, 2020 22:18

CecileRobertMichon reviewed Jul 17, 2020

View reviewed changes

alexeldeib force-pushed the ace/cloud branch 6 times, most recently from fd89f9f to 452f615 Compare July 18, 2020 01:36

alexeldeib force-pushed the ace/cloud branch from 31b903e to b15f644 Compare July 28, 2020 18:38

alexeldeib requested review from devigned and CecileRobertMichon July 28, 2020 20:06

CecileRobertMichon suggested changes Jul 28, 2020

View reviewed changes

hack/log/redact.sh Outdated Show resolved Hide resolved

✨ automatically generate azure.json

013374c

alexeldeib force-pushed the ace/cloud branch from b15f644 to 013374c Compare July 28, 2020 22:28

k8s-ci-robot assigned devigned and nader-ziada Jul 29, 2020

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

devigned approved these changes Jul 29, 2020

View reviewed changes

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

k8s-ci-robot merged commit 03df29c into kubernetes-sigs:master Jul 30, 2020

k8s-ci-robot added this to the v0.4.7 milestone Jul 30, 2020

This was referenced Jul 30, 2020

📖 Remove azure.json variable from quick start kubernetes-sigs/cluster-api#3433

Merged

✨ Add single stack IPv6 support #646

Merged

gab-satchi mentioned this pull request Sep 22, 2020

✨ Exclude credentials from worker-nodes' azure.json #955

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ automatically generate azure.json #802

✨ automatically generate azure.json #802

alexeldeib commented Jul 17, 2020 •

edited

Loading

CecileRobertMichon commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020 •

edited

Loading

CecileRobertMichon Jul 17, 2020

alexeldeib Jul 17, 2020

alexeldeib Jul 17, 2020

alexeldeib Jul 17, 2020

alexeldeib Jul 17, 2020 •

edited

Loading

CecileRobertMichon left a comment

alexeldeib commented Jul 28, 2020

CecileRobertMichon commented Jul 28, 2020

CecileRobertMichon commented Jul 29, 2020

nader-ziada commented Jul 29, 2020

devigned left a comment

CecileRobertMichon commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

CecileRobertMichon commented Jul 29, 2020

alexeldeib commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

alexeldeib commented Jul 29, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020 •

edited

Loading

✨ automatically generate azure.json #802

✨ automatically generate azure.json #802

Conversation

alexeldeib commented Jul 17, 2020 • edited Loading

FYI

CecileRobertMichon commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020

alexeldeib commented Jul 17, 2020 • edited Loading

CecileRobertMichon Jul 17, 2020

Choose a reason for hiding this comment

alexeldeib Jul 17, 2020

Choose a reason for hiding this comment

alexeldeib Jul 17, 2020

Choose a reason for hiding this comment

alexeldeib Jul 17, 2020

Choose a reason for hiding this comment

alexeldeib Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

alexeldeib commented Jul 28, 2020

CecileRobertMichon commented Jul 28, 2020

CecileRobertMichon commented Jul 29, 2020

nader-ziada commented Jul 29, 2020

devigned left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

CecileRobertMichon commented Jul 29, 2020

alexeldeib commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

alexeldeib commented Jul 29, 2020

CecileRobertMichon commented Jul 30, 2020

k8s-ci-robot commented Jul 30, 2020 • edited Loading

alexeldeib commented Jul 17, 2020 •

edited

Loading

alexeldeib commented Jul 17, 2020 •

edited

Loading

alexeldeib Jul 17, 2020 •

edited

Loading

k8s-ci-robot commented Jul 30, 2020 •

edited

Loading