add oidcIssuerProfile to AzureManagedControlPlane #3973

nojnhuh · 2023-09-08T01:13:04Z

What type of PR is this?
/kind feature
/area managedclusters

What this PR does / why we need it:

This PR exposes the AKS oidcIssuerProfile config from AzureManagedControlPlane at spec.oidcIssuerProfile. It also adds status.oidcIssuerProfile which includes the current actual state of enabled and the issuerURL.

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 #2498

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Added `spec.oidcIssuerProfile` to AzureManagedControlPlane

codecov · 2023-09-08T01:26:02Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.54% 🎉

Comparison is base (b082981) 56.26% compared to head (9ecdf98) 56.80%.
Report is 50 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3973      +/-   ##
==========================================
+ Coverage   56.26%   56.80%   +0.54%     
==========================================
  Files         190      188       -2     
  Lines       19447    19242     -205     
==========================================
- Hits        10941    10931      -10     
+ Misses       7877     7677     -200     
- Partials      629      634       +5

Files Changed	Coverage Δ
api/v1beta1/azuremanagedcontrolplane_types.go	`20.00% <ø> (ø)`
api/v1beta1/azuremanagedcontrolplane_default.go	`96.93% <100.00%> (+0.23%)`	⬆️
api/v1beta1/azuremanagedcontrolplane_webhook.go	`88.03% <100.00%> (+0.43%)`	⬆️
azure/scope/managedcontrolplane.go	`22.35% <100.00%> (-1.49%)`	⬇️
azure/services/managedclusters/managedclusters.go	`75.34% <100.00%> (+2.20%)`	⬆️
azure/services/managedclusters/spec.go	`52.95% <100.00%> (+1.72%)`	⬆️

... and 50 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

maciaszczykm

Really nice to have that! I hope it can be included in the incoming release.

CecileRobertMichon · 2023-09-08T22:55:37Z

/milestone v1.11

@maciaszczykm yes that's the plan

CecileRobertMichon · 2023-09-08T23:04:54Z

api/v1beta1/azuremanagedcontrolplane_types.go

+type OIDCIssuerProfileStatus struct {
+	// Enabled is whether the OIDC issuer is enabled.
+	// +optional
+	Enabled *bool `json:"enabled,omitempty"`


this is interesting

It differs a bit from existing patterns we have for setting "read-only" fields (eg. kubelet identity). This is arguably better and more aligned with k8s pattern (spec is desired state and status is read-only actual state) but inconsistent with other fields we already have. I'm not sure about duplicating the enabled field in both spec and status, that strikes me as redundant.

What was the reasoning in including the status field?

Overall this was done in the spirit of #3855. ASO's ManagedCluster type is also structured the same way. One concrete case that having enabled in the status helps with is the webhook validation to return an error when a user tries to disable the OIDC issuer, which is not allowed by AKS.

The field itself is passed through verbatim from CAPZ to the AKS API, where nil means "don't change" and non-nil true and false set it explicitly. That means when enabled is nil in the AzureManagedControlPlane spec, that by itself is not enough to know whether or not the OIDC issuer is actually enabled at that moment. Since AKS (always, AFAICT) populates enabled in its API when the user sends nil, propagating that to the status in AzureManagedControlPlane lets the webhook definitively know the actual state of enabled when validating an update.

What's the advantage of letting the user set the spec field to "nil"? Would the same enforcement be covered by a simpler immutable check (once it's set to true, cannot be unset or set to false)?

overall I'm +1 on making the IssuerURL part of status but I don't love that we're designing a new pattern for enabled/disabled and immutability on the fly that is inconsistent with other API fields which currently have the exact same behavior from an AKS perspective.

Allowing nil means users that have already set this on their clusters via other means won't have this change out from under them when CAPZ upgrades. I believe @mtougeron expressed this kind of concern a while ago around the autoscaler settings.

That's a valid reason for doing it this way. However, if we're going to support that scenario ("users that have already set this on their clusters via other means") we should do this consistently across all fields we support (that's what ASO does) https://github.com/Azure/azure-service-operator/blob/main/v2/api/containerservice/v1api20230201/managed_cluster_types_gen.go#L10167

Would be worth doing a quick audit to see how many fields are not following this pattern currently but could be, and then having an issue to convert them to use the same pattern. It might be the case that we end up wrapping around the ASO types before we get to it but would still be good to understand where the gaps are.

@CecileRobertMichon Are you overall more +1 to leaving this field as a plain non-pointer bool in the spec and letting it default to false in all cases?

And if spec.oidcIssuerProfile is null in the spec, should that always imply spec.oidcIssuerProfile.enabled is false?

@nojnhuh, no the danger with using a plain non-pointer is that the default could change and then we wouldn't know if a user is intentionally disabling it or not setting the field.

To summarize there are two options/patterns possible:

A) The spec field is an optional pointer bool and defaults the value to a sane default when nil. We don't support users setting things outside of the spec, the CAPZ spec is the only source of truth for the object. This is what we do for most fields if not all currently.
B) The spec field is an optional pointer bool, we don't default the value and let "nil" be a valid value to pass to the AKS API. We use a status field to track the "actual" value in Azure. This allows users to set this value outside of the CAPZ spec and not get overwritten, as long as AKS doesn't default/require a value. This is what this PR is doing currently, and what ASO does.

IMO both are valid. I tend to prefer consistency overall because that is less likely to lead users to make mistakes. This is not a hill I'm willing to die on (and I really want to see this PR get merged so I don't want to be standing in the way of that) but my personal preference would be to keep A) for now for consistency with the agreement that B) is what we're trying to evolve towards with a v2 of the API that wraps around ASO types (#3629).

my personal preference would be to keep A) for now for consistency with the agreement that B) is what we're trying to evolve towards with a v2 of the API that wraps around ASO types

Agree this seems like the best path for now. I'll update this.

@CecileRobertMichon This should work the way you described now. Can we still get this in for the v1.11 release?

nojnhuh · 2023-09-13T19:00:21Z

Updated this so it should be more consistent with other fields.

/hold for squash

nojnhuh · 2023-09-13T19:09:37Z

test/e2e/azure_test.go

@@ -839,15 +839,6 @@ var _ = Describe("Workload cluster creation", func() {
 					}
 				})
 			})
-
-			By("modifying OIDC issuer configuration", func() {


If we don't keep the "actual" state in the status, then we have to wait to add this test back until we can use SDK v2 clients in the tests.

nojnhuh · 2023-09-14T17:13:09Z

/assign @CecileRobertMichon

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2023-09-14T17:26:28Z

LGTM label has been added.

Git tree hash: e85602b848fd98ff2bf0047d57d54757826077e0

k8s-ci-robot · 2023-09-14T17:26:30Z

[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

jackfrancis · 2023-09-14T17:45:01Z

api/v1beta1/azuremanagedcontrolplane_default.go

+
+func (m *AzureManagedControlPlane) setDefaultOIDCIssuerProfile() {
+	if m.Spec.OIDCIssuerProfile == nil {
+		m.Spec.OIDCIssuerProfile = &OIDCIssuerProfile{}


I think this thread covers this: #3973 (review)

But, just to clarify, the reason we're doing it this way instead of the way that autoscaler does it above (if user doesn't declare a profile config we omit it from the spec entirely) is because this more explicit "is disabled" implementation removes edge cases from upgrade scenarios?

@jackfrancis this comment summarizes it #3973 (comment)

I think because autoscaler is a little different from other fields in that we want to send nil over the wire to AKS when not defined in the CAPZ spec. OIDC issuer is more like other fields where we always send a non-nil value.

nojnhuh · 2023-09-14T18:07:50Z

squashed!
/hold cancel

jackfrancis · 2023-09-14T18:15:01Z

/lgtm

nojnhuh · 2023-09-14T21:05:57Z

#3970
/retest

k8s-ci-robot · 2023-09-14T21:09:14Z

@nojnhuh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-e2e-aks	`9ecdf98`	link	unknown	`/test pull-cluster-api-provider-azure-e2e-aks`
pull-cluster-api-provider-azure-apidiff	`9ecdf98`	link	false	`/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 CecileRobertMichon and jackfrancis September 8, 2023 01:13

maciaszczykm approved these changes Sep 8, 2023

View reviewed changes

nojnhuh force-pushed the aks-oidc branch from 7430371 to 3559662 Compare September 8, 2023 14:06

k8s-ci-robot added this to the v1.11 milestone Sep 8, 2023

CecileRobertMichon reviewed Sep 8, 2023

View reviewed changes

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

nojnhuh commented Sep 13, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Sep 14, 2023

CecileRobertMichon reviewed Sep 14, 2023

View reviewed changes

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

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

jackfrancis reviewed Sep 14, 2023

View reviewed changes

add oidcIssuerProfile to AzureManagedControlPlane

9ecdf98

nojnhuh force-pushed the aks-oidc branch from ed863fc to 9ecdf98 Compare September 14, 2023 18:07

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

k8s-ci-robot assigned jackfrancis Sep 14, 2023

nojnhuh mentioned this pull request Sep 14, 2023

add spec.networkPluginMode to AzureManagedControlPlane #3988

Merged

4 tasks

k8s-ci-robot merged commit 09c96a7 into kubernetes-sigs:main Sep 14, 2023
11 checks passed

nojnhuh deleted the aks-oidc branch September 14, 2023 22:38

nojnhuh restored the aks-oidc branch September 15, 2023 05:32

nojnhuh deleted the aks-oidc branch September 15, 2023 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add oidcIssuerProfile to AzureManagedControlPlane #3973

add oidcIssuerProfile to AzureManagedControlPlane #3973

nojnhuh commented Sep 8, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

maciaszczykm left a comment

CecileRobertMichon commented Sep 8, 2023

CecileRobertMichon Sep 8, 2023

nojnhuh Sep 8, 2023

CecileRobertMichon Sep 12, 2023 •

edited

Loading

nojnhuh Sep 12, 2023

CecileRobertMichon Sep 12, 2023

nojnhuh Sep 13, 2023

nojnhuh Sep 13, 2023

CecileRobertMichon Sep 13, 2023

nojnhuh Sep 13, 2023

nojnhuh Sep 14, 2023

nojnhuh commented Sep 13, 2023

nojnhuh Sep 13, 2023

nojnhuh commented Sep 14, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

jackfrancis Sep 14, 2023

CecileRobertMichon Sep 14, 2023

nojnhuh Sep 14, 2023

nojnhuh commented Sep 14, 2023

jackfrancis commented Sep 14, 2023

nojnhuh commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

add oidcIssuerProfile to AzureManagedControlPlane #3973

add oidcIssuerProfile to AzureManagedControlPlane #3973

Conversation

nojnhuh commented Sep 8, 2023

codecov bot commented Sep 8, 2023 • edited Loading

Codecov Report

maciaszczykm left a comment

Choose a reason for hiding this comment

CecileRobertMichon commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon Sep 12, 2023 • 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

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

nojnhuh commented Sep 13, 2023

Choose a reason for hiding this comment

nojnhuh commented Sep 14, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojnhuh commented Sep 14, 2023

jackfrancis commented Sep 14, 2023

nojnhuh commented Sep 14, 2023

k8s-ci-robot commented Sep 14, 2023

codecov bot commented Sep 8, 2023 •

edited

Loading

CecileRobertMichon Sep 12, 2023 •

edited

Loading