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
OCPBUGS-11922,OCPBUGS-11910: Limit the nested repository path while mirroring the images #623
OCPBUGS-11922,OCPBUGS-11910: Limit the nested repository path while mirroring the images #623
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lmzuccarelli The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dbb4d1d
to
da0a36e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lmzuccarelli
We have a few areas where we have doubts still, and we want to work a bit on this PR, also using the feedback from the customer tests.
pkg/cli/mirror/mirror.go
Outdated
@@ -500,7 +499,7 @@ func (o *MirrorOptions) generateResults(mapping image.TypedImageMapping, dir str | |||
operator := image.ByCategory(mapping, v1alpha2.TypeOperatorBundle, v1alpha2.TypeOperatorRelatedImage) | |||
|
|||
getICSP := func(mapping image.TypedImageMapping, name string, builder ICSPBuilder) error { | |||
icsps, err := GenerateICSP(name, namespaceICSPScope, icspSizeLimit, mapping, builder) | |||
icsps, err := o.GenerateICSP(name, namespaceICSPScope, icspSizeLimit, mapping, builder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lmzuccarelli
I wonder if, here, instead of using namespaceICSPScope
, we shouldn't more simply use repositoryICSPScope
for the case where o.MaxNestedPaths>0
. Afterall, this is what you're trying to do in manifests.go later
pkg/cli/mirror/mirror.go
Outdated
destination := strings.Join([]string{mirror.Ref.Registry, mirror.Ref.RepositoryName()}, "/") | ||
depth := strings.Split(destination, "/") | ||
if nested > 0 && (len(depth) >= nested) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a doubt about what max-nested-path
should refer to.
image reference talks about a domain and one or more path-components.
Isn't it weird to include the domain (Ref.Registry) in the calculation of the path-components of the destination ?
pkg/cli/mirror/mirror.go
Outdated
|
||
if o.MaxNestedPaths > 0 { | ||
dir := ref.Ref | ||
full := strings.Join([]string{dir.Registry, dir.Namespace, dir.Name}, "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is safer to use dir.Exact() in this case to prevent scenarios where Registry, Namespace or Name might be empty
pkg/cli/mirror/mirror.go
Outdated
return imagesource.TypedImageReference{Ref: dir, Type: ref.Type} | ||
} | ||
// replace all '/' with '-' from the index (MaxNestedPaths - (len(usernamespace) + 1)) | ||
replacedName := strings.ReplaceAll(strings.Join(newName[newIndex:], "/"), "/", "-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this example:
"localhost:5000/ocpbugs-11922/test-paths/openshift4/ose-kube-rbac-proxy"
with --max-nested-path=4
we expected this
"localhost:5000/ocpbugs-11922/test-paths/openshift4-ose-kube-rbac-proxy"
but got this
"localhost:5000/ocpbugs-11922/test-paths/openshift4/ose-kube-rbac-proxy"
} else { | ||
// return original - no changes | ||
return imagesource.TypedImageReference{Ref: ref.Ref, Type: ref.Type} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that in this situation, everything in the path after the userNamespace should also be replaced with -
instead of /
, otherwise, the registry is going to complain about path exceeding the max accepted.
pkg/cli/mirror/mirror.go
Outdated
// OCPBUGS-11922 | ||
dstTIR := o.processNestedPaths(&dstRef) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this works, and mirrors correctly the images, the original TypedImageMapping (images
) is not modified, and is not returned by this method.
Therefore, the mapping.txt that is written L818 doesn't contain the renamed images, and nore will the ImageContentSourcePolicy that is generated from it.
@@ -816,7 +815,7 @@ func (o *MirrorOptions) mirrorToDiskWrapper(ctx context.Context, cfg v1alpha2.Im | |||
|
|||
mappingPath := filepath.Join(o.Dir, mappingFile) | |||
if o.DryRun { | |||
if err := writeMappingFile(mappingPath, mapping); err != nil { | |||
if err := o.writeMappingFile(mappingPath, mapping); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mapping file that we have at this point doesn't contain the image renaming
pkg/cli/mirror/manifests.go
Outdated
if paths > 0 { | ||
source := path.Join(imgRegistry, imgNamespace, k.Ref.Name) | ||
reg, org, repo, _, _ := v1alpha2.ParseImageReference(v.Ref.String()) | ||
dest := path.Join(reg, org, repo) | ||
registryMapping[source] = dest | ||
} else { | ||
source := path.Join(imgRegistry, imgNamespace) | ||
reg, org, _, _, _ := v1alpha2.ParseImageReference(v.Ref.AsRepository().Exact()) | ||
dest := path.Join(reg, org) | ||
registryMapping[source] = dest | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We rather should keep this the way it is, but use icspScope = repository
when max-nested-path is used, when calling getICSP, in mirror.go L543
@lmzuccarelli: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
/lgtm |
/retitle OCPBUGS-11922,OCPBUGS-11910: Limit the nested repository path while mirroring the images |
/jira refresh |
@sherine-k: No Jira issue is referenced in the title of this pull request. In 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 kubernetes/test-infra repository. |
/retitle OCPBUGS-11922,OCPBUGS-11910: Limit the nested repository path while mirroring the images |
@lmzuccarelli: This pull request references Jira Issue OCPBUGS-11922, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. This pull request references Jira Issue OCPBUGS-11910, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In 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 kubernetes/test-infra repository. |
@lmzuccarelli: Jira Issue OCPBUGS-11922: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-11922 has been moved to the MODIFIED state. Jira Issue OCPBUGS-11910: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-11910 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
@zhouying7780 |
/cherrypick release-4.13 |
@sherine-k: new pull request created: #632 In 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 kubernetes/test-infra repository. |
/cherrypick release-4.12 |
@lmzuccarelli: #623 failed to apply on top of branch "release-4.12":
In 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 kubernetes/test-infra repository. |
…irroring the images (openshift#623) * OCPBUGS-11922: Limit the nested repository path while mirroring the images * Fix issues with ICSP not containing the modified mirrors based on --max-nested-paths --------- Co-authored-by: Sherine Khoury <skhoury@redhat.com>
Description
This fix addresses the issue when mirroring to repositories with limited (restricted paths, typically oc-mirror fails with 401 unauthorized for some registries when nested path exceeds the expected maximum path-components
The fix consists of having oc-mirror use the flag --max-nested-paths.
For example a repository to mirror to could be something like
docker://registry.gitlab.com/xxx/yyy
When mirroring the nested paths could expand to
docker://registry.gitlab.com/xxx/yyy/zzz
in this case mirroring will fail with 401 unauthorized if we add one more nested path as indicated
To avoid this scenario oc-mirror will use the --max-nested-paths flag and then calculate the registry+username (in the example for
docker://registry.gitlab.com/xxx/yyy
the count is 3 -> registry + user namespace)If the max nested paths is set to 4, namespaces and component names will then have all instances of "/" converted to "-"
This will allow for successful mirroring.
Fixes # OCPBUGS-11922
Type of change
How Has This Been Tested?
Unit tests have been implemented to cover edge cases.
To ensure that the mirroring works correctly here is a manual test case to validate the mapping.tx and ImageContentSourcePolicy files
ImageSetConfig to use
Command line to execute (change the source-use-http flag as I used a local copy of the operator)
Once this completes execute the following command
Compare the output found in
oc-mirror-workspace/results-xxxx/mapping.txt
to./local-tmp/mapping.txt | grep git
Compare the output found in
oc-mirror-workspace/results-xxxx/imageContentSourcePolicy.yaml
with that of./local-tmp/imageContentSourcePolicy.yaml | grep git
Test Configuration:
Checklist: