Registry proxy support by a-ovchinnikov · Pull Request #90 · konflux-ci/konflux-build-cli

a-ovchinnikov · 2026-03-27T01:44:27Z

This change adds capability for passing package registry proxy URLs to Hermeto during prefetch.

snyk-io · 2026-03-27T01:44:34Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

chmeliik

Approach seems generally reasonable, most questions are about things that follow what config cache-proxy is doing 😅

chmeliik · 2026-03-31T12:45:13Z

pkg/cliwrappers/hermeto.go


 	log.Debugf("Executing %s", shellJoin("hermeto", args...))
-	_, _, _, err := hc.Executor.Execute(Cmd{Name: "hermeto", Args: args, LogOutput: true})
+	_, _, _, err := hc.Executor.Execute(Cmd{Name: "hermeto", Args: args, LogOutput: true, Env: hc.Env})


Note that a non-nil Env overrides the env that the process would normally get (inherit from parent process). If you intend to inherit the parent env and set additional env vars, they need to be appended to os.Environ()

This one I am not sure about, the standard pattern is to inherit from the enclosing env, but at the same time there is nothing Hermeto expects from it, so I decided that it should be safe to ignore any enclosing variables. Maybe this warrants a comment for better visibility, WDYT?

One variable that can affect Hermeto is PATH, since Hermeto does execute subprocesses quite a lot.
The Konflux prefetch task also exports NETRC, which I think also affects Hermeto.

Inheriting the parent env would be less surprising IMO.

That's fair, I'll update the code.

pkg/commands/prefetch_dependencies/main.go

chmeliik · 2026-03-31T12:59:42Z

pkg/commands/prefetch_dependencies/params.go

+	"use-package-registry-proxy": {
+		Name:       "use-package-registry-proxy",
+		EnvVarName: "USE_PACKAGE_REGISTRY_PROXY",
+		TypeKind:   reflect.String,
+		Usage:      "Whether to use package registry proxy proxy or not. Defaults to true.",
+		DefaultValue: "true",


This can be a reflect.Bool, I'm not sure why the cache-proxy subcommand uses strings for boolean params

My initial plan was to make the variable boolean, however then I followed cache-proxy precedent. If it is safe to convert it to boolean I'll do that (I do not know how it is supposed to interact with environment it is supposed to run in and thus decided that the precedent must be followed).

The build and build-image-index subcommands already have boolean params typed as bool, cache-proxy is the outlier.

Declaring it as a boolean means the accepted values will be the ones accepted by strconv.ParseBool: 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False. IMO that's reasonable behavior.

Ok, I was following the precedent and did not dig deeper, assuming instead that it had some special meaning. Thank you for the clarification! This bit will disappear in the next revision since there is no more need for a dedicated parameter for passing this from init task.

chmeliik · 2026-03-31T13:13:14Z

cmd/config/package_registry_proxy.go

+konflux-build-cli config package-registry-proxy --enable "true"
+
+To disable the package registry proxy:
+konflux-build-cli config package-registry-proxy --enable "false"


I'm not sure how I feel about separate subcommands for handling different parts of the config. But since config cache-proxy set the precedent, I think it's fine to take this approach.

@mmorhun @tisutisu WDYT? What was the reasoning for scoping the existing subcommand specifically to dealing with the cache proxy?

I don't think we need a separate command here. @a-ovchinnikov please use the ConfigReader in the PrefetchDependencies instead of PackageRegistryProxy.

@chmeliik for the init task, all what it does now is configure that cache proxy, so instead of konflux-build-cli init we have konflux-build-cli config cache-proxy. However, if we need to add more stuff there we'd need to refactor things. From the other side, with much higher results limit in Tekton, we'd be able to read all config and don't reread the Config Map in every task that needs Konflux config. But it's not clear now.

I believe the separate command is to satisfy konflux-ci/architecture#311, which proposes that the use-package-registry-proxy setting should be passed to the prefetch task as a param so that it shows up in the Chains attestation:

The init task resolves allow-package-registry-proxy and enable-package-registry-proxy into a single use-package-registry-proxy result that is passed to the prefetch-dependencies task, so that the effective setting is visible in Chains attestation data.

It would indeed be more obvious to just resolve this in the prefetch command, but we need to propose this change on the ADR first. I can do that.

I believe this separates concerns and improves visibility: if Hermeto for any reason misreports proxy URLs this will serve as an indication of intent by operator and there won't be a need to guess. Also summoning @taylormadore to the thread, could you please clarify if I understood the intent correctly?

Update: the command is not needed after all, I will remove it in the next revision.

chmeliik · 2026-03-31T13:15:28Z

pkg/commands/package_registry_proxy.go

+	if (lEPRP == "true" && gAPRP == "true") {
+		usePackageRegistryProxy = "true"
+		l.Logger.Info("Both local parameteres and global config permit the usage of package registry proxy")
+	} else
+	if (lEPRP == "true" && gAPRP == "false") {
+		usePackageRegistryProxy = "false"
+		l.Logger.Info("Global config forbids the usage of package registry proxy")
+	} else
+	if (lEPRP == "false" && gAPRP == "true") {
+		usePackageRegistryProxy = "false"
+		l.Logger.Info("Local config forbids the usage of package registry proxy")
+	} else
+	if (lEPRP == "false" && gAPRP == "false") {
+		usePackageRegistryProxy = "false"
+		l.Logger.Info("Both local and global configs forbid the usage of package registry proxy")
+	} else {
+		pmess := fmt.Sprintf("Impossible combination of enable-package-registry-proxy and allow-package-registry-proxy: " +
+	   		             "'%s' and '%s'.", lEPRP, gAPRP)
+		panic(pmess)


This could really benefit from making the params actual booleans. I can't think of a reason why they would have to be strings. @tisutisu WDYT, was there a reason to handle them as strings in the cache-proxy subcommand?

pkg/commands/prefetch_dependencies/main.go

eskultety · 2026-03-31T10:04:18Z

pkg/commands/prefetch_dependencies/main.go

+		}
+	}
+	return hermetoEnv, err
+}


Not sure if this is the cleanest way of design this in Go, probably not idiomatic. What if instead doing a post-process cleanup of the KonfluxInfo struct which, judging from the change later in config_reader.go, will end up a flat list of options mixed with specific Hermeto proxy settings (or just env settings), we'd already construct KonfluxInfo correctly in the first place? First, we'd declare a clear map for ALL hermeto proxy stuff, the concept I'm playing in my head:

// Hermeto proxy config map to env var mapping table var hermetoProxyMappings = []struct { ConfigMapKey string // key in the K8s ConfigMap / INI file EnvVar string // env var name Hermeto expects }{ {"hermeto-npm-proxy", "HERMETO_NPM__PROXY_URL"}, // {"hermeto-any-future-proxy", "HERMETO_ANY_FUTURE__PROXY_URL"}, }

We could then have a simple single-purpose proxy populating helper returning a map which we'd then consume in the respective config readers (they differ in how they access data), but essentially I'd envision to just see something like this:

return &KonfluxInfo{ ... PackageRegistryProxies: populateProxies(configMap.Data), }, nil }

I think this is a good idea in general, but am not sure it is in scope for this change.

I think it is because you're introducing the core bits I'm commenting on. Note this will become a copy-paste territory the moment you're current changes will get merged, so it will probably never happen, hence I'm raising it now.

eskultety · 2026-03-31T10:42:14Z

pkg/commands/package_registry_proxy.go

+	// Proxy must be used unless told otherwise by either pipeline owner or cluster
+	// operator.
+	usePackageRegistryProxy	:= "true"
+	gAPRP := "true"  // Global enable as defined in ConfigMap.


Is this global default correct?
Per konflux-ci/architecture#311 :

The administrator flag defaults to false, so the package registry proxy is disabled until explicitly enabled at the cluster level.

No, it is not, thank you for catching this!

pkg/commands/package_registry_proxy.go

eskultety · 2026-03-31T10:57:55Z

pkg/commands/package_registry_proxy.go

+		l.Logger.Warnf("Error while reading config data: %s\n" +
+		               "Falling back to allowing package registry proxy on cluster level", err.Error())
+	} else {
+		 gAPRP= packageRegistryProxyConfig.HermetoPackageRegistryProxyAllowed


So this changes the global default from above. Can this ever return an empty string under any circumstances? If so, then that means this fall through the following and execute the 'else' "impossible combination" branch, is that desired or do we want to be extra sure that we always have the global flag set to a meaningful default?

Yes, that's intentional. If there is some misconfiguration then it must be handled by someone else, it is not up to a prefetch job to guess what exactly a user meant.

pkg/commands/package_registry_proxy.go

pkg/commands/prefetch_dependencies/main.go

pkg/common/types.go

mmorhun · 2026-03-31T13:36:48Z

pkg/config/config_reader.go

+	AllowCacheProxy 			string
+	HttpProxy       			string
+	NoProxy         			string
+	HermetoNpmProxy 			string


nitpick: I'd separate Hermeto stuff with a new line. Also would be nice to have a comment.

@mmorhun what are your thoughts on #90 (comment)? Do we want this flat list of options with a distinct entry for each hermeto backend proxy (careful with the naming, you never know when a rebranding is around the corner, lol) or a nested struct which will hold these? It's probably a good idea to have a consensus now as reviewers before this will become a copy-paste territory.

pkg/commands/package_registry_proxy.go

pkg/commands/package_registry_proxy_test.go

mmorhun · 2026-03-31T17:05:10Z

cmd/config/package_registry_proxy.go

+	"github.com/spf13/cobra"
+)
+
+var ConfigPackageRegistryProxyCmd = &cobra.Command{


What is the reason for a separate command? Can we just handle it in Hermeto command?

My understanding is that this is necessary for attestation, there is a parallel discussion happening in 5442f24#r3024624033

mmorhun · 2026-03-31T17:19:36Z

pkg/commands/package_registry_proxy.go

+
+	err = c.ResultsWriter.WriteResultString(usePackageRegistryProxy, c.Params.UsePackageRegistryProxyResultPath)
+	if err != nil {
+		l.Logger.Errorf("failed to write result for http-proxy with error: %s", err.Error())


Should this error whole command?

mmorhun · 2026-03-31T17:20:13Z

pkg/commands/package_registry_proxy.go

+	} else {
+		pmess := fmt.Sprintf("Impossible combination of enable-package-registry-proxy and allow-package-registry-proxy: " +
+	   		             "'%s' and '%s'.", lEPRP, gAPRP)
+		panic(pmess)


Should we just return error instead?

Yes, absolutely, panic is an overkill.

This change makes Hermeto executor aware of ConfigMap and populates Hermeto environment with key-value pairs from it. The main goal of that is to share registry proxy URLs with Hermeto. This change also adds necessary machinery to disable package registries proxies for Hermeto basing on local state of a pipeline and global ConfigMap configuration. Signed-off-by: Alexey Ovchinnikov <aovchinn@redhat.com>

chmeliik reviewed Mar 31, 2026

View reviewed changes

eskultety reviewed Mar 31, 2026

View reviewed changes

mmorhun reviewed Mar 31, 2026

View reviewed changes

chmeliik mentioned this pull request Apr 1, 2026

Add Hermeto registry proxy configuration ADR konflux-ci/architecture#311

Open

a-ovchinnikov force-pushed the registry_proxy_support branch 6 times, most recently from fa465d8 to 5178826 Compare April 2, 2026 22:01

a-ovchinnikov force-pushed the registry_proxy_support branch from 5178826 to 8d40196 Compare April 2, 2026 22:13

Conversation

a-ovchinnikov commented Mar 27, 2026

Uh oh!

snyk-io bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

chmeliik left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmeliik Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmeliik Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmeliik Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

snyk-io bot commented Mar 27, 2026 •

edited

Loading

chmeliik left a comment •

edited

Loading

chmeliik Apr 2, 2026 •

edited

Loading

chmeliik Apr 2, 2026 •

edited

Loading

chmeliik Apr 1, 2026 •

edited

Loading