feat: aks provider #482

alexeldeib · 2020-04-02T10:11:33Z

What this PR does / why we need it:
demo for a managed cluster with machine pools. Still a bit rough around the edges but basic scenarios (create, add pool, delete pool, scale) work well. Sharing for some feedback, finally got around to trying the approach from here: kubernetes-sigs/cluster-api#2045 (comment)

xref:
#337
kubernetes-sigs/cluster-api#2045
kubernetes-sigs/cluster-api#980

fyi @vincepri @detiber @jsturtevant for some 👀 to start

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

Special notes for your reviewer:

To test this with Tilt, you can use the normal Azure config + this change in CAPI upstream:

"extra_args": {
    "core": ["--feature-gates=MachinePool=true", "--feature-gates=AKS=true"],
    "kubeadm-bootstrap": ["--feature-gates=MachinePool=true", "--feature-gates=AKS=true"],
    "azure": ["--feature-gates=MachinePool=true", "--feature-gates=AKS=true"]
}

kind create cluster
cd$(GOPATH)/src/sigs.k8s.io/cluster-api 
tilt up

# new terminal
cd$(GOPATH)/src/sigs.k8s.io/cluster-api-provider-azure
make create-aks-cluster

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Adds an AKS provider, implemented via AzureManagedControlPlane and AzureManagedMachinePool custom resources.

cloud/services/managedclusters/managedclusters.go

alexeldeib · 2020-04-02T10:31:06Z

/retest

CecileRobertMichon · 2020-04-02T16:37:42Z

I think we should consider moving this to an /exp folder since it is a POC

alexeldeib · 2020-04-03T20:46:22Z

So I changed to title to reflect that this isn't really a proof of concept, since I'd attempted this once already independently: https://github.com/alexeldeib/caks/blob/ace/private/controllers/azuremanagedmachine_controller.go

I'd consider this PR the "real" attempt to clean everything up and get it usable, especially with machine pool and my learnings from the initial cut.

Summary

Briefly summarizing approach + remaining work for what i'd consider a mergable cut.

This PR introduces two new types, AzureManagedCluster and AzureManagedMachinePool, and their associated controllers. AzureManagedCluster maps fairly directly to the specification for an AKS cluster. For generic Kubernetes configuration, the CAPI Cluster object contains some of the required information, AzureManagedCluster only models the set of options specific to AKS bootstrapping.

AKS requires a default machine pool. We model this as part of the AzureMangedClusterSpec by adding a reference to an AzureManagedMachinePool in the same namespace. We require the user to explicitly define this node pool the same as any other (TODO: consider defaulting).

CAPI machine pools also map nicely into AKS agent pools, so nothing super surprising there. AKS still lets us see the underlying MC resources, and we record all the resulting VMSS nodes into the AzureManagedMachinePool.

This PR does not introduce any defaulting or validation. defaulting + update validation in particular might be something to consider in the future.

Remaining Work

fix pod/service cidr plumbing
ensure all AKS configuration knobs exposed through AzureManagedCluster actually get plumbed through to the AKS cluster
- n.b.: I'm explicitly not saying the immediate goal is to support every permutation of AKS cluster. I'm starting from a fairly stripped down / blessed config.
testing in some shape/form
handle default pool deletion (aka, you can't do this)
investigate webhook authentication failures?

detiber · 2020-04-06T16:31:23Z

@alexeldeib I didn't dive too deep into the implementation yet, but I do wonder if it would provide a better user experience (or at least similar upgrade experience) if this also made use of a control plane provider as well instead of just AzureManagedCluster and AzureManagedMachinePool. I'm not sure if this would require moving the majority of the AzureManagedCluster implementation to an AzureManagedControlPlane and implementing a mostly noop AzureManagedCluster resource, or if it would require some more fine grained separation.

alexeldeib · 2020-04-06T21:28:39Z

@detiber seems reasonable to me. I can shuffle things around and see if I hit any issues.

alexeldeib · 2020-04-23T01:53:10Z

@devigned @CecileRobertMichon I think this works fine as a control plane provider now, and I've cleaned up the CIDR issue. Going to work on testing and flesh out the AKS options, maybe validation on various edge cases after that. Would appreciate any glaring feedback you see, I know it's a big PR already.

vincepri

Reviewed first half of all the changes, mostly nits :)

cloud/interfaces.go

cloud/scope/managedcontrolplane.go

vincepri · 2020-05-07T20:48:25Z

cloud/services/agentpools/agentpools.go

+		return errors.New("expected agent pool specification")
+	}
+
+	if err == nil {


Are we missing the generic err != nil or we don't care about it?

The following paths are a little hard to read, maybe use a case switch with some comments on what the logic is doing?

Nice catch, I originally left it off because we wouldn't care about transient errors but I should have added back when I differentiated between Create and Update.

I'll also add a doc comment that the reason for differentiating the paths is that you can't use the ManagedClusters API to create agent pools on update, only at creation time.

cloud/services/agentpools/agentpools.go

cloud/services/managedclusters/managedclusters.go

vincepri · 2020-05-07T20:57:42Z

cloud/services/managedclusters/managedclusters.go

+func (s *Service) Reconcile(ctx context.Context, spec interface{}) error {
+	managedClusterSpec, ok := spec.(*Spec)
+	if !ok {
+		return errors.New("expected managed cluster specification")
+	}


Same as above, re:avoiding interface

💯 this is vestigial and we are working on a design proposal to change this across the entire project. At this point, it's followed to be consistent with the rest of the repo.

@CecileRobertMichon or @justaugustus may have more background.

Yep right now interfaces are in a bunch of places, let's stay consistent for now - currently working on a proposal to refactor all of them as David said.

vincepri · 2020-05-07T20:58:12Z

cloud/services/managedclusters/managedclusters.go

+		// HACK: set the last octet of the IP to .10
+		// This ensures the dns IP is valid in the service cidr without forcing the user
+		// to specify it in both the Capi cluster and the Azure control plane.
+		// https://golang.org/src/net/ip.go#L48
+		ip[15] = byte(10)
+		dnsIP := ip.String()
+		properties.NetworkProfile.DNSServiceIP = &dnsIP


Open to suggestions 🙂

n.b., consider how defaulting should work if we allow but don't require both dns and service IP as separate values (or if that's valuable).

Could this be something that we might move into a defaulting webhook? And probably some docs I guess

Agreed, I was mostly trying to scope down this PR as I keep finding things to add 🙂

vincepri · 2020-05-07T20:59:09Z

cloud/services/managedclusters/managedclusters.go

+		} else if strings.EqualFold(*managedClusterSpec.NetworkPolicy, "Calico") {
+			properties.NetworkProfile.NetworkPolicy = containerservice.NetworkPolicyCalico
+		} else {
+			return fmt.Errorf("invalid network policy: '%s'. Allowed options are 'calico' and 'azure'", *managedClusterSpec.NetworkPolicy)


Should this be in a validation webhook or kubebuilder enum?

Probably, I'm marking webhooks as work for a followup because this is already a large PR

I'll create issues when this actually merges // make a list in the PR

I did do the Kubebuilder enums, btw, do we currently support clusters without structural schema?

what do you mean?

I added these

cluster-api-provider-azure/exp/api/v1alpha3/azuremanagedcontrolplane_types.go

Line 59 in e09bf5a

// +kubebuilder:validation:Enum=Calico;Azure

Ignore the comment about structural schema, I was confusing that with trivialVersions conversion in Kubebuilder.

Perfect, that should do it!

cloud/services/managedclusters/managedclusters.go

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

CecileRobertMichon · 2020-05-12T22:05:18Z

@alexeldeib is this ready to go?

alexeldeib · 2020-05-12T22:18:16Z

I think so, I'll start breaking things out into separate issues:

e2e testing - [managedcluster] e2e tests #611
validation webhooks
- dns ip inside service CIDR - [managedcluster] validating webhook for DNS IP #612
- combinations of options (networking)
defaulting webhooks - [managedcluster] defaulting webhooks #615
docs - [managed cluster] documentation #613
SP from secret - [multitenancy/managedcluster] enable reading Azure credentials from secret #614

CecileRobertMichon · 2020-05-13T16:26:02Z

@alexeldeib please open the follow up issues and link them to the PR. I'll do a last round review in the meantime. Thanks!

CecileRobertMichon

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2020-05-14T18:04:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib, 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

k8s-ci-robot requested review from awesomenix and nader-ziada April 2, 2020 10:11

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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 2, 2020

alexeldeib force-pushed the ace/caks branch 2 times, most recently from 76eb8fa to 3c4d4e3 Compare April 2, 2020 10:15

alexeldeib commented Apr 2, 2020

View reviewed changes

cloud/services/managedclusters/managedclusters.go Show resolved Hide resolved

alexeldeib force-pushed the ace/caks branch 2 times, most recently from 9952b5c to f8412eb Compare April 2, 2020 10:29

alexeldeib force-pushed the ace/caks branch 2 times, most recently from 2e136ab to e52ad2b Compare April 2, 2020 10:42

alexeldeib changed the title ~~[wip] feat: poc aks provider~~ [wip] feat: aks provider Apr 3, 2020

alexeldeib mentioned this pull request Apr 7, 2020

WIP: add managed kubernetes provider proposal kubernetes-sigs/cluster-api#2045

Closed

alexeldeib force-pushed the ace/caks branch 2 times, most recently from 61971ec to b8244a6 Compare April 18, 2020 21:25

detiber mentioned this pull request Apr 22, 2020

[Umbrella] Support for EKS kubernetes-sigs/cluster-api-provider-aws#1534

Closed

11 tasks

alexeldeib force-pushed the ace/caks branch from b8244a6 to defbb8a Compare April 22, 2020 23:47

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 23, 2020

⚒️ tooling changes

1bc3edd

alexeldeib force-pushed the ace/caks branch 2 times, most recently from b0d1600 to 452d2f3 Compare May 7, 2020 01:22

alexeldeib added 4 commits May 6, 2020 18:32

✨ sample templates

f386084

✨ introduce AKS feature flag

4076b14

✨ add Go structs for AKS

96a61a7

✨ add CRD, RBAC manifests

04d10c5

alexeldeib force-pushed the ace/caks branch from 452d2f3 to 55110ce Compare May 7, 2020 01:38

alexeldeib added 2 commits May 7, 2020 11:31

✨ add azure service integrations, scope

fde4598

✨ implement controllers

451a0ff

alexeldeib force-pushed the ace/caks branch from 55110ce to 451a0ff Compare May 7, 2020 18:32

vincepri reviewed May 7, 2020

View reviewed changes

alexeldeib force-pushed the ace/caks branch from 62871a3 to 41b5ec6 Compare May 8, 2020 21:31

⚒️ cleanup from review feedback

e09bf5a

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

alexeldeib force-pushed the ace/caks branch from 41b5ec6 to e09bf5a Compare May 8, 2020 22:16

CecileRobertMichon mentioned this pull request May 12, 2020

Refactor: Services should not return interfaces #609

Closed

CecileRobertMichon approved these changes May 14, 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 14, 2020

k8s-ci-robot assigned CecileRobertMichon May 14, 2020

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

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

k8s-ci-robot merged commit 4b04993 into kubernetes-sigs:master May 14, 2020

k8s-ci-robot added this to the v0.5 milestone May 14, 2020

cheahjs mentioned this pull request May 21, 2020

wip add gke types Improbable-Archive/cluster-api-provider-gcp#1

Draft

CecileRobertMichon mentioned this pull request Mar 29, 2022

Graduate AzureManagedCluster out of experimental #2204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: aks provider #482

feat: aks provider #482

alexeldeib commented Apr 2, 2020 •

edited

Loading

alexeldeib commented Apr 2, 2020

CecileRobertMichon commented Apr 2, 2020

alexeldeib commented Apr 3, 2020 •

edited

Loading

detiber commented Apr 6, 2020

alexeldeib commented Apr 6, 2020

alexeldeib commented Apr 23, 2020

vincepri left a comment

vincepri May 7, 2020

vincepri May 7, 2020

alexeldeib May 7, 2020

vincepri May 7, 2020

devigned May 8, 2020

CecileRobertMichon May 11, 2020

vincepri May 7, 2020

alexeldeib May 7, 2020

alexeldeib May 7, 2020

vincepri May 8, 2020

alexeldeib May 8, 2020

vincepri May 7, 2020

alexeldeib May 7, 2020

alexeldeib May 7, 2020

alexeldeib May 8, 2020 •

edited

Loading

vincepri May 11, 2020

alexeldeib May 11, 2020

vincepri May 11, 2020

CecileRobertMichon commented May 12, 2020

alexeldeib commented May 12, 2020 •

edited

Loading

CecileRobertMichon commented May 13, 2020

CecileRobertMichon left a comment

k8s-ci-robot commented May 14, 2020

feat: aks provider #482

feat: aks provider #482

Conversation

alexeldeib commented Apr 2, 2020 • edited Loading

alexeldeib commented Apr 2, 2020

CecileRobertMichon commented Apr 2, 2020

alexeldeib commented Apr 3, 2020 • edited Loading

Summary

Remaining Work

detiber commented Apr 6, 2020

alexeldeib commented Apr 6, 2020

alexeldeib commented Apr 23, 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

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

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 May 8, 2020 • edited Loading

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

CecileRobertMichon commented May 12, 2020

alexeldeib commented May 12, 2020 • edited Loading

CecileRobertMichon commented May 13, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 14, 2020

alexeldeib commented Apr 2, 2020 •

edited

Loading

alexeldeib commented Apr 3, 2020 •

edited

Loading

alexeldeib May 8, 2020 •

edited

Loading

alexeldeib commented May 12, 2020 •

edited

Loading