Support APIServerAccessProfile in AzureManagedControlPlane #1640

richardchen331 · 2021-08-21T20:09:10Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds APIServerAccessProfile support in AzureManagedControlPlane.

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

Special notes for your reviewer:

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

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Allow specifying APIServerAccessProfile in AzureManagedControlPlane.

k8s-ci-robot · 2021-08-21T20:09:18Z

Hi @richardchen331. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

mboersma · 2021-08-23T17:55:14Z

/ok-to-test

richardchen331 · 2021-08-23T21:50:24Z

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

CecileRobertMichon · 2021-08-24T18:15:03Z

/assign @alexeldeib @LochanRn @nprokopic

LochanRn · 2021-08-26T05:37:14Z

lgtm
@alexeldeib @nprokopic ManagedClusterAPIServerAccessProfile also provides config to create private clusters I was thinking adding those set of features will also be great WDYT ?

LochanRn · 2021-08-27T18:51:27Z

/lgtm

LochanRn · 2021-08-27T18:53:51Z

@richardchen331 Great Job !!

WDYT about adding spec for private clusters ?

richardchen331 · 2021-08-27T22:19:53Z

Thanks @mboersma @LochanRn ! Sure, let me add spec for private clusters as well.

nprokopic · 2021-08-31T14:29:30Z

exp/api/v1alpha3/azuremanagedcontrolplane_types.go

+	AuthorizedIPRanges []string `json:"authorizedIPRanges,omitempty"`
+	// EnablePrivateCluster - Whether to create the cluster as a private cluster or not.
+	// +optional
+	EnablePrivateCluster bool `json:"enablePrivateCluster,omitempty"`


If this field is optional, I believe a bool pointer *bool would be better. Same for EnablePrivateClusterPublicFQDN. WDYT?

@nprokopic Thanks for your feedback! I'm pretty new to golang and cluster api, and my (limited) understanding of using a pointer is that we could differentiate between a zero value and a not specified value, but then we have to deal with nil. For EnablePrivateCluster, I think explicitly passing in a zero value (false) is equivalent to not passing it (both tells Azure to not enable private cluster). Therefore I'm not aware of the benefit of using a pointer.

Could you help explain a bit more why a bool pointer is better? I think I must have missed something and I'd love to learn :) Thank you!

It is part of the k8s API conventions:

Therefore, we ask that pointers always be used with optional fields that do not have a built-in nil value.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

Got it, thanks for the reference! I'll change the fields to use pointers.

It is part of the k8s API conventions

I vaguely remembered something like that, and that we had a similar discussion for some other CAPZ type some time ago, just couldn't find it.

Thanks Cecile for referencing this.

LochanRn · 2021-09-02T05:35:54Z

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

nprokopic · 2021-09-08T16:35:18Z

exp/api/v1alpha4/azuremanagedcontrolplane_default.go

+
+// setDefaultAPIServerAccessProfile sets the default APIServerAccessProfile for an AzureManagedControlPlane.
+func (r *AzureManagedControlPlane) setDefaultAPIServerAccessProfile() {
+	if r.Spec.APIServerAccessProfile == nil {


nit: Is this required? Wouldn't just leaving nil also work?

@CecileRobertMichon are there some other examples of setting default values like this?

You are right, I don't think this is required. (I was thinking setting it so there will be less working in validation webhook, but i don't think this is a good idea, since it's not good for unit test)

exp/api/v1alpha4/azuremanagedcontrolplane_webhook.go

richardchen331 · 2021-09-13T23:28:40Z

/retest

LochanRn · 2021-09-15T14:20:01Z

@richardchen331 can you please also update the managedclusters documentation about APIServerAccessProfile.

@alexeldeib over all this PR looks fine to me now. Over to you for a final review.

richardchen331 · 2021-09-15T19:13:14Z

@richardchen331 can you please also update the managedclusters documentation about APIServerAccessProfile.

@alexeldeib over all this PR looks fine to me now. Over to you for a final review.

Sure. I'll rebase and update the documentation after the other open PR (#1680) is merged since they modify similar set of files :)

richardchen331 · 2021-09-21T22:13:48Z

@alexeldeib I just rebased this PR and also added documentation. Could you help take a final look? Thanks!

richardchen331 · 2021-09-21T23:58:59Z

/retest

richardchen331 · 2021-09-22T03:04:45Z

/retest

richardchen331 · 2021-09-22T16:26:15Z

/retest

richardchen331 · 2021-09-24T04:07:10Z

/retest

alexeldeib · 2021-09-29T16:14:39Z

azure/types.go

+	// PrivateDNSZone - Private dns zone mode for private cluster.
+	PrivateDNSZone *string
+	// EnablePrivateClusterPublicFQDN - Whether to create additional public FQDN for private cluster or not.
+	EnablePrivateClusterPublicFQDN *bool


I believe this will default to true on AKS side if private cluster is enabled. should we let that happen, or default it inside CAPZ?

it would only mean that a user who explicitly sets it would trigger a no-op reconcile, and a user who wants to change it will be able to.

Thanks for the feedback! I would slightly prefer letting AKS to handle the default. My rationales are:

This is true for other fields as well (e.g. managedOutboundIPs, sku tier)

We need to add some logic when choosing the default values (e.g. set EnablePrivateClusterPublicFQDN to true when EnablePrivateCluster is true)

Given 1,2, this will add some extra complexity to the CAPZ code
The downside is that there will be some unnecessary no-op reconciles, but i feel that's probably fine?

I'm open to change it if you feel strongly about it :)

Fine w me, I asked because I know this defaulting to true has surprised some folks in the past

But that’s really an aks issue

alexeldeib · 2021-09-29T16:16:00Z

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

probably related to the failures we've been discussing. I chatted a bit with @LochanRn about watch issues unrelated to the AKS backend issue, we still have some debugging to do

alexeldeib · 2021-09-29T16:16:22Z

/approve
/lgtm

(adding both since this was already approved)

one minor comment but not a blocker

k8s-ci-robot · 2021-09-29T16:16:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeldeib

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

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

richardchen331 · 2021-10-01T18:11:24Z

/retest

richardchen331 · 2021-10-01T21:28:30Z

/retest

alexeldeib · 2021-10-01T22:37:34Z

/lgtm

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 21, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 21, 2021

k8s-ci-robot requested review from juan-lee and mboersma August 21, 2021 20:09

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 21, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 23, 2021

richardchen331 force-pushed the api_server_access_profile branch from d995729 to 6b1b825 Compare August 23, 2021 18:58

k8s-ci-robot assigned alexeldeib, LochanRn and nprokopic Aug 24, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2021

richardchen331 force-pushed the api_server_access_profile branch from 06effc8 to 3ea6026 Compare August 31, 2021 05:06

nprokopic reviewed Aug 31, 2021

View reviewed changes

richardchen331 requested a review from nprokopic August 31, 2021 16:45

nprokopic reviewed Sep 8, 2021

View reviewed changes

alexeldeib mentioned this pull request Sep 11, 2021

Allow user to specify LoadBalancerProfile in AzureManagedControlPlaneSpec #1680

Merged

3 tasks

richardchen331 force-pushed the api_server_access_profile branch from c2955ef to 590993a Compare September 13, 2021 21:42

richardchen331 requested a review from alexeldeib September 13, 2021 21:44

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2021

richardchen331 force-pushed the api_server_access_profile branch from 590993a to 0e0453e Compare September 21, 2021 22:12

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

alexeldeib reviewed Sep 29, 2021

View reviewed changes

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

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

jsturtevant mentioned this pull request Sep 29, 2021

E2E failure: cannot get remote client to workload cluster #1114

Closed

Support APIServerAccessProfile in AzureManagedControlPlane

2707d8d

richardchen331 force-pushed the api_server_access_profile branch from 0e0453e to 2707d8d Compare October 1, 2021 19:04

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2021

k8s-ci-robot merged commit df75af2 into kubernetes-sigs:main Oct 1, 2021

k8s-ci-robot added this to the v0.5 milestone Oct 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support APIServerAccessProfile in AzureManagedControlPlane #1640

Support APIServerAccessProfile in AzureManagedControlPlane #1640

richardchen331 commented Aug 21, 2021 •

edited

Loading

k8s-ci-robot commented Aug 21, 2021

mboersma commented Aug 23, 2021

richardchen331 commented Aug 23, 2021

CecileRobertMichon commented Aug 24, 2021

LochanRn commented Aug 26, 2021

LochanRn commented Aug 27, 2021

LochanRn commented Aug 27, 2021

richardchen331 commented Aug 27, 2021

nprokopic Aug 31, 2021

richardchen331 Aug 31, 2021

CecileRobertMichon Sep 7, 2021

richardchen331 Sep 8, 2021

nprokopic Sep 8, 2021

LochanRn commented Sep 2, 2021

nprokopic Sep 8, 2021

richardchen331 Sep 8, 2021

richardchen331 commented Sep 13, 2021

LochanRn commented Sep 15, 2021

richardchen331 commented Sep 15, 2021 •

edited

Loading

richardchen331 commented Sep 21, 2021

richardchen331 commented Sep 21, 2021

richardchen331 commented Sep 22, 2021

richardchen331 commented Sep 22, 2021

richardchen331 commented Sep 24, 2021

alexeldeib Sep 29, 2021

richardchen331 Sep 29, 2021

alexeldeib Sep 29, 2021 •

edited

Loading

alexeldeib Sep 29, 2021

alexeldeib commented Sep 29, 2021

alexeldeib commented Sep 29, 2021

k8s-ci-robot commented Sep 29, 2021

richardchen331 commented Oct 1, 2021

richardchen331 commented Oct 1, 2021

alexeldeib commented Oct 1, 2021

Support APIServerAccessProfile in AzureManagedControlPlane #1640

Support APIServerAccessProfile in AzureManagedControlPlane #1640

Conversation

richardchen331 commented Aug 21, 2021 • edited Loading

k8s-ci-robot commented Aug 21, 2021

mboersma commented Aug 23, 2021

richardchen331 commented Aug 23, 2021

CecileRobertMichon commented Aug 24, 2021

LochanRn commented Aug 26, 2021

LochanRn commented Aug 27, 2021

LochanRn commented Aug 27, 2021

richardchen331 commented Aug 27, 2021

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

LochanRn commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardchen331 commented Sep 13, 2021

LochanRn commented Sep 15, 2021

richardchen331 commented Sep 15, 2021 • edited Loading

richardchen331 commented Sep 21, 2021

richardchen331 commented Sep 21, 2021

richardchen331 commented Sep 22, 2021

richardchen331 commented Sep 22, 2021

richardchen331 commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Sep 29, 2021

alexeldeib commented Sep 29, 2021

k8s-ci-robot commented Sep 29, 2021

richardchen331 commented Oct 1, 2021

richardchen331 commented Oct 1, 2021

alexeldeib commented Oct 1, 2021

richardchen331 commented Aug 21, 2021 •

edited

Loading

richardchen331 commented Sep 15, 2021 •

edited

Loading

alexeldeib Sep 29, 2021 •

edited

Loading