optimize: avoid redundant blob fetching

[justadogistaken]

I read through the code of proxyBlobStore. I found that the logic of ServeBlob could be optimized. We don't need to request twice for the same blob when caching it.

[codecov-commenter]

Codecov Report

Merging #3569 (abfc675) into main (02e2231) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3569      +/-   ##
==========================================
- Coverage   56.34%   56.32%   -0.02%     
==========================================
  Files         101      101              
  Lines        7314     7309       -5     
==========================================
- Hits         4121     4117       -4     
+ Misses       2536     2535       -1     
  Partials      657      657              

Impacted Files	Coverage Δ
...tion/distribution/registry/proxy/proxyblobstore.go	55.31% <0.00%> (-0.81%)	⬇️
...thub.com/distribution/distribution/context/http.go	63.07% <0.00%> (-0.29%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02e2231...abfc675. Read the comment docs.

[justadogistaken]

/cc @milosgajdos @deleteriousEffect

[milosgajdos]

We should address the CodeQL issues despite their not being directly related to this PR.

[justadogistaken]

We should address the CodeQL issues despite their not being directly related to this PR.

So let't disable the configuration for insecure cipher suites?

[thaJeztah]

It's better to move this commit to a separate pull request, otherwise the security change might get overlooked (burried together with unrelated changes).

I'd also just remove the commented code (as it has no real purpose), and instead putting this information in the commit message, e.g.

This commit removes the following cipher suites that are known to be insecure:

    TLS_RSA_WITH_RC4_128_SHA
    TLS_RSA_WITH_AES_128_CBC_SHA256
    TLS_ECDHE_ECDSA_WITH_RC4_128_SHA
    TLS_ECDHE_RSA_WITH_RC4_128_SHA
    TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
    TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256

That said, perhaps we should follow the recommendation that's posted in CI (if that works):

The following example shows how to create a safer TLS configuration:

package main

import "crypto/tls"

func saferTLSConfig() {
    config := &tls.Config{}
    config.MinVersion = tls.VersionTLS12
    config.MaxVersion = tls.VersionTLS13
    // OR
    config.MaxVersion = 0 // GOOD: Setting MaxVersion to 0 means that the highest version available in the package will be used.
}

[justadogistaken]

Got it!

[milosgajdos]

This look ok to me. I only have nits around naming vars.

[wy65701436]

please move the Stat to line 114, just before Create(), to avoid unnecessy load for failure case.

[wy65701436]

and please wrapper an method for the code block.

All the code should be in the same level,

func {
  method()
  method()
  details....
}

== >

func {
  method()
  method()
  method()
}

[wy65701436]

I am not quite sure about the original idea to use go rountine to async calls the storeLocal(). But this PR changes it to sync mode, if I understand correctly. Is it expceted? cc @milosgajdos

[wy65701436]

pbs.scheduler.AddBlob(blobRef, repositoryTTL)

is removed for this PR, it's right?

[justadogistaken]

Sorry, I forgot this part.

[sc0530]

Any progress on this optimization? It appears to be a valuable approach for eliminating redundant blob requests to the registry.

[milosgajdos]

This needs a rebase @justadogistaken

[justadogistaken]

This needs a rebase @justadogistaken

I will rebase it later.

[justadogistaken]

@thaJeztah @wy65701436 Please help review this work.

[Jamstah]

If a copy is already in flight, does it just fall back to streaming directly from the remote store to the client? If that is the case, a comment might be good there.

Just trying to get my head around the logic before adding a LGTM.

[justadogistaken]

If a copy is already in flight, does it just fall back to streaming directly from the remote store to the client? If that is the case, a comment might be good there.

Just trying to get my head around the logic before adding a LGTM.

Yes, it will fall back to streaming directly from remote store. Which is what it used to be.
This work majorly reduces a redundant blob fetching when "the requested blob does not exist locally" and "the blob digest it not in the flight."

[Jamstah]

I'd appreciate a comment in ServeBlob explaining that if there is already a copy in flight that the current thread will just proxy straight from the remote to the client, I assume because there's no sensible way to join in with the tee.

But that's not blocking and overall, very sensible change, lgtm.

[justadogistaken]

I'd appreciate a comment in ServeBlob explaining that if there is already a copy in flight that the current thread will just proxy straight from the remote to the client, I assume because there's no sensible way to join in with the tee.

But that's not blocking and overall, very sensible change, lgtm.

Thank you. And the comments are added.