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

Add ULN vs normal URL type validation when updating or creating a remote #3520

Closed
quba42 opened this issue Apr 26, 2024 · 4 comments · Fixed by #3558
Closed

Add ULN vs normal URL type validation when updating or creating a remote #3520

quba42 opened this issue Apr 26, 2024 · 4 comments · Fixed by #3558
Assignees
Labels

Comments

@quba42
Copy link
Contributor

quba42 commented Apr 26, 2024

Version
Affects all versions.

Describe the bug
pulp_rpm knows two types of remote. ULN and RPM. The url field for a ULN remote must always have a value starting with uln://. Conversely a RPM remote with a url starting with uln:// never makes sense, and cannot possibly work. We should add sanity validation to both remotes to prevent users from accidentally creating a remote of the wrong type when the intention is to create a ULN/RPM remote. For RPM remotes we should validate the URL does not start with uln://, for ULN we should validate that it does.

Without this validation users will only hit an error once they try to sync using the remote, and the error won't be very clear about what went wrong. We saw this situation with Katello users who decided to change URL type on a Katello repo, but Katello would update the existing remote, rather than destroying it and re-creating one of the other type without an error. We have fixed this on the Katello side, but it still seems like Pulp should throw a meaningful error at remote creation time, rather than a cryptic one at sync time.

To Reproduce
Create or update a ULN or RPM remote with a url of the wrong type. => Successfully creates an unusable remote.

Expected behavior
Throw an error stating clearly what was wrong about the user input.

Additional context
The related Katello issue: https://projects.theforeman.org/issues/37279

@quba42
Copy link
Contributor Author

quba42 commented Apr 26, 2024

@dralley
Copy link
Contributor

dralley commented May 14, 2024

This is reasonable. In hindsight though we maybe should have avoided having a separate ULNRemote type at all. We already have divergent behavior if you pass a file:// path to a normal remote, it could have been handled similarly with uln://

@quba42
Copy link
Contributor Author

quba42 commented May 14, 2024

This is reasonable. In hindsight though we maybe should have avoided having a separate ULNRemote type at all. We already have divergent behavior if you pass a file:// path to a normal remote, it could have been handled similarly with uln://

That would have made some things easier. On the other hand it also has advantages not to overload a single remote type with many different special meanings and fields that are only meaningful to a small subset of users. ULN is a pretty singular use case that in addition to the special upstream url values also adds the uln_server_base_url. This is perhaps similar to the SLES specific fields already on the normal remote. I am torn on which approach is best. Which probably means simply sticking with the choices we made in the past.

@pedro-psb pedro-psb self-assigned this May 24, 2024
@pedro-psb
Copy link
Member

pedro-psb commented May 24, 2024

To tight this up, can we validate that RpmRemote acceptable urls are only http, https and file?

pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue May 24, 2024
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue May 24, 2024
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue May 24, 2024
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue May 27, 2024
pedro-psb added a commit to pedro-psb/pulp_rpm that referenced this issue May 28, 2024
patchback bot pushed a commit that referenced this issue May 29, 2024
dralley pushed a commit that referenced this issue May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

3 participants