Implemented OpenStack token-based authentication#1394
Merged
sarika-pf9 merged 14 commits intomainfrom Jan 22, 2026
Merged
Conversation
bd7c72b to
8ebc8b2
Compare
Contributor
There was a problem hiding this comment.
Other comments (13)
- pkg/common/validation/openstack/validate.go (87-87) There's a potential issue with the token-based authentication flow. If authentication fails but doesn't return an error (which can happen with some invalid tokens), the EndpointLocator might not be properly initialized. Consider adding a check after authentication to verify that the EndpointLocator is properly initialized before proceeding.
- k8s/migration/internal/controller/openstackcreds_controller.go (132-159) The code should also check for and include OS_PROJECT_NAME in the secret data, as it's mentioned in the PR description's rc file example but not handled in the current implementation.
- k8s/migration/internal/controller/openstackcreds_controller.go (132-159) The implementation should also handle OS_IDENTITY_API_VERSION and OS_INTERFACE fields which are mentioned in the PR description's rc file example but not included in the secret data.
- k8s/migration/pkg/sdk/keystone/authenticator.go (75-75) The `Auth` method ignores the `AuthOptions` parameter. Consider handling the options consistently with other authenticator implementations, especially for potential future extensions.
- v2v-helper/openstack/openstackops.go (85-86) When using token-based authentication, `AllowReauth` is set to `false`. This means if the token expires during a long-running operation, the client won't attempt to reauthenticate, potentially causing failures. Consider adding a warning in the documentation about token expiration or implementing a mechanism to handle token refreshes.
- v2v-helper/openstack/openstackops.go (64-70) The function handles `OS_TENANT_NAME` and `OS_PROJECT_NAME`, but doesn't support `OS_PROJECT_ID` which is commonly used in OpenStack RC files. Consider adding support for this variable to improve compatibility with different OpenStack environments.
-
deploy/00crds.yaml (1043-1048)
The new `retryable` field should have a default value specified. For boolean fields in CRDs, it's recommended to set a default to ensure consistent behavior when the field is not explicitly set.
retryable: description: |- Retryable indicates whether this migration can be retried when it fails. Set to false for VMs with RDM (Raw Device Mapping) disks that share storage, as RDM disk migration state prevents automatic retry. type: boolean default: true - pkg/common/validation/openstack/validate.go (87-132) There's significant code duplication in the error handling between token-based and password-based authentication paths. Consider extracting the common error handling logic into a helper function to improve maintainability.
- k8s/migration/pkg/sdk/keystone/keystone.go (429-431) The error message here is specific to 'token info', but this same error handling pattern appears in other functions with similar but contextually different error messages. Consider extracting this pattern into a helper function to ensure consistent error handling across all API calls.
- ui/src/api/helpers.ts (99-105) The credentials object now has optional username, password, and auth token fields, but there's no validation to ensure that at least one authentication method (either username/password pair or auth token) is provided. Consider adding validation to prevent creating invalid credentials.
- k8s/migration/api/v1alpha1/openstackcreds_types.go (88-88) The field `osAuthUrl` uses camelCase for 'Url', but Go convention is to use 'URL' in all caps for acronyms. Consider renaming to `osAuthURL` for consistency with the `AuthURL` field in the `OpenStackCredsInfo` struct.
- k8s/migration/pkg/sdk/keystone/authenticator.go (70-70) The function name `NewStaticTokenGenerator` is misleading as it creates an authenticator, not a token generator. Consider renaming to `NewStaticTokenAuthenticator` for consistency with the type name and to better reflect its purpose.
- k8s/migration/api/v1alpha1/openstackcreds_types.go (88-102) The newly added fields in `OpenstackCredsSpec` are missing documentation comments. Consider adding descriptive comments for each field to maintain consistency with the rest of the codebase and improve API documentation.
💡 To request another review, post a new comment with "/windsurf-review".
dd34a94 to
99f8802
Compare
Collaborator
Author
|
/windsurf-review |
Contributor
There was a problem hiding this comment.
Other comments (5)
- k8s/migration/api/v1alpha1/openstackcreds_types.go (102-102) The `OsInsecure` field is defined as `*bool` while the `Insecure` field in `OpenStackCredsInfo` is a non-pointer `bool`. Consider making these consistent to avoid potential nil pointer issues when mapping between these structures.
- v2v-helper/openstack/openstackops.go (79-79) The implementation is missing support for `OS_USER_DOMAIN_NAME` and `OS_PROJECT_DOMAIN_NAME` environment variables, which are commonly used in OpenStack v3 authentication. Consider adding support for these variables to ensure compatibility with various OpenStack deployments.
- k8s/migration/pkg/utils/bmprovisionerutils.go (17-23) The import order has been changed (netutils and scope imports swapped positions). While this doesn't affect functionality, it would be good to understand if this was intentional or if there's a specific import ordering convention being followed.
- ui/src/utils/openstackRCFileParser.ts (22-26) The exported `REQUIRED_OPENSTACK_FIELDS` array is now redundant as it contains the same values as `COMMON_REQUIRED_FIELDS`. Consider either removing this export and using `COMMON_REQUIRED_FIELDS` directly, or making `REQUIRED_OPENSTACK_FIELDS` reference `COMMON_REQUIRED_FIELDS` to avoid duplication.
- k8s/migration/pkg/sdk/keystone/authenticator.go (74-77) The function name `NewStaticTokenGenerator` is misleading since this authenticator doesn't generate tokens but validates existing ones. Consider removing this redundant function since it just calls `NewStaticTokenAuthenticator` with the same parameters.
💡 To request another review, post a new comment with "/windsurf-review".
2cee69c to
45b3132
Compare
spai-p9
approved these changes
Jan 22, 2026
46fddd9 to
4c11e98
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Which issue(s) this PR fixes
fixes #1315
Testing done
use rc file as:
validated successfully:
