openshift-ci-robot
commented
2 months ago @sherine-k: This pull request references CLID-101 which is a valid jira issue.
In response to [this](https://github.com/openshift/oc-mirror/pull/838):
># Description
>
>This PR removes the field `TlsVerify` from the `CopyOptions.Global` which was bypassing TLS handshake completely for all registries involved.
>This field was added there when some code was copied from the skopeo code base, where it was already marked deprecated.
>
>Fixes # [CLID-101](https://issues.redhat.com//browse/CLID-101)
>
>## Type of change
>
>Please delete options that are not relevant.
>
>- [x] Bug fix (non-breaking change which fixes an issue)
>- [ ] New feature (non-breaking change which adds functionality)
>- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
>- [ ] This change requires a documentation update
>
>### Implementation details
>
>Instead of a global `TlsVerify`, one should set tls verification under either `CopyOptions.SrcImage` or `CopyOptions.DestImage`. The exact field can be found under type `dockerImageOptions`, and its type is `commonFlag.OptionalBool`.
>
>This makes it impossible to set the field directly. Instead, we use the flags associated with the options to set the value.
>Example:
>```
>srcFlags, srcOpts := mirror.ImageSrcFlags(global, sharedOpts, deprecatedTLSVerifyOpt, "src-", "screds")
>destFlags, destOpts := mirror.ImageDestFlags(global, sharedOpts, deprecatedTLSVerifyOpt, "dest-", "dcreds")
>
>srcFlags.Set("src-tls-verify", "false")
>destFlags.Set("dest-tls-verify", "false")
>```
>
>#### Fixing unit tests
>Most unit tests don't really call a registry. The fix here is easy: simply remove field `TlsVerify` when initializing the Global structure.
>For some unit tests, where a HTTPMock is used as the registry (image-blob-gatherer_test.go for instance), the flags need to be set. This is because the containers/image code is by default now going to make a TLS handshake, unless we set --src-tls-verify and --dest-tls-verify to false.
>
>#### Fixing executor.go + delete.go
>Here we use the same technique (using the flagSet.Set) to force the value of the flags as follows:
>* When DiskToMirror, the source registry is the cache, and therefore ONLY `src-tls-verify` should be set
>* When MirrorToDisk, the destination registry is the cache, and therefore ONLY `dest-tls-verify` should be set
>
>The problem here is that the Mode (m2d, d2m or m2m) is determined at the `Complete` method. At this point, the flagsets are unreachable (flags and options were initialized in func `NewMirrorCmd`. That is why I had to declare, in the `ExecutorSchema` type, a private `srcFlagSet` and `destFlagSet`, in order to be able to later use them.
>
>#### Fixing image-blob-gatherer.go
>
>ImageBlobGatherer is used during delete and MirrorToDisk, where:
>* dest-tls-verify should be set to false (because we target the cache)
>* but src-tls-verify should not be set: it should do TLS handshake with the origin registries
>
>From the ImageBlobGatherer's perspective though, the local cache is considered a source registry.
>In order not to disturb TLS handshake for the mirroring, but still be able to gather blobs from cache during archive generation, I added this extra method `imageOptions.NewSystemContextWithTLSVerificationOverride` : calling it with tlsVerify=false as an input, creates a source system context specific for this use case that bypasses TLS.
>
># How Has This Been Tested?
>
>All Unit and E2E tests pass.
>With an imageSetConfig containing just an additional image, I tested workflows:
>* mirrorToDisk
>* diskToMirror
>* mirrorToMirror
>* Delete
>:warning: graph image generation not tested. This is tricky because the graph image is built using go-containerregistry, and we need to take the tlsVerify from the context and create new setup for go-containerregistry with it. I'm not sure if this is working.
>Operator mirroring and release mirroring shouldn't be impacted.
>
>## Expected Outcome
>No errors related to HTTP such as
>```
>2024/04/23 17:42:53 [ERROR] : unable to add image blobs to the archive : unable to find blobs corresponding to docker://localhost:55000/ubi8/ubi:latest: pinging container registry localhost:55000: Get "https://localhost:55000/v2/": http: server gave HTTP response to HTTPS client
>```
Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Foc-mirror). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot]
aguidirh
openshift-bot
Description
This PR removes the field
TlsVerifyfrom theCopyOptions.Globalwhich was bypassing TLS handshake completely for all registries involved. This field was added there when some code was copied from the skopeo code base, where it was already marked deprecated.Fixes # CLID-101
Type of change
Please delete options that are not relevant.
Implementation details
Instead of a global
TlsVerify, one should set tls verification under eitherCopyOptions.SrcImageorCopyOptions.DestImage. The exact field can be found under typedockerImageOptions, and its type iscommonFlag.OptionalBool.This makes it impossible to set the field directly. Instead, we use the flags associated with the options to set the value. Example:
Fixing unit tests
Most unit tests don't really call a registry. The fix here is easy: simply remove field
TlsVerifywhen initializing the Global structure. For some unit tests, where a HTTPMock is used as the registry (image-blob-gatherer_test.go for instance), the flags need to be set. This is because the containers/image code is by default now going to make a TLS handshake, unless we set --src-tls-verify and --dest-tls-verify to false.Fixing executor.go + delete.go
Here we use the same technique (using the flagSet.Set) to force the value of the flags as follows:
src-tls-verifyshould be setdest-tls-verifyshould be setThe problem here is that the Mode (m2d, d2m or m2m) is determined at the
Completemethod. At this point, the flagsets are unreachable (flags and options were initialized in funcNewMirrorCmd. That is why I had to declare, in theExecutorSchematype, a privatesrcFlagSetanddestFlagSet, in order to be able to later use them.Fixing image-blob-gatherer.go
ImageBlobGatherer is used during delete and MirrorToDisk, where:
From the ImageBlobGatherer's perspective though, the local cache is considered a source registry. In order not to disturb TLS handshake for the mirroring, but still be able to gather blobs from cache during archive generation, I added this extra method
imageOptions.NewSystemContextWithTLSVerificationOverride: calling it with tlsVerify=false as an input, creates a source system context specific for this use case that bypasses TLS.How Has This Been Tested?
All Unit and E2E tests pass. With an imageSetConfig containing just an additional image, I tested workflows:
Expected Outcome
No errors related to HTTP such as