HOSTEDCP-1826: review Azure API #4963

enxebre · 2024-10-22T11:51:54Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci · 2024-10-22T11:52:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

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

netlify · 2024-10-22T11:53:09Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`9d206c3`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/671f65c2b38dbb0008de8732
😎 Deploy Preview	https://deploy-preview-4963--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

openshift-ci-robot · 2024-10-22T11:55:03Z

@enxebre: This pull request references HOSTEDCP-1826 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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 openshift-eng/jira-lifecycle-plugin repository.

bryan-cox · 2024-10-24T13:25:32Z

/lgtm
/hold
Hold for tests; others to take a look; sync with ARO folks

bryan-cox · 2024-10-24T14:58:21Z

/test e2e-aks
flaky test

bryan-cox · 2024-10-24T17:15:47Z

/test e2e-aks

bennerv · 2024-10-24T17:16:44Z

api/hypershift/v1beta1/azure.go

-	// AvailabilityZone is the failure domain identifier where the VM should be attached to. This must not be specified
-	// for clusters in a location that does not support AvailabilityZone.
+	// availabilityZone is the failure domain identifier where the VM should be attached to.
+	// This must not be specified for clusters in a location that does not support AvailabilityZone because... TODO: why?


because... TODO: why?

It will fail to provision? We should create availability sets instead if not specified?

bennerv · 2024-10-24T17:17:24Z

api/hypershift/v1beta1/azure.go

-	// MachineIdentityID is a user-assigned identity assigned to the VMs used to authenticate with Azure services. This
-	// field is expected to exist under the same resource group as HostedCluster.Spec.Platform.Azure.ResourceGroupName. This
+	// machineIdentityID is a user-assigned identity assigned to the VMs used to authenticate with Azure services. The
+	// identify is expected to exist under the same resource group as HostedCluster.Spec.Platform.Azure.ResourceGroupName. This
 	// user assigned identity is expected to have the Contributor role assigned to it and scoped to the resource group
 	// under HostedCluster.Spec.Platform.Azure.ResourceGroupName.
 	//
 	// If this field is not supplied, the Service Principal credentials will be written to a file on the disk of each VM
 	// in order to be accessible by the cloud provider; the aforementioned credentials provided are the same ones as
 	// HostedCluster.Spec.Platform.Azure.Credentials. However, this is less secure than using a managed identity.
 	//
+	// TODO: What is the valid character set for this field? What about minimum and maximum lengths?
+	//
 	// +optional
 	MachineIdentityID string `json:"machineIdentityID,omitempty"`


I thought we were removing machine identity cc @bryan-cox

From the API, yeah we still need to. I can put in a follow up PR if Alberto doesn't want to do that here. This PR is essentially blocking the cert work.

yes, let's follow up in different PR for that.

bryan-cox · 2024-10-25T00:35:29Z

/retest

miguelsorianod · 2024-10-25T08:36:31Z

api/hypershift/v1beta1/azure.go

 	// +optional
-	AvailabilityZone string `json:"availabilityZone,omitempty"`
+	AvailabilityZone int32 `json:"availabilityZone,omitempty"`


Can we change this to a string?

In CAPZ this is a string https://capz.sigs.k8s.io/reference/v1beta1-api#infrastructure.cluster.x-k8s.io/v1beta1.AzureMachineTemplateResource, as well as in Azure (https://learn.microsoft.com/en-us/rest/api/compute/resource-skus/list?view=rest-compute-2024-07-01&tabs=HTTP#resourceskulocationinfo) instead of an integer. I understand that it currently maps to integers but if validation is desired that can be performed anyway with a string, but we would be consistent with what's in Azure and we also give space in case in the future changes, which is why I suspect Azure / CAPZ have them as strings and not integers too.

I had the same thought, I don't think we can have guarantees that azure will never introduce a different string that can't be mapped to a int. cc @JoelSpeed since you first introduce the change, any objections?

The potential to not be an integer in the future is a fair one

What about using the kube IntOrString type? That would mean we don't have to force users to quote their integers, but, does it make validation harder? Perhaps

What validation can we impose now if we make this a string type, to ensure the zones provided are only "1", "2" and "3" as we currently expect them to be?

What validation can we impose now if we make this a string type, to ensure the zones provided are only "1", "2" and "3" as we currently expect them to be?

Yes. In the future that validation then can then be relaxed if needed

What about using the kube IntOrString type? That would mean we don't have to force users to quote their integers, but, does it make validation harder? Perhaps

For me a string would be just fine. I don't see the need of coupling it to kube IntOrString or add more complexity. In Azure/CAPZ they don't do that.

ok thanks! I choose to go with a string for simplicity, future flexibility while still keeping validation UX

bryan-cox · 2024-10-28T12:17:34Z

/lgtm

enxebre · 2024-10-28T12:19:15Z

/hold cancel

enxebre · 2024-10-28T12:44:34Z

/retest

enxebre · 2024-10-28T12:46:46Z

/hold
to ship #4872 first so it can be consumed without this changes

enxebre · 2024-10-28T17:04:58Z

/hold cancel

openshift-ci-robot · 2024-10-28T20:08:52Z

/retest-required

Remaining retests: 0 against base HEAD a95fc46 and 2 for PR HEAD 47eabe9 in total

bryan-cox · 2024-10-28T20:08:54Z

/test e2e-aws-4-17

openshift-ci · 2024-10-28T21:56:06Z

@enxebre: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2024-10-28T23:45:30Z

[ART PR BUILD NOTIFIER]

Distgit: hypershift
This PR has been included in build ose-hypershift-container-v4.18.0-202410282308.p0.g9aafe83.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci bot added the do-not-merge/needs-area label Oct 22, 2024

openshift-ci bot requested review from hasueki and isco-rodriguez October 22, 2024 11:52

openshift-ci bot added area/cli Indicates the PR includes changes for CLI area/documentation Indicates the PR includes changes for documentation labels Oct 22, 2024

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels Oct 22, 2024

enxebre mentioned this pull request Oct 22, 2024

[WIP] HOSTEDCP-1826: API review and feedback for Azure APIs #4536

Closed

4 tasks

enxebre changed the title ~~Api review~~ HOSTEDCP-1826: review Azure API Oct 22, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 22, 2024

enxebre force-pushed the api-review branch from fd04d50 to 66c3f52 Compare October 22, 2024 12:59

bryan-cox mentioned this pull request Oct 23, 2024

OCPBUGS-42434: Update Azure CLI to Handle Authenticating with Service Principal Backed by Certificates #4877

Merged

4 tasks

enxebre force-pushed the api-review branch 2 times, most recently from e1c8643 to 8d9735e Compare October 24, 2024 10:42

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

enxebre force-pushed the api-review branch from 8d9735e to 275d054 Compare October 24, 2024 10:42

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024

enxebre force-pushed the api-review branch 3 times, most recently from 4acaf33 to fe47f8f Compare October 24, 2024 12:36

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2024

openshift-ci bot assigned bryan-cox Oct 24, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2024

bennerv reviewed Oct 24, 2024

View reviewed changes

miguelsorianod reviewed Oct 25, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2024

enxebre added 2 commits October 28, 2024 11:18

Add azure API changes

cccafb4

Run make update

2612a9c

enxebre force-pushed the api-review branch from fe47f8f to 9d206c3 Compare October 28, 2024 10:21

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2024

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024

openshift-ci bot added the area/api Indicates the PR includes changes for the API label Oct 28, 2024

Fix consumption of new fields

47eabe9

enxebre force-pushed the api-review branch from 9d206c3 to 47eabe9 Compare October 28, 2024 11:03

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 28, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024

openshift-merge-bot bot merged commit 9aafe83 into openshift:main Oct 28, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOSTEDCP-1826: review Azure API #4963

HOSTEDCP-1826: review Azure API #4963

enxebre commented Oct 22, 2024

openshift-ci bot commented Oct 22, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

openshift-ci-robot commented Oct 22, 2024 •

edited by openshift-ci bot

Loading

bryan-cox commented Oct 24, 2024

bryan-cox commented Oct 24, 2024

bryan-cox commented Oct 24, 2024

bennerv Oct 24, 2024

bennerv Oct 24, 2024

bryan-cox Oct 24, 2024

enxebre Oct 28, 2024

bryan-cox commented Oct 25, 2024

miguelsorianod Oct 25, 2024 •

edited

Loading

enxebre Oct 25, 2024

JoelSpeed Oct 25, 2024

miguelsorianod Oct 25, 2024

miguelsorianod Oct 25, 2024

enxebre Oct 28, 2024

bryan-cox commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

openshift-ci-robot commented Oct 28, 2024

bryan-cox commented Oct 28, 2024

openshift-ci bot commented Oct 28, 2024

openshift-bot commented Oct 28, 2024

HOSTEDCP-1826: review Azure API #4963

HOSTEDCP-1826: review Azure API #4963

Conversation

enxebre commented Oct 22, 2024

openshift-ci bot commented Oct 22, 2024

netlify bot commented Oct 22, 2024 • edited Loading

✅ Deploy Preview for hypershift-docs ready!

openshift-ci-robot commented Oct 22, 2024 • edited by openshift-ci bot Loading

bryan-cox commented Oct 24, 2024

bryan-cox commented Oct 24, 2024

bryan-cox commented Oct 24, 2024

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

bryan-cox commented Oct 25, 2024

miguelsorianod Oct 25, 2024 • 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

bryan-cox commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

enxebre commented Oct 28, 2024

openshift-ci-robot commented Oct 28, 2024

bryan-cox commented Oct 28, 2024

openshift-ci bot commented Oct 28, 2024

openshift-bot commented Oct 28, 2024

netlify bot commented Oct 22, 2024 •

edited

Loading

openshift-ci-robot commented Oct 22, 2024 •

edited by openshift-ci bot

Loading

miguelsorianod Oct 25, 2024 •

edited

Loading