Allow skipping Linux profile on managed clusters #3677

maciaszczykm · 2023-06-29T09:13:31Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
In order to be able to not set LinuxProfile in AKS I have changed AzureManagedControlPlane SSHPublicKey to be optional. If it is null then new key will not be autogenerated and LinuxProfile will not be set, if it is set to empty string then new key will be generated just like it was done until now.

See Slack thread: https://kubernetes.slack.com/archives/CEX9HENG7/p1687961825757119

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:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Allow skipping Linux profile on managed clusters

maciaszczykm · 2023-06-29T09:23:59Z

What do you think about this approach?

I was also considering adding another field like AutogenerateSSHKey that would be true by default, but once disabled then SSH key would not be autogenerated. Then we would not be changing PublicSSHKey type but adding new field instead.

Edit: Not sure if the solution from above is possible as we would need to make PublicSSHKey optional anyways.

Edit 2: Another idea that comes to my mind is detecting if there is existing cluster that does not have SSH key, if that is the case then autogenerate could be skipped. As above, we would need to make PublicSSHKey optional.

cc @jackfrancis

codecov · 2023-06-29T12:14:15Z

Codecov Report

Patch coverage: 76.92% and project coverage change: -0.04 ⚠️

Comparison is base (a32e1dc) 54.09% compared to head (49f8572) 54.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3677      +/-   ##
==========================================
- Coverage   54.09%   54.05%   -0.04%     
==========================================
  Files         185      186       +1     
  Lines       18751    18839      +88     
==========================================
+ Hits        10143    10184      +41     
- Misses       8063     8107      +44     
- Partials      545      548       +3

Impacted Files	Coverage Δ
api/v1beta1/azuremanagedcontrolplane_types.go	`20.00% <ø> (ø)`
azure/scope/managedcontrolplane.go	`24.31% <0.00%> (-0.32%)`	⬇️
azure/services/managedclusters/spec.go	`46.70% <84.21%> (+0.71%)`	⬆️
api/v1beta1/azuremanagedcontrolplane_default.go	`96.70% <100.00%> (ø)`
api/v1beta1/azuremanagedcontrolplane_webhook.go	`82.30% <100.00%> (-0.04%)`	⬇️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

api/v1beta1/azuremanagedcontrolplane_types.go

.golangci.yml

api/v1beta1/azuremanagedcontrolplane_types.go

api/v1beta1/azuremanagedcontrolplane_default.go

maciaszczykm · 2023-07-06T14:19:52Z

Is there anything that I can do to push it forward?

/retest

CecileRobertMichon · 2023-07-06T23:51:56Z

Is there anything that I can do to push it forward?

@maciaszczykm I think the remaining open items are:

the gocyclo linter bump discussion
the backwards compatibility of the pointer change
the aks e2e test is failing

Once all of this is resolved, please squash commits

CecileRobertMichon · 2023-07-06T23:52:07Z

/test ls

k8s-ci-robot · 2023-07-06T23:52:09Z

@CecileRobertMichon: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-azure-build
/test pull-cluster-api-provider-azure-ci-entrypoint
/test pull-cluster-api-provider-azure-e2e
/test pull-cluster-api-provider-azure-e2e-aks
/test pull-cluster-api-provider-azure-test
/test pull-cluster-api-provider-azure-verify

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-azure-apidiff
/test pull-cluster-api-provider-azure-apiversion-upgrade
/test pull-cluster-api-provider-azure-capi-e2e
/test pull-cluster-api-provider-azure-conformance
/test pull-cluster-api-provider-azure-conformance-with-ci-artifacts
/test pull-cluster-api-provider-azure-e2e-optional
/test pull-cluster-api-provider-azure-e2e-workload-upgrade
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts
/test pull-cluster-api-provider-azure-windows-containerd-upstream-with-ci-artifacts-serial-slow

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-azure-apidiff
pull-cluster-api-provider-azure-build
pull-cluster-api-provider-azure-ci-entrypoint
pull-cluster-api-provider-azure-e2e
pull-cluster-api-provider-azure-e2e-aks
pull-cluster-api-provider-azure-test
pull-cluster-api-provider-azure-verify

In response to this:

/test ls

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.

CecileRobertMichon · 2023-07-06T23:52:31Z

Also going to start an apiversion upgrade test:

/test pull-cluster-api-provider-azure-apiversion-upgrade

maciaszczykm · 2023-07-07T07:38:37Z

@CecileRobertMichon E2E failure might just be flaky test. I will rerun it a few times as I saw similar thing in my other PRs.

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

nawazkh · 2023-07-11T23:25:06Z

One more question, the implemented UX of this PR looks a little cramped to me(although completely justified). Do you think we could add a new field selfGenerateSSHKey bool (or something analogous to it) to generate a new SSHKey instead of relying on different behaviors of SSHPublicKey = nil and SSHPublicKey = ?

Its just a thought but I am completely ok with this current implementation as well.

maciaszczykm · 2023-07-13T09:28:21Z

@nawazkh I considered adding new field in my comment above:

I was also considering adding another field like AutogenerateSSHKey that would be true by default, but once disabled then SSH key would not be autogenerated. Then we would not be changing PublicSSHKey type but adding new field instead.

But after thinking longer about it, all fields that are part of control plane spec map somehow to AKS fields. This would be used only by CAPZ and it would be used only once during init, after that it would be redundant. I prefer current solution but if you would like then I can change it. Both ways should work.

maciaszczykm · 2023-07-13T10:47:25Z

/retest

nawazkh

Just one final suggestion from my end :) Everything else looks great to me! Thank you

azure/services/managedclusters/spec_test.go

nawazkh · 2023-07-13T18:34:08Z

@nawazkh I considered adding new field in my comment above:

I was also considering adding another field like AutogenerateSSHKey that would be true by default, but once disabled then SSH key would not be autogenerated. Then we would not be changing PublicSSHKey type but adding new field instead.

But after thinking longer about it, all fields that are part of control plane spec map somehow to AKS fields. This would be used only by CAPZ and it would be used only once during init, after that it would be redundant. I prefer current solution but if you would like then I can change it. Both ways should work.

"This would be used only by CAPZ and it would be used only once during init, after that it would be redundant." Makes sense, thanks for sharing the context again!

maciaszczykm · 2023-07-14T08:27:31Z

@nawazkh Done, PTAL.

azure/services/managedclusters/spec_test.go

Co-authored-by: Nawaz Hussain K <[email protected]>

maciaszczykm · 2023-07-14T10:40:47Z

@nawazkh PTAL.

k8s-ci-robot · 2023-07-14T10:42:59Z

@maciaszczykm: The following test 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-apidiff	`49f8572`	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.

nawazkh · 2023-07-14T15:01:14Z

Thank you for putting this together and working through the conversations. Let's get this merged! 🚀
/lgtm

k8s-ci-robot · 2023-07-14T15:01:22Z

LGTM label has been added.

Git tree hash: 60c96b64916993fed31f10f87e0476c2557a5fc3

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2023-07-18T17:44:23Z

[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

nawazkh · 2023-08-07T18:31:05Z

Should this PR be cherry-picked to other release branches ?

CecileRobertMichon · 2023-08-07T18:50:06Z

@nawazkh IMO it's not eligible for cherry-pick because it's a feature

see https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/docs/book/src/developers/releasing.md#release-support

nawazkh · 2023-08-07T21:52:00Z

Makes sense, thank you!

k8s-ci-robot requested review from mboersma and sonasingh46 June 29, 2023 09:13

maciaszczykm marked this pull request as draft June 29, 2023 09:26

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 Jun 29, 2023

maciaszczykm marked this pull request as ready for review June 29, 2023 09:49

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2023

maciaszczykm changed the title ~~Allow skipping Linux profile on managed clusters~~ [WIP] Allow skipping Linux profile on managed clusters Jun 29, 2023

k8s-ci-robot requested review from marosset and willie-yao June 29, 2023 09:49

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2023

maciaszczykm changed the title ~~[WIP] Allow skipping Linux profile on managed clusters~~ Allow skipping Linux profile on managed clusters Jun 29, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 29, 2023

jackfrancis reviewed Jun 29, 2023

View reviewed changes

api/v1beta1/azuremanagedcontrolplane_types.go Show resolved Hide resolved

davidspek reviewed Jun 30, 2023

View reviewed changes

api/v1beta1/azuremanagedcontrolplane_types.go Outdated Show resolved Hide resolved

nojnhuh reviewed Jul 3, 2023

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

nojnhuh reviewed Jul 3, 2023

View reviewed changes

api/v1beta1/azuremanagedcontrolplane_types.go Show resolved Hide resolved

api/v1beta1/azuremanagedcontrolplane_default.go Show resolved Hide resolved

maciaszczykm force-pushed the linux-profile branch from 86de5c6 to f6d5660 Compare July 13, 2023 09:19

maciaszczykm requested review from CecileRobertMichon and nawazkh July 13, 2023 09:28

nawazkh reviewed Jul 13, 2023

View reviewed changes

azure/services/managedclusters/spec_test.go Outdated Show resolved Hide resolved

maciaszczykm force-pushed the linux-profile branch from fe93e3b to 29822b1 Compare July 14, 2023 08:27

nawazkh reviewed Jul 14, 2023

View reviewed changes

azure/services/managedclusters/spec_test.go Outdated Show resolved Hide resolved

maciaszczykm force-pushed the linux-profile branch from 29822b1 to 67d25cd Compare July 14, 2023 10:39

Allow skipping Linux profile on managed clusters

49f8572

Co-authored-by: Nawaz Hussain K <[email protected]>

maciaszczykm force-pushed the linux-profile branch from 67d25cd to 49f8572 Compare July 14, 2023 10:40

k8s-ci-robot assigned nawazkh Jul 14, 2023

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

CecileRobertMichon reviewed Jul 18, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jul 18, 2023

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

k8s-ci-robot merged commit dd4d34c into kubernetes-sigs:main Jul 18, 2023

k8s-ci-robot added this to the v1.11 milestone Jul 18, 2023

maciaszczykm deleted the linux-profile branch July 19, 2023 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow skipping Linux profile on managed clusters #3677

Allow skipping Linux profile on managed clusters #3677

maciaszczykm commented Jun 29, 2023 •

edited

Loading

maciaszczykm commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

maciaszczykm commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

k8s-ci-robot commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

maciaszczykm commented Jul 7, 2023

nawazkh commented Jul 11, 2023

maciaszczykm commented Jul 13, 2023

maciaszczykm commented Jul 13, 2023

nawazkh left a comment

nawazkh commented Jul 13, 2023

maciaszczykm commented Jul 14, 2023

maciaszczykm commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023 •

edited

Loading

nawazkh commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 18, 2023

nawazkh commented Aug 7, 2023

CecileRobertMichon commented Aug 7, 2023

nawazkh commented Aug 7, 2023

Allow skipping Linux profile on managed clusters #3677

Allow skipping Linux profile on managed clusters #3677

Conversation

maciaszczykm commented Jun 29, 2023 • edited Loading

maciaszczykm commented Jun 29, 2023 • edited Loading

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

maciaszczykm commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

k8s-ci-robot commented Jul 6, 2023

CecileRobertMichon commented Jul 6, 2023

maciaszczykm commented Jul 7, 2023

nawazkh commented Jul 11, 2023

maciaszczykm commented Jul 13, 2023

maciaszczykm commented Jul 13, 2023

nawazkh left a comment

Choose a reason for hiding this comment

nawazkh commented Jul 13, 2023

maciaszczykm commented Jul 14, 2023

maciaszczykm commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023 • edited Loading

nawazkh commented Jul 14, 2023

k8s-ci-robot commented Jul 14, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 18, 2023

nawazkh commented Aug 7, 2023

CecileRobertMichon commented Aug 7, 2023

nawazkh commented Aug 7, 2023

maciaszczykm commented Jun 29, 2023 •

edited

Loading

maciaszczykm commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

k8s-ci-robot commented Jul 14, 2023 •

edited

Loading