builder: skip name validation for docker context by crazy-max · Pull Request #1879 · docker/buildx

crazy-max · 2023-06-14T10:08:30Z

Although a builder from the store cannot be created unless it has a valid name, this is not the case for a Docker context.

We should skip name validation when checking a node from the store and fall back to finding one from Docker context instead.

store/store_test.go

store/storeutil/storeutil.go

Although a builder from the store cannot be created unless it has a valid name, this is not the case for a Docker context. We should skip name validation when checking a node from the store and fall back to finding one from Docker context instead. Signed-off-by: CrazyMax <[email protected]>

thaJeztah · 2023-06-21T10:49:00Z

Interesting; it looks like context-store in docker/cli does have a validation function;

buildx/vendor/github.com/docker/cli/cli/context/store/store.go

Lines 214 to 226 in bd672ea

    
           // ValidateContextName checks a context name is valid. 
        
           func ValidateContextName(name string) error { 
        
           	if name == "" { 
        
           		return errors.New("context name cannot be empty") 
        
           	} 
        
           	if name == "default" { 
        
           		return errors.New(`"default" is a reserved context name`) 
        
           	} 
        
           	if !restrictedNameRegEx.MatchString(name) { 
        
           		return errors.Errorf("context name %q is invalid, names are validated against regexp %q", name, restrictedNamePattern) 
        
           	} 
        
           	return nil 
        
           }

buildx/vendor/github.com/docker/cli/cli/context/store/store.go

Lines 22 to 24 in bd672ea

    
           const restrictedNamePattern = "^[a-zA-Z0-9][a-zA-Z0-9_.+-]+$" 
        
           var restrictedNameRegEx = regexp.MustCompile(restrictedNamePattern)

But I was actually looking at other parts of the code, and it looks like validation is done externally 😞 https://github.com/docker/cli/blob/085d5c281612298583d19806aa8ec637b572e4cc/cli/command/context/create.go#L124-L135

func checkContextNameForCreation(s store.Reader, name string) error {
	if err := store.ValidateContextName(name); err != nil {
		return err
	}
	if _, err := s.GetMetadata(name); !errdefs.IsNotFound(err) {
		if err != nil {
			return errors.Wrap(err, "error while getting existing contexts")
		}
		return errors.Errorf("context %q already exists", name)
	}
	return nil
}

https://github.com/docker/cli/blob/085d5c281612298583d19806aa8ec637b572e4cc/cli/command/context/create.go#L71-L76

// RunCreate creates a Docker context
func RunCreate(cli command.Cli, o *CreateOptions) error {
	s := cli.ContextStore()
	err := checkContextNameForCreation(s, o.Name)
	if err != nil {
		return err
	}
	swit

So I wonder if there's a bug at hand here, where validation was missed?

crazy-max · 2023-06-21T10:53:40Z

Oh good catch @thaJeztah. Yeah there's smth fishy here 🤔

thaJeztah · 2023-06-21T10:56:18Z

Yeah, was looking "why isn't the validation done by the context store code?", and then saw this PR fly by in my notifications 😂

Is the case here that BuildKit / buildx can use either hostname or context-name, and doesn't know which is the case?

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [docker/buildx-bin](https://github.com/docker/buildx) | stage | patch | `0.11.0` -> `0.11.2` | --- ### Release Notes <details> <summary>docker/buildx (docker/buildx-bin)</summary> ### [`v0.11.2`](https://github.com/docker/buildx/releases/tag/v0.11.2) [Compare Source](docker/buildx@v0.11.1...v0.11.2) Welcome to the v0.11.2 release of buildx! Please try out the release binaries and report any issues at https://github.com/docker/buildx/issues. ##### Contributors - [Justin Chadwell](https://github.com/jedevc) - [CrazyMax](https://github.com/crazy-max) - [Sebastiaan van Stijn](https://github.com/thaJeztah) ##### Changes - Fix a regression that caused buildx to not read the `KUBECONFIG` path from the instance store [#1941](docker/buildx#1941) - Fix a regression with result handle builds showing up in the build history incorrectly [#1954](docker/buildx#1954) ##### Dependency Changes - **github.com/docker/docker** v24.0.2 -> [`36e9e79`](docker/buildx@36e9e796c6fc) - **github.com/moby/buildkit** [`67a0862`](docker/buildx@67a08623b95a) -> [`faa0cc7`](docker/buildx@faa0cc7da353) - **github.com/tonistiigi/fsutil** [`9e7a6df`](docker/buildx@9e7a6df48576) -> [`36ef4d8`](docker/buildx@36ef4d8c0dbb) - **github.com/xeipuuv/gojsonpointer** [`4e3ac27`](docker/buildx@4e3ac2762d5f) -> [`02993c4`](docker/buildx@02993c407bfb) Previous release can be found at [v0.11.1](https://github.com/docker/buildx/releases/tag/v0.11.1) ### [`v0.11.1`](https://github.com/docker/buildx/releases/tag/v0.11.1) [Compare Source](docker/buildx@v0.11.0...v0.11.1) Welcome to the v0.11.1 release of buildx! Please try out the release binaries and report any issues at https://github.com/docker/buildx/issues. ##### Contributors - [CrazyMax](https://github.com/crazy-max) - [Justin Chadwell](https://github.com/jedevc) - [David Karlsson](https://github.com/dvdksn) - [Jhan S. Álvarez](https://github.com/yastanotheruser) ##### Changes - Fix a regression for bake where services in profiles would not be loaded. [#1903](docker/buildx#1903) - Fix a regression where `--cgroup-parent` option had no effect during build. [#1913](docker/buildx#1913) - Fix a regression where valid docker contexts could fail buildx builder name validation. [#1879](docker/buildx#1879) - Fix an issue where the `host-gateway` special address could not be used as an argument to `--add-host`. [#1894](docker/buildx#1894) (also requires moby/moby#45767) - Fix a possible panic when terminal is resized during the build. [#1929](docker/buildx#1929) ##### Dependency Changes - **github.com/docker/cli-docs-tool** v0.5.1 -> v0.6.0 Previous release can be found at [v0.11.0](https://github.com/docker/buildx/releases/tag/v0.11.0) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. �� **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://codeberg.org/woodpecker-plugins/docker-buildx/pulls/85 Co-authored-by: Patrick Schratz <[email protected]> Co-committed-by: Patrick Schratz <[email protected]>

crazy-max requested review from jedevc and tonistiigi June 14, 2023 10:08

crazy-max added the needs-cherry-pick/v0.11 label Jun 14, 2023

crazy-max marked this pull request as ready for review June 14, 2023 10:12

jedevc added this to the v0.11.1 milestone Jun 14, 2023

jedevc reviewed Jun 14, 2023

View reviewed changes

store/store_test.go Show resolved Hide resolved

jedevc reviewed Jun 14, 2023

View reviewed changes

store/storeutil/storeutil.go Show resolved Hide resolved

crazy-max force-pushed the fix-ctx-validation branch from 5585460 to b1c5449 Compare June 14, 2023 12:20

jedevc approved these changes Jun 14, 2023

View reviewed changes

crazy-max merged commit daba16f into docker:master Jun 15, 2023

crazy-max deleted the fix-ctx-validation branch June 15, 2023 12:05

crazy-max mentioned this pull request Jun 15, 2023

[v0.11] cherry-picks for v0.11.1 #1885

Merged

crazy-max removed the needs-cherry-pick/v0.11 label Jun 15, 2023

crazy-max added the status/needs-release-notes label Jun 30, 2023

crazy-max removed the status/needs-release-notes label Jul 7, 2023

jedevc mentioned this pull request Jul 18, 2023

buildx cannot handle contexts whose name starts with a number #1956

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builder: skip name validation for docker context#1879

builder: skip name validation for docker context#1879
crazy-max merged 1 commit intodocker:masterfrom
crazy-max:fix-ctx-validation

crazy-max commented Jun 14, 2023

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jun 21, 2023

Uh oh!

crazy-max commented Jun 21, 2023

Uh oh!

thaJeztah commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

crazy-max commented Jun 14, 2023

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jun 21, 2023

Uh oh!

crazy-max commented Jun 21, 2023

Uh oh!

thaJeztah commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants