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.

Sign up for GitHub

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

Fixes #38269 - fix container-image-name #11336

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

Fixes #38269 - fix container-image-name #11336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/lib/katello/validators/container_image_name_validator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def validate_each(record, attribute, value)
end

def self.validate_name(name)
if name.empty? || name.length > 255 || !/\A([a-z0-9]+[a-z0-9\-_.]*)+(\/[a-z0-9]+[a-z0-9\-_.]*)*\z/.match?(name)
if name.empty? || name.length > 255 || !/\A([a-z0-9]+([\-_.]*[a-z0-9]+)*)+(\/[a-z0-9]+([\-_.]*[a-z0-9]+)*)*\z/.match?(name)
Copy link
Member

Choose a reason for hiding this comment

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

Here is the official regex from the OCI spec:

[a-z0-9]+([._-][a-z0-9]+)*(/[a-z0-9]+([._-][a-z0-9]+)*)*

https://specs.opencontainers.org/distribution-spec/?v=v1.0.0#DISTRIBUTION-SPEC-27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There seems to be a small difference to how pulp_container defines it, they do a split('/') first and check each element separately:

[a-z0-9]+(?:(?:[._]|__|[-]*)[a-z0-9])*

https://github.com/pulp/pulp_container/blob/main/pulp_container/app/serializers.py#L39

The official regex would suggest that ., -, and _ must be followed by alphanumerical character so a__b or c---d would not be allowed. While pulp seems to explicitly allow them 😵
If we choose the official Regexp we should make sure that pulp uses that one as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mostly posted this in case it was interesting, we don't need to open this can of worms yet :)

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch though, I can ask them why their regex is different from the OCI one. Maybe the OCI spec changed at some point.

return false
end
true
Expand Down
11 changes: 8 additions & 3 deletions app/models/katello/repository.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,11 @@ def self.safe_render_container_name(repository, pattern = nil)
env_exists = repository.environment.present?
is_env_pattern_provided = env_exists && repository.environment.registry_name_pattern.present?
is_cv_default = repository.content_view.default?
items = if Validators::ContainerImageNameValidator.validate_name(repository.organization.label)
[repository.organization.label]
else
['id', repository.organization.id]
end

if is_env_pattern_provided || (is_pattern_provided && env_exists)
pattern ||= repository.environment.registry_name_pattern
Expand All @@ -968,11 +973,11 @@ def self.safe_render_container_name(repository, pattern = nil)
pattern = box.eval(erb.src, allowed_vars, scope_variables)
return Repository.clean_container_name(pattern)
elsif is_cv_default && !is_pattern_provided
items = [repository.organization.label, repository.product.label, repository.label]
items += [repository.product.label, repository.label]
elsif env_exists
items = [repository.organization.label, repository.environment.label, repository.content_view.label, repository.product.label, repository.label]
items += [repository.environment.label, repository.content_view.label, repository.product.label, repository.label]
else
items = [repository.organization.label, repository.content_view.label, repository.content_view_version.version, repository.product.label, repository.label]
items += [repository.content_view.label, repository.content_view_version.version, repository.product.label, repository.label]
end
Repository.clean_container_name(items.compact.join("/"))
Copy link
Member

@ianballou ianballou Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I see why the VCR tests are failing now. The label needs to have clean_container_name applied to it in the new ID logic.

In fact, this raises the question if we even need to use this ID method at all. We already seem to have a precedent for cleaning up container names, which could cause them to clash. So, the question is then, what is missing from this clean_container_name method?

Copy link
Member

Choose a reason for hiding this comment

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

I think what is missing is a check for an underscore before a forward slash, like:

weird_organization_/product/repository...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the question is: what should it do with it?
If it drops the _ before the / then this might create a new collision. I admit the case is far fetched that someone has Weird Organization and Weird Organization! with similar Container Image structure, but it would trigger undefined behaviour.

Copy link
Member

@ianballou ianballou Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the collision is okay since it's the current norm. If an organization label contained any of the characters that are cleaned up today, it could already collide, which we have handling for. Theoretically, the error will then bubble up to the user that they need to update their name. In this case, it could mean removing the organization name from the lifecycle environment container naming pattern.

It's likely that the error won't immediately tell users what the issue is, but it would also (hopefully) be super rare that a user would have two organizations where the only difference is a special character at the end. And in that case, we have a workaround that they can apply (change the name pattern) once they dig into it.

The error would be Registry name pattern results in duplicate container image names for these repositories: %s..

https://github.com/ianballou/katello/blob/71b0fafb18b4bc3f72c0a230f7b9f6116c45326e/app/lib/katello/validators/environment_docker_repositories_validator.rb#L37

The message could potentially be improved to guide the users to the fix.

The reason I'm thinking hard about the above is that this new logic would probably have users wondering why
their container names are deviating from the normal container naming pattern. It's possible that users have container images today that are being 'cleaned' -- these images would then change to the new ID-pattern (worth a test). Actually -- I think most users' container repos would end up in the ID category since the default organization label has capital letters!

[2] pry(main)> ::Organization.second.label
=> "Default_Organization"
[3] pry(main)> ::Katello::Repository.clean_container_name(::Organization.second.label)
=> "default_organization"

On top of that, since it's a very rare case, it makes me wonder if the regression risk is work the gain.

All that said, it's better for users if they have more flexibility. I think, if we go the ID route, we'll want to have it work as well for content views, repositories, lifecycle environments, and products. All of these records could suffer from the same collision issue.

I will add that this ID pattern could also be implemented by the user in the container naming pattern on the LCE. Perhaps our documentation could be updated to guide them there?

Edit: the naming pattern can't include IDs, but the pattern could be modified to not have the organization in it at least (or provide a custom label)

Copy link
Member

Choose a reason for hiding this comment

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

One more thought, even if the orgs are the same, the other parts that make up the name would also have to collide for it to be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

image

end
Expand Down
Loading