-
Notifications
You must be signed in to change notification settings - Fork 4
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
CNF-14839: Initial dell-hwmgr adaptor implementation #13
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Don Penney <[email protected]>
@donpenney: This pull request references CNF-14839 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 sub-task to target the "4.18.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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold |
Signed-off-by: Don Penney <[email protected]>
This update extends the HardwareManager CRD to define the configuration attributes necessary for the dell-hmgr adaptor to establish an authenticated connection to a hardware manager, and adds adaptor code to do so. When reconciling a HardwareManager CR, the dell-hwmgr adaptor will create a client and request a bearer token, using information from the CR and corresponding secret, updating the CR Validation condition depending on success or failure in getting an authenticated token. Signed-off-by: Don Penney <[email protected]>
|
||
## Adaptors | ||
DELL_ADAPTOR_DIR ?= adaptors/dell-hwmgr | ||
DELL_OPENAPI ?= $(DELL_ADAPTOR_DIR)/dell-oapi.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.
I am not able to locate this json file as part of this commit/project ... Has this not been included deliberately ?
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.
also... could we store it as yaml instead of json since openapi formatted as json is really difficult to read. please!
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.
It has been intentionally excluded for the time being, as it triggered an alert for "potential data leak" in sample content within the file.
/lgtm |
} | ||
|
||
// GetToken sends a request to the hardware manager to request an authentication token | ||
func (r *HardwareManagerReconciler) GetToken(ctx context.Context, client *hwmgrapi.ClientWithResponses, hwmgr *pluginv1alpha1.HardwareManager) (string, error) { |
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.
So for the auth functionality, you should be able to use the existing library that I used for this similar functionality in the oran-o2ims repo. There shouldn't be any need for us to hand manage the tokens and their expiry/refresh.
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.
... otherwise, each time you make an API call you need to handle a 401 and refresh the token. The oauth library should be able to handle that... since this particular h/w mgr appears to be using a compatible authn/authz strategy.
@@ -69,7 +69,20 @@ type DellData struct { | |||
// +kubebuilder:validation:Required | |||
// +required | |||
// +operator-sdk:csv:customresourcedefinitions:type=spec | |||
User string `json:"user"` | |||
ClientId string `json:"clientId"` |
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.
IMO, the client-id should also be contained in the Secret along with the other pieces of info.
ApiUrl string `json:"apiUrl"` | ||
|
||
// +operator-sdk:csv:customresourcedefinitions:type=spec | ||
CertsConfigMap string `json:"certsConfigMap,omitempty"` |
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.
Might want to rename this to CaBundleConfigMap
since I am assuming this is for a set of custom CA certificates. ...and then internally it should have a ca-bundle.pem
key to align with other similar config maps.
// NewClientWithResponses creates an authenticated client connected to the hardware manager | ||
func (r *HardwareManagerReconciler) NewClientWithResponses(ctx context.Context, hwmgr *pluginv1alpha1.HardwareManager) (*hwmgrapi.ClientWithResponses, error) { | ||
tr := &http.Transport{ | ||
TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, // nolint: gosec |
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 should endeavor not to have this set to true
... since you have the ability to take in a custom CA bundle we should force this to false right from the beginning even though, yes, it causes an extra step in setup.
This update adds the initial implementation of the dell-hwmgr adaptor, including the following: