-
Notifications
You must be signed in to change notification settings - Fork 117
[DRAFT] ARO-15052: support cluster credential retrieval for aro_hcp/v1alpha1 #1050
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
base: main
Are you sure you want to change the base?
Conversation
|
@JameelB: This pull request references ARO-15052 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 epic to target the "4.20.0" version, but no target version was set. DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JameelB The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @JameelB. Thanks for your PR. I'm waiting for a openshift-online member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
@JameelB: This pull request references ARO-15052 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 epic to target the "4.20.0" version, but no target version was set. DetailsIn 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. |
|
/ok-to-test |
model/clusters_mgmt/v1/break_glass_credential_status_details_type.model
Outdated
Show resolved
Hide resolved
|
@JameelB: This pull request references ARO-15052 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 epic to target the "4.20.0" version, but no target version was set. DetailsIn 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. |
model/aro_hcp/v1alpha1/break_glass_credential_status_details_type.model
Outdated
Show resolved
Hide resolved
| } | ||
| }, | ||
| "responses": { | ||
| "201": { |
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.
Is the method async? If so, should we be returning 202 instead
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.
yes its async, but I'm waiting on #1049 to get merged so i can generate the async add method. That PR updates the metamodel that includes support for that.
You'll notice below that Delete is the same. I'm going to regenerate this once that pr is merged.
914c189 to
acffd9c
Compare
…tial type in clusters_mgmt/v1
acffd9c to
30035c1
Compare
30035c1 to
2e01e50
Compare
| class BreakGlassCredentialStatusDetails { | ||
| // The current state of the credential | ||
| State BreakGlassCredentialState | ||
|
|
||
| // A descriptive message providing additional context about the current state of | ||
| // the credential | ||
| Message String | ||
| } |
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.
The initial proposal for this property was as follows:
"status_details": {
"message": "cert failed to be issued for x reason"
}
However, after reviewing the node pool state, I'm proposing to align this with the NodePoolStatus and NodePoolState types to have consistency as to how the status/state is defined across the API.
Instead, the new property will be as follows:
"status_details": {
"kind": "string",
"id": "string",
"href": "string",
"message": "string",
"state": {
"last_updated_timestamp": "2025-06-13T12:43:34.624Z",
"value": "issuing"
}
},
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.
I agree with making this a class, but should it be named status instead of status_details? I haven't seen the use of a status_details struct before, so this seems to add additional disparities between 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.
I agree with making this a
class, but should it be namedstatusinstead ofstatus_details? I haven't seen the use of astatus_detailsstruct before, so this seems to add additional disparities between resources.
You are correct pointing out this @vkareh, I guess here the issue is that there is a "status" attribute in the top-level of the break glass api resource already, with its type being an enum (a typed string in Go). See model/clusters_mgmt/break_glass_credential_type.model file in the main branch of this repo
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.
+1 it would be nicer to have it as status but as Miguel mentioned it would be difficult to change as we will be switching it from a string to a struct in an existing api used by rosa hcp. The status_details was proposed as an alternative.
The other option was using state as a struct but this isn't consistent with how we use this field in other resources, where it is either a string that indicates the status of the resource, or a struct that contains the value of the state along with other state metadata like last_updated_timestamp.
Accepting any alternatives here 🙏
| // Representation of the state of a break glass credential | ||
| struct BreakGlassCredentialState { | ||
| // A string value representing the credential's current state | ||
| Value BreakGlassCredentialStatus |
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.
Set this to type BreakGlassCredentialStatus to align with the outer Status since it's an enum. Alternatively, we could set this to a string type which would make it a little bit more flexible to add new states later on.
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.
Would BreakGlassCredentialStatusValue or BreakGlassCredentialStateValue be better alternatives? That way we can use BreakGlassCredentialStatus for the credentials.status struct.
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.
Sounds good, will update this to BreakGlassCredentialStatusValue
|
@JameelB: This pull request references ARO-15052 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 epic to target the "4.20.0" version, but no target version was set. DetailsIn 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. |
| */ | ||
|
|
||
| // Manages a specific break glass credential. | ||
| resource BreakGlassCredential { |
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.
Not sure if it has been brought up, but the "break-glass" moniker no longer holds, since these are not "break glass" in the sense that they're only used as an emergency when trying to access/debug a cluster. Any chance we can rename the ARO API to be Credential or AdminCredential?
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.
We can, as long as we're in agreement of the name change. It wasn't a requirement before, but happy to change it. I don't think it should break anything as these are new endpoints 🤔
| class BreakGlassCredentialStatusDetails { | ||
| // The current state of the credential | ||
| State BreakGlassCredentialState | ||
|
|
||
| // A descriptive message providing additional context about the current state of | ||
| // the credential | ||
| Message String | ||
| } |
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.
I agree with making this a class, but should it be named status instead of status_details? I haven't seen the use of a status_details struct before, so this seems to add additional disparities between resources.
| // Representation of the state of a break glass credential | ||
| struct BreakGlassCredentialState { | ||
| // A string value representing the credential's current state | ||
| Value BreakGlassCredentialStatus |
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.
Would BreakGlassCredentialStatusValue or BreakGlassCredentialStateValue be better alternatives? That way we can use BreakGlassCredentialStatus for the credentials.status struct.
|
PR needs rebase. DetailsInstructions 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. |

h4. What
The design document for this API can be found in XCMSTRAT-1135.
The proposed endpoints in ARO HCP v1alpha1 is as follows:
The break glass credentials API for both clusters_mgmt/v1 and aro_hcp/v1alpha1 should have similar features apart from the following differences:
issuing,revokinganddeniedA few additional properties has been added to clusters_mgmt/v1
BreakGlassCredentialstype:CreationTimestamp: added in order to determine when to clean up 'denied' and 'failed' credentials. It may also be useful for auditing/tracing.
For existing credentials, these will be set to a default of zero time. These will be omitted in the API output. Moving forward, all break glass credentials under both clusters_mgmt/v1 and aro_hcp/v1alpha1 will have this property set.
StatusDetails: added to contain additional properties about the break glass credential's state such as 'message'. As there is already an existing
Statusproperty, we can't align its name with the rest of the other endpoints like Node Pools to capture this information. A few alternative options on naming were considered such as 'State' or 'StatusContext'.Additional endpoints that we may consider adding:
Only the openapi files have been generated for now to make it easier to review the changes here.
Resulting changes in the ARO HCP API
