[gitpod-protocol] handle host:port:token for getGitpodImageAuth #20806
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the parsing of the Gitpod image authentication environment variable by supporting both "host:token" and "host:port:token" formats. In addition to updating the splitting logic in getGitpodImageAuth, comprehensive unit tests have been added to cover various scenarios including malformed inputs and entries with extra spaces.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| components/gitpod-protocol/src/protocol.ts | Updated getGitpodImageAuth to correctly handle host:token and host:port:token formats. |
| components/gitpod-protocol/src/protocol.spec.ts | Added tests for the new parsing logic and ensured various cases (empty, malformed, multiple entries) are covered. |
There was a problem hiding this comment.
@kylos101 We have at least two other places where we parse this forward, which we need to adjust as well.
IIRC it was in proxy.go (image-builder-bob), and maybe even somewhere else - maybe around docker-up? 🤔
I can pull out the references on Monday - or you try to ask cline 😉
| .map((e) => e.trim().split(":")) | ||
| .filter((e) => e.length == 2) | ||
| .forEach((e) => res.set(e[0], e[1])); | ||
| (imageAuth.value || "").split(",").forEach((entry) => { |
There was a problem hiding this comment.
Somehow it's very hard for me to understand the new implementation, while the old I could grok in an instant.
Does it really need that many lines? And if the answer is really yes: We should probably have a separate (in-function?) "parse" method.
There was a problem hiding this comment.
I introduced an inline function and reduced lines of code in c0be79f.
You'll notice a truthiness check on all parts, this is to ensure we're not working with junk. I could remove it, but that'll cause tests to fail like so:
LMKWYT? I'll move onto checking other components now.
Thanks again for the tip, @geropl! It seems supervisor is impacted here,image-builder-bob here and here, and perhaps image-builder-mk3 here are impacted (I'll start with supervisor). |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Gitpod image authentication mechanism by supporting both "host:" and "host:port:" formats. Key changes include:
- Introducing a helper function getHost to determine the host (with optional port) from the token.
- Updating the image authentication parsing logic accordingly.
- Adding extensive unit tests to cover various input scenarios for image authentication.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| components/gitpod-protocol/src/protocol.ts | Added helper function and updated parsing logic for handling host with optional port |
| components/gitpod-protocol/src/protocol.spec.ts | Added new tests to verify correct handling of image auth entries, including edge cases |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Description
Be able to consume GITPOD_IMAGE_AUTH, where it is
host:<token>orhost:port:<token>.Related Issue(s)
Fixes #16237
Fixes CLC-1364
How to test
See unit tests
Documentation
Preview status
gitpod:summary
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold