Skip to content
This repository has been archived by the owner on Feb 6, 2024. It is now read-only.

Add ParseTagOption to k8s/go/pkg/resolver #698

Merged
merged 2 commits into from
Jun 17, 2022
Merged

Add ParseTagOption to k8s/go/pkg/resolver #698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
834d77e
Make go_version an argument to deps()
fejta Jun 15, 2022
979aacd
Add ParseTagOption to k8s/go/pkg/resolver
gonzojive May 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions k8s/go/pkg/resolver/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,41 @@ func RegisterFlags(flagset *flag.FlagSet) *Flags {
return &flags
}

// Option can be passed to NewResolver to configure the resolver.
type Option struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about why we're exporting the struct to other packages but not the methods?

Can you show me a concrete use case for how you want to use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Option pattern is pretty common in Go to parameterize a function with optional arguments.

By using your library within a custom resolver binary, it's possible to replace the heuristics built into the resolver library with custom logic without needing to fork the library.

var (
	insecureRegistries = map[string]struct{}{
		"localhost:32000":      {},
		"othermachine:32000":       {},
	}
)

func main() {
	flagset := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
	flags := lib.RegisterFlags(flagset)
	flagset.Parse(os.Args[1:])

	resolver := lib.NewResolver(flags, lib.ParseTagOption(parseImageTag))
	resolved, err := resolver.Resolve()
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(resolved)
}

func parseImageTag(t string, opts ...name.Option) (name.Tag, error) {
	got, err := name.NewTag(t, opts...)
	if err != nil {
		return got, err
	}

	regName := got.Context().Registry.Name()
	if _, isInsecure := insecureRegistries[regName]; isInsecure {
		var allOpts []name.Option
		allOpts = append(allOpts, opts...)
		allOpts = append(allOpts, name.Insecure)
		return name.NewTag(t, allOpts...)
	}
	return got, fmt.Errorf("unknown registry name %q", regName)
}

Copy link
Contributor

@fejta fejta Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how that is possible when Options struct rather than an interface. No one will be able to provide an alternative implementation of apply() (which by being lower case is also inaccessible to other packages)

Copy link
Contributor Author

@gonzojive gonzojive Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resolver library provides the only means of constructing an Option, like ParseTagOption. I don't see a need for any other option types at a this time, so I kept the type private to the package. You can always open it up later.

This pattern is used by the cmp library, though it uses an interface with unexported methods rather than a struct. Either works. https://pkg.go.dev/github.com/google/go-cmp/cmp#Option

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of this API design, but I guess we can change it later if necessary.

apply func(r *Resolver)
}

// Resolver performs substitutions and resolves/pushes images
type Resolver struct {
flags *Flags

// parseTag is called instead of name.NewTag, which allows overriding how image
// tags are parsed.
parseTag func(name string, opts ...name.Option) (name.Tag, error)
}

// ParseTagOption specifies a function to be used instead of name.NewTag to parse image tags.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to add a hook here vs just replacing the resolver binary?

rules_k8s/k8s/object.bzl

Line 284 in 6323c19

default = Label("//k8s/go/cmd/resolver"),

Copy link
Contributor Author

@gonzojive gonzojive Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is needed to override the name resolution heuristics. Please see the example program below. A custom binary is still needed. However, the binary can reuse all the work done in the resolver library.

//
// This option allows specifying different name.Option values in the call to name.Tag. For
// example, the default logic for detecting whether a registry name is insecure can be
// overridden.
func ParseTagOption(f func(name string, opts ...name.Option) (name.Tag, error)) Option {
return Option{
func(r *Resolver) { r.parseTag = f },
}
}

// NewResolver takes some Flags and returns a Resolver
func NewResolver(flags *Flags) *Resolver {
return &Resolver{flags: flags}
func NewResolver(flags *Flags, option ...Option) *Resolver {
r := &Resolver{
flags: flags,
parseTag: name.NewTag,
}
for _, o := range option {
o.apply(r)
}
return r
}

// Resolve will parse the files pointed by the flags and return a resolvedTemplate and error as applicable
Expand Down Expand Up @@ -238,13 +265,13 @@ func (r *Resolver) publishSingle(spec imageSpec, stamper *compat.Stamper) (strin
var ref name.Reference
if r.flags.ImgChroot != "" {
n := path.Join(r.flags.ImgChroot, stampedName)
t, err := name.NewTag(n, name.WeakValidation)
t, err := r.parseTag(n, name.WeakValidation)
if err != nil {
return "", fmt.Errorf("unable to create a docker tag from stamped name %q: %v", n, err)
}
ref = t
} else {
t, err := name.NewTag(stampedName, name.WeakValidation)
t, err := r.parseTag(stampedName, name.WeakValidation)
if err != nil {
return "", fmt.Errorf("unable to create a docker tag from stamped name %q: %v", stampedName, err)
}
Expand Down
5 changes: 3 additions & 2 deletions k8s/k8s_go_deps.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,15 @@ load(
)
load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

def deps():
def deps(go_version = "1.18.3"):
"""Import dependencies for Go binaries in rules_k8s.

This macro assumes k8s_repositories in //k8s:k8s.bzl has been called
already.
"""
go_rules_dependencies()
go_register_toolchains("1.18.3")
if go_version:
go_register_toolchains(go_version)
gazelle_dependencies()
rules_docker_repositories()
rules_docker_go_deps()
Expand Down