[master] MGMT-21834: remove supported-platforms endpoint#10122
[master] MGMT-21834: remove supported-platforms endpoint#10122andrej1991 wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@andrej1991: This pull request references MGMT-21834 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 bug to target the "4.22.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. |
openshift-ci
Bot
added
the
size/M
WalkthroughThe PR deprecates the ChangesGetClusterSupportedPlatforms Endpoint Deprecation
LibpqDSN Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrej1991 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 |
openshift-ci
Bot
added
the
api-review
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10122 +/- ##
==========================================
+ Coverage 44.24% 44.33% +0.09%
==========================================
Files 415 417 +2
Lines 72499 72840 +341
==========================================
+ Hits 32075 32293 +218
- Misses 37511 37612 +101
- Partials 2913 2935 +22
🚀 New features to boost your workflow:
|
|
/retest |
andrej1991
force-pushed
the
remove_supported_platforms_api
branch
from
7b72e11 to
38a704b
Compare
openshift-ci
Bot
added
size/L
|
@andrej1991: This pull request references MGMT-21834 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 bug to target the "5.0.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. |
|
/retest |
danmanor
left a comment
danmanor
left a comment
There was a problem hiding this comment.
@andrej1991 Are you sure the task is to remove it rather than deprecate it ? cc @CrystalChun / @gamli75
I believe Alona changed it to "remove" since it's not used by QE |
|
@andrej1991 can you remove |
deprecating assisted-install/v2/clusters//supported-platforms endpoint because it gives a bad result and it isn't used by anyone
andrej1991
force-pushed
the
remove_supported_platforms_api
branch
from
38a704b to
bafa3bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd` main go go mod internal.txt:
- Around line 13-16: The code currently passes the raw parser error
(validationErr) to log.WithError which can expose the unredacted DSN; change the
logging to avoid including the raw error object—replace
log.WithError(validationErr).Fatal("Invalid DB connection config") with a
sanitized log such as log.Fatal("Invalid DB connection config") or, if you need
error text, log.WithField("error", validationErr.Error()).Fatal("Invalid DB
connection config") so you do not pass the original validationErr value; update
the call site that handles Options.DBConfig.LibpqDSN() accordingly.
In `@internal/bminventory/inventory_v2_handlers.go`:
- Around line 105-112: The GetClusterSupportedPlatforms handler currently still
returns 200 OK, leaving /v2/clusters/{cluster_id}/supported-platforms callable;
change it so the endpoint is no longer publicly exposed by either removing the
route registration that references GetClusterSupportedPlatforms or updating the
handler GetClusterSupportedPlatforms(ctx, params) to immediately return a non-OK
response (e.g., 404 Not Found or 410 Gone) using the appropriate installer
response constructor (e.g.,
NewGetClusterSupportedPlatformsNotFound/NewGetClusterSupportedPlatformsGone or a
common error responder) and include a short deprecation message in the
payload/log so callers cannot successfully invoke it.
In `@swagger.yaml`:
- Around line 4092-4134: The OpenAPI spec still advertises the deprecated
endpoint /v2/clusters/{cluster_id}/supported-platforms with operationId
GetClusterSupportedPlatforms; remove this path object from swagger.yaml to fully
drop the endpoint from the published contract (delete the entire
/v2/clusters/{cluster_id}/supported-platforms entry including its GET operation,
parameters and responses), and ensure no other references to
GetClusterSupportedPlatforms remain in the spec or related documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0df39486-132a-4858-8074-4731226762dd
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/assisted-service/client/installer/installer_client.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (11)
client/installer/installer_client.gocmd main go go mod internal.txtdocs/user-guide/rest-api-v1-v2-transition-guide.mdinternal/bminventory/inventory.gointernal/bminventory/inventory_test.gointernal/bminventory/inventory_v2_handlers.gopkg/auth/auth_assisted_service_mock_test.gorestapi/configure_assisted_install.gorestapi/embedded_spec.gorestapi/operations/installer/get_cluster_supported_platforms.goswagger.yaml
💤 Files with no reviewable changes (2)
- docs/user-guide/rest-api-v1-v2-transition-guide.md
- internal/bminventory/inventory.go
✅ Files skipped from review due to trivial changes (5)
- restapi/operations/installer/get_cluster_supported_platforms.go
- restapi/configure_assisted_install.go
- client/installer/installer_client.go
- pkg/auth/auth_assisted_service_mock_test.go
- restapi/embedded_spec.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/bminventory/inventory_test.go
| + dbConnectionStr, validationErr := Options.DBConfig.LibpqDSN() | ||
| + if validationErr != nil { | ||
| + log.WithError(validationErr).Fatal("Invalid DB connection config") | ||
| + } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does github.com/jackc/pgx/v5/pgconn ParseConfig return errors that may include parts of the input DSN (including password or full conn string)? Please cite docs/issues/examples.
💡 Result:
Yes, github.com/jackc/pgx/v5/pgconn.ParseConfig returns errors of type *pgconn.ParseConfigError, which includes the full input DSN (ConnString field) and may expose parts of it including passwords or the full connection string [1][2][3]. The Error method includes a redacted version of the ConnString (passwords replaced with xxxxxx via redactPW) in its string output, e.g., "cannot parse postgresql://postgres:xxxxxx@127.0.0.1:5432/postgres: failed to parse as URL..." [4][5][2]. Redaction is best-effort and not perfect for malformed DSNs, as noted in issues; underlying errors (e.g., from url.Parse) may also contain unredacted input [4][6]. The ConnString field itself is exported and unredacted, accessible via type assertion [1]. Examples from issues show password leakage in parse errors [4][5].
Citations:
- 1: https://go.pact.im/doc/use/github.com/jackc/pgx/v5/pgconn..ParseConfigError.html
- 2: https://github.com/jackc/pgx/blob/70f7cad2226dc12406b105f8bb5be9c62780aaf7/pgconn/errors.go
- 3: https://go.pact.im/doc/src/github.com/jackc/pgx/v5/pgconn/errors.go.html
- 4: Sensitive connection string information part of parse error jackc/pgx#1271
- 5: Please help: Error using password in connect string jackc/pgx#1285
- 6: Password not redacted in case of a URL that isn't fully valid jackc/pgconn#51
Avoid logging raw DSN parse errors at fatal level.
The pgconn.ParseConfig error includes the full, unredacted connection string with credentials in its exported ConnString field. While the Error() method provides a redacted version (passwords replaced with xxxxxx), type assertion can access the unredacted value, and underlying errors may bypass redaction entirely. Passing this error to log.WithError() risks exposing database credentials in logs.
Log a sanitized error message and exclude the raw parser error:
dbConnectionStr, validationErr := Options.DBConfig.LibpqDSN()
if validationErr != nil {
- log.WithError(validationErr).Fatal("Invalid DB connection config")
+ log.Fatal("Invalid DB connection config")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd` main go go mod internal.txt around lines 13 - 16, The code currently
passes the raw parser error (validationErr) to log.WithError which can expose
the unredacted DSN; change the logging to avoid including the raw error
object—replace log.WithError(validationErr).Fatal("Invalid DB connection
config") with a sanitized log such as log.Fatal("Invalid DB connection config")
or, if you need error text, log.WithField("error",
validationErr.Error()).Fatal("Invalid DB connection config") so you do not pass
the original validationErr value; update the call site that handles
Options.DBConfig.LibpqDSN() accordingly.
| func (b *bareMetalInventory) GetClusterSupportedPlatforms(ctx context.Context, params installer.GetClusterSupportedPlatformsParams) middleware.Responder { | ||
| logutil.FromContext(ctx, b.log).Warn("deprecated API invoked: GET /v2/clusters/{cluster_id}/supported-platforms; use cluster hosts plus GET /v2/support-levels/features instead") | ||
| supportedPlatforms, err := b.GetClusterSupportedPlatformsInternal(ctx, params) | ||
| if err != nil { | ||
| return common.GenerateErrorResponder(err) | ||
| } | ||
| return installer.NewGetClusterSupportedPlatformsOK().WithPayload(*supportedPlatforms) | ||
| } |
There was a problem hiding this comment.
Endpoint is still publicly exposed instead of removed.
Line 105 adds a public handler and Line 111 still returns 200 OK, so /v2/clusters/{cluster_id}/supported-platforms remains callable. That conflicts with the PR objective to remove this endpoint.
Proposed direction
-func (b *bareMetalInventory) GetClusterSupportedPlatforms(ctx context.Context, params installer.GetClusterSupportedPlatformsParams) middleware.Responder {
- logutil.FromContext(ctx, b.log).Warn("deprecated API invoked: GET /v2/clusters/{cluster_id}/supported-platforms; use cluster hosts plus GET /v2/support-levels/features instead")
- supportedPlatforms, err := b.GetClusterSupportedPlatformsInternal(ctx, params)
- if err != nil {
- return common.GenerateErrorResponder(err)
- }
- return installer.NewGetClusterSupportedPlatformsOK().WithPayload(*supportedPlatforms)
-}
+// Remove this handler and unregister the route in generated/config wiring
+// so the endpoint is no longer served.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/bminventory/inventory_v2_handlers.go` around lines 105 - 112, The
GetClusterSupportedPlatforms handler currently still returns 200 OK, leaving
/v2/clusters/{cluster_id}/supported-platforms callable; change it so the
endpoint is no longer publicly exposed by either removing the route registration
that references GetClusterSupportedPlatforms or updating the handler
GetClusterSupportedPlatforms(ctx, params) to immediately return a non-OK
response (e.g., 404 Not Found or 410 Gone) using the appropriate installer
response constructor (e.g.,
NewGetClusterSupportedPlatformsNotFound/NewGetClusterSupportedPlatformsGone or a
common error responder) and include a short deprecation message in the
payload/log so callers cannot successfully invoke it.
| /v2/clusters/{cluster_id}/supported-platforms: | ||
| get: | ||
| tags: | ||
| - installer | ||
| description: | | ||
| Deprecated. Returns a list of platforms that this cluster can support in its current configuration. | ||
| Prefer deriving platform eligibility from cluster hosts and inventory together with | ||
| GET /v2/support-levels/features (or GET /v2/support-levels/features/detailed) for the cluster OpenShift version and CPU architecture. | ||
| deprecated: true | ||
| operationId: GetClusterSupportedPlatforms | ||
| security: | ||
| - userAuth: [ admin, read-only-admin, user ] | ||
| parameters: | ||
| - in: path | ||
| name: cluster_id | ||
| description: The cluster whose platform types should be retrieved. | ||
| type: string | ||
| format: uuid | ||
| required: true | ||
| responses: | ||
| "200": | ||
| description: Success. | ||
| schema: | ||
| type: array | ||
| items: | ||
| $ref: '#/definitions/platform_type' | ||
| "401": | ||
| description: Unauthorized. | ||
| schema: | ||
| $ref: '#/definitions/infra_error' | ||
| "403": | ||
| description: Forbidden. | ||
| schema: | ||
| $ref: '#/definitions/infra_error' | ||
| "404": | ||
| description: Error. | ||
| schema: | ||
| $ref: '#/definitions/error' | ||
| "500": | ||
| description: Error. | ||
| schema: | ||
| $ref: '#/definitions/error' | ||
|
|
There was a problem hiding this comment.
Endpoint is still exposed instead of removed
Line 4092 keeps /v2/clusters/{cluster_id}/supported-platforms in the published OpenAPI spec. That means the API contract still advertises and supports this known-incorrect endpoint, which conflicts with the stated removal objective for MGMT-21834.
Suggested change
- /v2/clusters/{cluster_id}/supported-platforms:
- get:
- tags:
- - installer
- description: |
- Deprecated. Returns a list of platforms that this cluster can support in its current configuration.
- Prefer deriving platform eligibility from cluster hosts and inventory together with
- GET /v2/support-levels/features (or GET /v2/support-levels/features/detailed) for the cluster OpenShift version and CPU architecture.
- deprecated: true
- operationId: GetClusterSupportedPlatforms
- security:
- - userAuth: [ admin, read-only-admin, user ]
- parameters:
- - in: path
- name: cluster_id
- description: The cluster whose platform types should be retrieved.
- type: string
- format: uuid
- required: true
- responses:
- "200":
- description: Success.
- schema:
- type: array
- items:
- $ref: '#/definitions/platform_type'
- "401":
- description: Unauthorized.
- schema:
- $ref: '#/definitions/infra_error'
- "403":
- description: Forbidden.
- schema:
- $ref: '#/definitions/infra_error'
- "404":
- description: Error.
- schema:
- $ref: '#/definitions/error'
- "500":
- description: Error.
- schema:
- $ref: '#/definitions/error'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /v2/clusters/{cluster_id}/supported-platforms: | |
| get: | |
| tags: | |
| - installer | |
| description: | | |
| Deprecated. Returns a list of platforms that this cluster can support in its current configuration. | |
| Prefer deriving platform eligibility from cluster hosts and inventory together with | |
| GET /v2/support-levels/features (or GET /v2/support-levels/features/detailed) for the cluster OpenShift version and CPU architecture. | |
| deprecated: true | |
| operationId: GetClusterSupportedPlatforms | |
| security: | |
| - userAuth: [ admin, read-only-admin, user ] | |
| parameters: | |
| - in: path | |
| name: cluster_id | |
| description: The cluster whose platform types should be retrieved. | |
| type: string | |
| format: uuid | |
| required: true | |
| responses: | |
| "200": | |
| description: Success. | |
| schema: | |
| type: array | |
| items: | |
| $ref: '#/definitions/platform_type' | |
| "401": | |
| description: Unauthorized. | |
| schema: | |
| $ref: '#/definitions/infra_error' | |
| "403": | |
| description: Forbidden. | |
| schema: | |
| $ref: '#/definitions/infra_error' | |
| "404": | |
| description: Error. | |
| schema: | |
| $ref: '#/definitions/error' | |
| "500": | |
| description: Error. | |
| schema: | |
| $ref: '#/definitions/error' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@swagger.yaml` around lines 4092 - 4134, The OpenAPI spec still advertises the
deprecated endpoint /v2/clusters/{cluster_id}/supported-platforms with
operationId GetClusterSupportedPlatforms; remove this path object from
swagger.yaml to fully drop the endpoint from the published contract (delete the
entire /v2/clusters/{cluster_id}/supported-platforms entry including its GET
operation, parameters and responses), and ensure no other references to
GetClusterSupportedPlatforms remain in the spec or related documentation.
|
@andrej1991: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
remove assisted-install/v2/clusters//supported-platforms endpoint because it gives a bad result and it isn't used by anyone
When querying the endpoint it returns none, however it should returned the supported platforms by the underlying infrastructure.
An example:
$ curl -s -X GET "https://api.openshift.com/api/assisted-install/v2/clusters/$CLUSTER_ID" -H "Content-Type: application/json"
-H "Authorization: Bearer $API_TOKEN"
| jq '.platform'
{
"external": {},
"type": "baremetal"
}
$ curl -s -X GET "https://api.openshift.com/api/assisted-install/v2/clusters/$CLUSTER_ID/supported-platforms" -H "Content-Type: application/json"
-H "Authorization: Bearer $API_TOKEN"
| jq
[
"none"
]
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit
Deprecations
GET /v2/clusters/{cluster_id}/supported-platformsendpoint as deprecated. Use cluster hosts/inventory data combined with support-levels feature endpoints to determine platform eligibility instead.Improvements
Documentation