Validate providerID for user-assigned IDs in webhook #3618

mboersma · 2023-06-09T00:57:02Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Adds ProviderID to the validation of user-assigned identities called by the AzureMachine webhook. Also moves ParseResourceID() to util/azure to avoid circular import errors.

Which issue(s) this PR fixes:

Inspired by this comment.
Refs #3597

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Validate providerID for user-assigned IDs in webhook

mboersma · 2023-06-09T01:04:50Z

api/v1beta1/azuremachine_validation.go

+		}
+		for _, identity := range userAssignedIdentities {
+			if identity.ProviderID != "" {
+				if _, err := arm.ParseResourceID(strings.TrimPrefix(identity.ProviderID, "azure://")); err != nil {


We can't use the azure.ParseResourceID() func added in #3616 because it would cause a circular import error.

The azure.go file itself doesn't import the api package, I wonder if we should move all azure "util" functions to its own package so we can use the same logic in both places

@mboersma thoughts on this?

Yes, I actually tried moving the ParseResourceID() function to util/azure and that would work. I'll try to update this PR later today.

codecov · 2023-06-09T01:10:39Z

Codecov Report

Patch coverage: 76.00% and project coverage change: -0.11 ⚠️

Comparison is base (b6e1474) 53.73% compared to head (ddd2641) 53.62%.

❗ Current head ddd2641 differs from pull request most recent head 8e39922. Consider uploading reports for the commit 8e39922 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3618      +/-   ##
==========================================
- Coverage   53.73%   53.62%   -0.11%     
==========================================
  Files         185      185              
  Lines       18589    18554      -35     
==========================================
- Hits         9988     9949      -39     
- Misses       8059     8063       +4     
  Partials      542      542

Impacted Files	Coverage Δ
azure/defaults.go	`8.06% <ø> (-1.46%)`	⬇️
azure/services/natgateways/spec.go	`4.08% <0.00%> (ø)`
azure/services/virtualmachines/client.go	`0.00% <0.00%> (ø)`
azure/types.go	`16.27% <0.00%> (ø)`
azure/scope/machinepool.go	`32.31% <50.00%> (ø)`
azure/services/scalesetvms/scalesetvms.go	`63.41% <66.66%> (ø)`
api/v1beta1/azuremachine_validation.go	`83.86% <100.00%> (+0.31%)`	⬆️
azure/converters/identity.go	`100.00% <100.00%> (ø)`
azure/scope/machine.go	`51.12% <100.00%> (ø)`
azure/services/scalesets/scalesets.go	`69.83% <100.00%> (ø)`
... and 3 more

... and 3 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.

nawazkh · 2023-06-12T19:17:00Z

/lgtm

k8s-ci-robot · 2023-06-12T19:17:06Z

LGTM label has been added.

Git tree hash: 8530d3e60adc4844637313779ba212f8aec239a9

mboersma · 2023-06-15T16:44:36Z

/milestone v1.10

mboersma · 2023-06-15T18:54:45Z

/hold

I rebased this on #3612 so moving ParseResourceID() wasn't too confusing. I'll rebase again on main after that merges.

Edit: re-rebased, but kept as two commits for now, pending review.

CecileRobertMichon

/lgtm

k8s-ci-robot · 2023-06-19T14:14:03Z

LGTM label has been added.

Git tree hash: d8769afe6e58020cde9d2fecd67043f8d8a62add

mboersma · 2023-06-19T17:22:45Z

/hold cancel

CecileRobertMichon · 2023-06-19T17:24:34Z

/lgtm
/approve

k8s-ci-robot · 2023-06-19T17:24:39Z

LGTM label has been added.

Git tree hash: a1fb2c71aaf86ad764fbe2d9b5c870c132e168c3

k8s-ci-robot · 2023-06-19T17:24:41Z

[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

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

k8s-ci-robot requested review from CecileRobertMichon and sonasingh46 June 9, 2023 00:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 9, 2023

mboersma commented Jun 9, 2023

View reviewed changes

mboersma mentioned this pull request Jun 12, 2023

Allow azure:// prefix when parsing resource IDs #3616

Merged

4 tasks

k8s-ci-robot assigned nawazkh Jun 12, 2023

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

k8s-ci-robot added this to the v1.10 milestone Jun 15, 2023

mboersma force-pushed the is-your-provider-id-valid-sir branch from 0451032 to d7a5b34 Compare June 15, 2023 18:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2023

k8s-ci-robot requested a review from nawazkh June 15, 2023 18:51

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

mboersma force-pushed the is-your-provider-id-valid-sir branch from d7a5b34 to ddd2641 Compare June 15, 2023 19:46

CecileRobertMichon reviewed Jun 19, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jun 19, 2023

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

Validate providerID for user-assigned IDs in webhook

8e39922

mboersma force-pushed the is-your-provider-id-valid-sir branch from ddd2641 to 8e39922 Compare June 19, 2023 16:25

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2023

k8s-ci-robot requested a review from CecileRobertMichon June 19, 2023 16:25

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

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

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

k8s-ci-robot merged commit 41d1a58 into kubernetes-sigs:main Jun 19, 2023

mboersma deleted the is-your-provider-id-valid-sir branch June 19, 2023 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate providerID for user-assigned IDs in webhook #3618

Validate providerID for user-assigned IDs in webhook #3618

mboersma commented Jun 9, 2023 •

edited

Loading

mboersma Jun 9, 2023

CecileRobertMichon Jun 12, 2023 •

edited

Loading

CecileRobertMichon Jun 15, 2023

mboersma Jun 15, 2023

codecov bot commented Jun 9, 2023 •

edited

Loading

nawazkh commented Jun 12, 2023

k8s-ci-robot commented Jun 12, 2023

mboersma commented Jun 15, 2023

mboersma commented Jun 15, 2023 •

edited

Loading

CecileRobertMichon left a comment

k8s-ci-robot commented Jun 19, 2023

mboersma commented Jun 19, 2023

CecileRobertMichon commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

Validate providerID for user-assigned IDs in webhook #3618

Validate providerID for user-assigned IDs in webhook #3618

Conversation

mboersma commented Jun 9, 2023 • edited Loading

mboersma Jun 9, 2023

Choose a reason for hiding this comment

CecileRobertMichon Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon Jun 15, 2023

Choose a reason for hiding this comment

mboersma Jun 15, 2023

Choose a reason for hiding this comment

codecov bot commented Jun 9, 2023 • edited Loading

Codecov Report

nawazkh commented Jun 12, 2023

k8s-ci-robot commented Jun 12, 2023

mboersma commented Jun 15, 2023

mboersma commented Jun 15, 2023 • edited Loading

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 19, 2023

mboersma commented Jun 19, 2023

CecileRobertMichon commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

k8s-ci-robot commented Jun 19, 2023

mboersma commented Jun 9, 2023 •

edited

Loading

CecileRobertMichon Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 9, 2023 •

edited

Loading

mboersma commented Jun 15, 2023 •

edited

Loading