Skip to content
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

Raise an exception for unknown CSPs #1207

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

jesusbv
Copy link
Collaborator

@jesusbv jesusbv commented Aug 19, 2024

Description

Instead of saving nil or empty value on the DB for system_token field, raise an exception if the CSP value from the metadata is unknown

How to test

Create a system with an unknown CSP, request should fail

Change Type

Please select the correct option.

  • Bug Fix (a non-breaking change which fixes an issue)
  • New Feature (a non-breaking change which adds new functionality)
  • Documentation Update (a change which only updates documentation)

Checklist

Please check off each item if the requirement is met.

  • I have reviewed my own code and believe that it's ready for an external review.
  • I have provided comments for any hard-to-understand code.
  • I have documented the MANUAL.md file with any changes to the user experience.
  • If my changes are non-trivial, I have added a changelog entry to notify users at package/obs/rmt-server.changes.

Review

Please check out our review guidelines
and get in touch with the author to get a shared understanding of the change.

@jesusbv jesusbv requested a review from digitaltom August 19, 2024 12:46
@jesusbv jesusbv self-assigned this Aug 19, 2024
@jesusbv jesusbv requested a review from rjschwei August 19, 2024 12:46
unless INSTANCE_ID_KEYS.key?(id_key)
error_message = 'Unknown cloud provider'
Rails.logger.error error_message
raise ActionController::TranslatedError.new(error_message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I am unaware of any conversation at the business level where a decision was made that 3rd party CSPs that run their own update infrastructure are not allowed to offer BYOS pass through.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the CSP is unknown we would not have a valid value for system_token

do we not care about that ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what the system_token has to do with this change. Nor what system_token has to do with now breaking functionality for 3rd party CSPs that previously probably worked when disable_system_tokens: true is set in the rmt.conf file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are setting the system_token for BYOS systems and we use system_token to get the system

The method get_instance_id is the method that sets that value

@rjschwei rjschwei mentioned this pull request Aug 19, 2024
7 tasks
@jesusbv jesusbv marked this pull request as draft August 19, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants