Skip to content

✨ Add UserAgentID option to the manager #2506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

✨ Add UserAgentID option to the manager #2506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 16 additions & 5 deletions pkg/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"net"
"net/http"
"reflect"
"strings"
"time"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -120,6 +121,11 @@ type Options struct {
// Only use a custom NewCache if you know what you are doing.
NewCache cache.NewCacheFunc

// UserAgentID is the name of the manager to represent itself in the client requests.
//
// If set, the UserAgentID will be used to override the first part of rest.DefaultKubernetesUserAgent()
UserAgentID string
Copy link
Member

Choose a reason for hiding this comment

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

What is the advantage of this over setting it in the rest config?

Copy link
Member Author

Choose a reason for hiding this comment

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

In comparison to rest.Config this thing has two big advantages I see:

  1. It re-uses DefaultKubernetesUserAgent, having the same structure and versioning as it was before, but with fixed os.Args[0] part.
  2. It allows an operator to continue using manager.New(config.GetConfigOrDie(), opts), without additional thoughts about UserAgent manipulation, but setting the ID in the Options.

Generally speaking, I'd think having the field named as Name or ClientName would be better and could be reused somewhere in the futuer, but don't see where it could be reused just now.

Copy link
Member

Choose a reason for hiding this comment

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

It re-uses DefaultKubernetesUserAgent, having the same structure and versioning as it was before, but with fixed os.Args[0] part.

I don't know what that means because I don't know how the default looks like. Why does it matter to re-use the same structure and versioning rather than setting the entire user-agent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, DefaultKubernetesUserAgent is the useragent like: manager/v1.0.1 (linux/amd64) kubernetes/12312afdb, where we have os.Args[0] + version + runtime.Os/arch + git hash.

So in the way of keep version and runtime the solution is to cut off the first part and replace it with correct name, rather than requiring operator to re-construct the whole UserAgent string with version and runtime lookups.

The better way to achieve it is to have a name param to client-go DefaultKubernetesUserAgent (or have the similar method) and set the UserAgent explicitely, but it is out of scope of controller-runtime.. But yes, I think I'd better do a PR to client-go, as it makes more sense there


// Client is the client.Options that will be used to create the default Client.
// By default, the client will use the cache for reads and direct calls for writes.
Client client.Options
Expand Down Expand Up @@ -319,6 +325,16 @@ func New(config *rest.Config, options Options) (Manager, error) {
// Set default values for options fields
options = setOptionsDefaults(options)

config = rest.CopyConfig(config)
if config.UserAgent == "" {
config.UserAgent = rest.DefaultKubernetesUserAgent()
if options.UserAgentID != "" {
if _, suffix, ok := strings.Cut(config.UserAgent, "/"); ok {
config.UserAgent = options.UserAgentID + "/" + suffix
}
}
}

cluster, err := cluster.New(config, func(clusterOptions *cluster.Options) {
clusterOptions.Scheme = options.Scheme
clusterOptions.MapperProvider = options.MapperProvider
Expand All @@ -333,11 +349,6 @@ func New(config *rest.Config, options Options) (Manager, error) {
return nil, err
}

config = rest.CopyConfig(config)
if config.UserAgent == "" {
config.UserAgent = rest.DefaultKubernetesUserAgent()
}

// Create the recorder provider to inject event recorders for the components.
// TODO(directxman12): the log for the event provider should have a context (name, tags, etc) specific
// to the particular controller that it's being injected into, rather than a generic one like is here.
Expand Down
29 changes: 10 additions & 19 deletions pkg/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,16 @@ var _ = Describe("manger.Manager", func() {
Expect(isCustomWebhook).To(BeTrue())
})

It("should allow passing custom useragent id", func() {
m, err := New(cfg, Options{UserAgentID: "custom"})
Expect(err).NotTo(HaveOccurred())
Expect(m).NotTo(BeNil())

managerConfig := m.GetConfig()
Expect(managerConfig).NotTo(BeNil())
Expect(managerConfig.UserAgent).To(HavePrefix("custom/"))
})

Context("with leader election enabled", func() {
It("should only cancel the leader election after all runnables are done", func() {
m, err := New(cfg, Options{
Expand Down Expand Up @@ -732,25 +742,6 @@ var _ = Describe("manger.Manager", func() {
wgRunnableStarted.Wait()
})

It("should not manipulate the provided config", func() {
// strip WrapTransport, cause func values are PartialEq, not Eq --
// specifically, for reflect.DeepEqual, for all functions F,
// F != nil implies F != F, which means no full equivalence relation.
cfg := rest.CopyConfig(cfg)
cfg.WrapTransport = nil
originalCfg := rest.CopyConfig(cfg)
// The options object is shared by multiple tests, copy it
// into our scope so we manipulate it for this testcase only
options := options
options.newResourceLock = nil
m, err := New(cfg, options)
Expect(err).NotTo(HaveOccurred())
for _, cb := range callbacks {
cb(m)
}
Expect(m.GetConfig()).To(Equal(originalCfg))
})

It("should stop when context is cancelled", func() {
m, err := New(cfg, options)
Expect(err).NotTo(HaveOccurred())
Expand Down