Clarify --registry-insecure flag description

[norbjd]

Changes

• 🧹 Clarify --registry-insecure flag description

/kind documentation

Related to #2335, where we discovered that the description of registry-insecure flag was not really accurate/clear.

After investigation, I've found that enabling this flag just means "skip TLS certificate verification when communicating to the registry", not "allow plain HTTP registries", as the current description ("Disable HTTPS when communicating to the registry") can let users think.

---

Today, the RegistryInsecure boolean in the config (filled from the flag) is used at two places:

1. In the build command: https://github.com/knative/func/blob/1e7dd33/cmd/build.go#L389

The value is simply forwarded to the Pusher, and is used to configure InsecureSkipVerify: https://github.com/knative/func/blob/1e7dd33/pkg/oci/pusher.go#L129-L135:

        if p.Insecure {
		t := remote.DefaultTransport.(*http.Transport).Clone()
		t.TLSClientConfig = &tls.Config{
			InsecureSkipVerify: true,
		}
		oo = append(oo, remote.WithTransport(t))
	}

2. In the deploy command: https://github.com/knative/func/blob/1e7dd33/cmd/deploy.go#L282

The value is simply forwarded to the fn.Client (through NewClient): https://github.com/knative/func/blob/1e7dd33/cmd/client.go#L58, and then used to create the transport with InsecureSkipVerify:

https://github.com/knative/func/blob/1e7dd33/cmd/client.go#L96-L98

func newTransport(insecureSkipVerify bool) fnhttp.RoundTripCloser {
	return fnhttp.NewRoundTripper(fnhttp.WithInsecureSkipVerify(insecureSkipVerify), fnhttp.WithOpenShiftServiceCA())
}

I'm pretty confident it's not used elsewhere, but I'm not familiar with the codebase, so tell me if I've missed something.

---

Note: To me, a better solution would be to completely rename this flag, to something like --insecure-skip-verify or --registry-insecure-skip-verify, as it's effectively only used to configure the InsecureSkipVerify booleans in HTTP transport-related parts of the code.

However, since renaming is a breaking change, I'm not sure if it can be done easily. Another approach, if we want to rename, would be to just 1) add a new flag --insecure-skip-verify, 2) deprecate the existing --registry-insecure, and 3) eventually sunset --registry-insecure.

Internally changing the variable name from RegistryInsecure to InsecureSkipVerify, without touching the flag name, is also possible though. Tell me what you think of this.

In the meantime, changing the description is a good first step IMO to help users figuring out what this flag really means.

Release Note

/

Docs

/

[knative-prow]

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.

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-sigs/prow repository.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.11%. Comparing base (1e7dd33) to head (6bf7c8f).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2348      +/-   ##
==========================================
+ Coverage   60.22%   63.11%   +2.88%     
==========================================
  Files         128      128              
  Lines       14890    11532    -3358     
==========================================
- Hits         8968     7278    -1690     
+ Misses       5003     3339    -1664     
+ Partials      919      915       -4     

Flag	Coverage Δ
e2e-test	38.00% <100.00%> (ø)
e2e-test-oncluster	31.18% <100.00%> (-0.05%)	⬇️
e2e-test-oncluster-runtime	27.12% <100.00%> (?)
e2e-test-runtime-go	26.01% <100.00%> (?)
e2e-test-runtime-node	27.00% <100.00%> (?)
e2e-test-runtime-python	27.00% <100.00%> (?)
e2e-test-runtime-quarkus	27.10% <100.00%> (?)
e2e-test-runtime-rust	26.11% <100.00%> (?)
e2e-test-runtime-springboot	26.15% <100.00%> (?)
e2e-test-runtime-typescript	27.09% <100.00%> (?)
integration-tests	?
unit-tests	49.29% <100.00%> (?)
unit-tests-macos-latest	?
unit-tests-ubuntu-latest	?
unit-tests-windows-latest	?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[norbjd]

I can't really understand why the integration test is failing (logs seems ok to me, but I have trouble reading the logs).

I'm assuming the test is just flaky and will work after a few retries 🔧

[matejvasek]

@norbjd for some reason tests run out of space in the GH action.

[matejvasek]

@dsimansk would you please override the test?

[jrangelramos]

/lgtm

[dsimansk]

/override "Integration Test (ubuntu-latest)"

[knative-prow]

@dsimansk: Overrode contexts on behalf of dsimansk: Integration Test (ubuntu-latest)

In response to this:

/override "Integration Test (ubuntu-latest)"

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-sigs/prow repository.

[matejvasek]

/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• OWNERS [matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment