-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HOSTEDCP-2193: documenting --managed-identities-file flag #5199
HOSTEDCP-2193: documenting --managed-identities-file flag #5199
Conversation
@Patryk-Stefanski: This pull request references HOSTEDCP-2193 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 task to target the "4.19.0" version, but no target version was set. In response to this:
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. |
1839f15
to
14424d3
Compare
/lgtm |
/hold |
Waiting to merge this once the old SP logic is cleaned up and the new one is fully implemented and live in our e2es |
OUTPUT_FILE="service-principals.json" | ||
|
||
cat <<EOF > SP_FILE | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have the control plane vs data plane distinction? Are you planning to reuse this file for workload identity for the managed identities on the data plane side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep we will use one file for all the managed-identities we will have to pass in
}, | ||
"managedIdentitiesKeyVault": { | ||
"name": "${KV_NAME}", | ||
"tenantID": "$(az account show --query tenantId -o tsv)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should just be predefined above. Maybe the MI KV can be in a different tenant than the use running this.
14424d3
to
aa302b9
Compare
aa302b9
to
2c5e6d3
Compare
|
||
## Steps | ||
Note: Steps 1-7 set up the environment so that if created in a persistent group they can be | ||
reused for creation of clusters in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the why should be added here, i.e. quota issues.
@Patryk-Stefanski: The following test failed, say
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. |
@@ -4,6 +4,10 @@ set -x | |||
# This creates an Azure HostedCluster with the VNET in its own RG and the NSG in its own NSG. | |||
# The MANAGED_RG_NAME contains all the cloud resources created by the HC creation. | |||
|
|||
# Prerequisites: | |||
# 1. File with control plane managed identities in json format, see steps 5 & 6 in setup_dev_environment.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
# 1. File with control plane managed identities in json format, see steps 5 & 6 in setup_dev_environment.md | |
# 1. JSON file containing control plane service principals, see steps 5 & 6 in setup_dev_environment.md |
DNS_RECORD_NAME=<DNS_RECORD_NAME> | ||
EXTERNAL_DNS_SP_NAME=<EXTERNAL_DNS_SP_NAME> | ||
EXTERNAL_DNS_CREDS=<PATH_TO_FILE_WITH_DNS_CREDS> | ||
DNS_ZONE_NAME="<DNS_RECORD_NAME>.hypershift.azure.devcluster.openshift.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this block and the two below be generic and not point to our actual resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding note to that these are meant for hypershift engineers and that attention should be paid when using these
### 5. Create Service Principals | ||
|
||
```sh | ||
cloudProvider=$(az ad sp create-for-rbac --name "${CLOUD_PROVIDER_SP_NAME}" --create-cert --cert "${CLOUD_PROVIDER_SP_NAME}" --keyvault "${KV_NAME}" --query "{clientID: appId, certificateName: '${CLOUD_PROVIDER_SP_NAME}'}" -o json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just double checking, this automatically gets filled out correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
2c5e6d3
to
14c88b7
Compare
/hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, Patryk-Stefanski 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:
Approvers can indicate their approval by writing |
/lgtm |
[ART PR BUILD NOTIFIER] Distgit: hypershift |
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