Welcome @norbjd! It looks like this is your first PR to knative/func 🎉

Hi @norbjd. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@norbjd Thanks for your contribution!

Note: maybe we should rename --registry-insecure to just --insecure (and made it toplevel flag) since this allows plain HTTP also for some functionality other than registry access (e.g. allows plain HTTP for function invocation).
WDYT @lkingland @matzew @gauron99 ?

/ok-to-test

@matejvasek no problem :) btw, I've noticed that the description of --registry-insecure flag was not really clear/correct:

  --registry-insecure        Disable HTTPS when communicating to the registry ($FUNC_REGISTRY_INSECURE)

This flag is not really disabling HTTPS, it's just bypassing the certificate check (InsecureSkipVerify). I was confused first because I thought my registry could be in plain HTTP with this flag, but faced some issues like:

Error: failed to run pipeline: creating push check transport for kind-registry:5000 failed: Get "https://kind-registry:5000/v2/": http: server gave HTTP response to HTTPS client

To me, something like this would be clearer:

  --registry-insecure        Skip certificate verification when communicating in HTTPS with the registry ($FUNC_REGISTRY_INSECURE)

Or, renaming it --registry-insecure-skip-verify would clarify even further 😄 These are just my 2 cents as a user 😉

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.31%. Comparing base (a2bc6f6) to head (387cf06).
Report is 6 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2335      +/-   ##
==========================================
+ Coverage   55.61%   62.31%   +6.69%     
==========================================
  Files         128      128              
  Lines       14891    11531    -3360     
==========================================
- Hits         8281     7185    -1096     
+ Misses       5762     3435    -2327     
- Partials      848      911      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The more I'm testing, the more I think there are also missing parts in the insecure registry management when doing remote builds, and this PR might not be enough 👀

Even with the flag honored, I was sadly not able to get a running function 😕: with pack default builder, I couldn't get past this error in the Tekton PipelineRun:

ERROR: failed to initialize analyzer: validating registry read access: ensure registry read access to kind-registry:5000/something/myfunc:latest

After adding more debug logs on Tekton side, I figured out the real error:

Error checking read access: Get "https://kind-registry:5000/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority

Again! 😄 The error comes from here: https://github.com/buildpacks/lifecycle/blob/v0.19.6/image/registry_handler.go#L77.

I'm assuming that the --insecure-registry flag passed to func deploy is not propagated to the buildpack builder, hence the error, and I'm not really sure on how to do. If you have an idea, please tell me 🙏 if possible, that's something I can include in this PR. From my small experience with the codebase, it should probably be put somewhere around here: https://github.com/knative/func/blob/v0.41.0/pkg/pipelines/resources/tekton/task/func-buildpacks/0.2/func-buildpacks.yaml#L159 (e.g. add the -insecure-registry flag, documented here). Unfortunately, the -insecure-registry flag does not seem supported by the default builder (tested with ghcr.io/knative/builder-jammy-tiny:0.0.244), so I'm stuck 😕

For now, on my side, I think I'm just going to use a HTTPS registry with a valid certificate (something like DockerHub) because initially, I just wanted to test on a kind local environment, but I got already too deep in the rabbithole 😅

I'm wondering if the insecure registry management in deploy worked at some point. @dprotaso do you remember if you got it working (just asking because you've added this flag here: #2234)? If it's not working, maybe you could consider removing this flag temporarily? Or maybe if it's just working with local builds and what I'm trying to do is just an edge case, we can probably add a mutually exclusive rule to avoid passing --insecure-registry and --remote at the same time? Anything that could help users to fail fast would be great IMO.

Thanks!

Side note: with the s2i builder, I got a similar issue, but at least, I was able to go through it by easily (and manually) changing the TLSVERIFY parameter to false. I'm facing other issues with this builder though later in the process, so I won't investigate more, the pack builder seems the most adapted to me.

I think so - I used it for the demo in kubecon here

https://github.com/salaboy/knative-kubecon-eu-2024/blob/main/setup-cluster.sh

though I used the env arg - https://github.com/salaboy/knative-kubecon-eu-2024/blob/main/.envrc#L7

It would be an improvement to bundle the insecurity settings under a simple --insecure flag.

Thanks for reporting the issue.

@dprotaso thanks for the link to the repo, I am indeed doing something very similar with kind. The only difference is that I'm trying to trigger remote (on-cluster) builds with the --remote flag. On your side, it looks like you're using local builds and the experimental "host" builder (https://github.com/knative/func/blob/v0.41.0/cmd/deploy.go#L207).

So, I'm assuming the issue just affects remote builds and insecure registries.

OK, I might have an idea on what's happening. There seems to be a misunderstanding on the --registry-insecure flag meaning when doing remote builds. Let me summarize what I've found.

As of today, --registry-insecure in func just means "don't verify TLS certificate" (the value of the flag is used when building the HTTP transport). It does not mean, as the description shows, "Disable HTTPS when communicating to the registry". We can validate this by trying to use a plain HTTP registry and setting --registry-insecure to the deploy command:

Error: failed to run pipeline: creating push check transport for kind-registry:5000 failed: Get "https://kind-registry:5000/v2/": http: server gave HTTP response to HTTPS client

Even with --registry-insecure flag, func will try to send an HTTPS request. Since the registry only speaks HTTP, this will throw the previous error.

That being said, if registry is really speaking HTTPS, but uses self-signed certificates (or more generally, certificates signed by an unknown CA), the previous error will be gone ✨ BUT then, the on-cluster/remote build (tekton pipelinerun) will complain during the "analyze" step:

$ kn func deploy --path myfunc --remote --registry kind-registry:5000/something --registry-insecure
Running Pipeline Task: Building function image on the cluster
Error: function pipeline run has failed with message:

Starting creator...
Parsing inputs...
Ensuring privileges...
Executing command...
===> ANALYZING
Error checking read access: Get "https://kind-registry:5000/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority
ERROR: failed to initialize analyzer: validating registry read access: ensure registry read access to kind-registry:5000/something/myfunc:latest

This is because buildpack is not configured to ignore certificates signed by unknown authorities. One lead I had was to set CNB_INSECURE_REGISTRIES on the buildpack environment variables:

https://github.com/knative/func/compare/a2bc6f6ffd73c34ea1ab0e3f1cf12a5ea3bfde43...norbjd:knative-func:b931d3c37c2e206b8695359593ba642467c4c81f.diff

diff --git a/pkg/pipelines/resources/tekton/task/func-buildpacks/0.2/func-buildpacks.yaml b/pkg/pipelines/resources/tekton/task/func-buildpacks/0.2/func-buildpacks.yaml
index df5d1f7efd..1e40a8dfeb 100644
--- a/pkg/pipelines/resources/tekton/task/func-buildpacks/0.2/func-buildpacks.yaml
+++ b/pkg/pipelines/resources/tekton/task/func-buildpacks/0.2/func-buildpacks.yaml
@@ -72,6 +72,10 @@ spec:
     env:
       - name: CNB_PLATFORM_API
         value: "0.10"
+      - name: CNB_INSECURE_REGISTRIES
+        value: "kind-registry:5000"
+      - name: CNB_LOG_LEVEL
+        value: "debug"
 
   steps:
     - name: prepare

And build a custom version of func to retrieve the pipeline spec from there:

make build FUNC_REPO_REF=norbjd/knative-func FUNC_REPO_BRANCH_REF=custom-buildpack-pipeline

With this change, the func deploy command error slightly changes and now complains about a HTTP request sent to an HTTPS server:

Running Pipeline Task: Building function image on the cluster
Error: function pipeline run has failed with message:

Starting creator...
Parsing inputs...
Ensuring privileges...
Executing command...
===> ANALYZING
Error checking read/write access: creating push check transport for kind-registry:5000 failed: Get "https://kind-registry:5000/v2/": tls: failed to verify certificate: x509: certificate signed by unknown authority; GET http://kind-registry:5000/v2/: unexpected status code 400 Bad Request: Client sent an HTTP request to an HTTPS server.
ERROR: failed to initialize analyzer: validating registry write access: ensure registry read/write access to kind-registry:5000/something/myfunc:latest

This is because for buildpack, insecure means "plain HTTP", and not "use TLS without certificate check": https://github.com/buildpacks/imgutil/blob/4af87862ff7e7b14b82a061cf8ec40d56ad054f5/remote/options.go#L23

So, with the current behavior of --registry-insecure on func, I am stuck on a loop on the build step: buildpack expects registry to speak plain HTTP, but func expects registry to speak HTTPS; if I use a HTTP registry, func complains, etc ♾️

I'm assuming the best fix would be on func side to change the meaning of --registry-insecure from "don't verify TLS certificate" to "use an HTTP registry without TLS". This could probably be done by not defining a TLSClientConfig here instead of passing InsecureSkipVerify flag).

From there, we could use a HTTP registry with func without getting server gave HTTP response to HTTPS client errors, and pass CNB_INSECURE_REGISTRIES to buildpack. I'm not sure on how to do this still, because everything in the func-buildpacks.yaml file seems to be hardcoded and retrieved from Git before building, so it might not be possible to make dynamic changes.

Hello @norbjd

Thanks so much for your clear and thorough examination of how the flag works.

How about we start off with a small PR which updates the flag's description to be more correct.

Then, we can discuss how to enable insecure registries for remote builds.

If setting an environment variable for the Pack builder in the on-cluster build creates the correct behavior, that is something that I believe we can set/create when creating the Pipeline-as-Code artifact which is sent to the server for building.

Again, thanks for looking into this. I suspected this flag was not thoroughly implemented yet through to remote builds, and now we know what's missing.

I'm approving this because, while it doesn't completely fix the OP issue, it does get us further down the path towards correctness.

Hoping for more!

/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, norbjd

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@lkingland thanks!

How about we start off with a small PR which updates the flag's description to be more correct.

Here we go: #2348 🥳