Skip to content
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.

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

Added auto detection for kubelet client config #19

Merged
merged 6 commits into from
Nov 16, 2020

Conversation

cristianciutea
Copy link
Contributor

@cristianciutea cristianciutea commented Nov 5, 2020

This PR will add the capability to auto-detect the kubelet endpoint configuration by fetching the node information.
If this mechanism will be used only if no configuration is specified via cmd line args.
If this mechanism fails, the old behavior is use.
In addition to this, a new flag is added to be able to overwrite the hostname
--host (default localhost). If not specified the previous behavior is maintained.

@cristianciutea cristianciutea linked an issue Nov 5, 2020 that may be closed by this pull request
)

var _ = flag.String(namespaces, "", "(optional, default '') Comma separated list of namespaces to discover pods on")
var _ = flag.Bool(insecure, false, `(optional, default false, deprecated) Use insecure (non-ssl) connection.
For backwards compatibility this flag takes precedence over 'tls')`)
var _ = flag.Bool(autoConfig, false, "(optional, default false) fetch node info for configuration")
var _ = flag.Int(timeout, 5000, "(optional, default 5000) timeout in ms")

Choose a reason for hiding this comment

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

do we need to mention this new flag in PR description + release notes?

}

buff, _ := ioutil.ReadAll(resp.Body)
_ = resp.Body.Close()

Choose a reason for hiding this comment

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

why do we need this blank variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes linters happy when you assign errors to a blank variable rather than just ignoring them.

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 more concerned we don't check the error from ioutil.ReadAll or at least make it part of the return on line 90.

Copy link

@haooliveira84 haooliveira84 left a comment

Choose a reason for hiding this comment

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

lgtm

return found
}

// IsAutoConfig returns true if no config parameter was provided as cmd line arg.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 👍

Copy link
Contributor

@carlosroman carlosroman left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the HTTP changes as well.

@cristianciutea cristianciutea merged commit 8100246 into master Nov 16, 2020
@cristianciutea cristianciutea deleted the cciutea/add_auto_config branch November 16, 2020 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow overriding host in Discovery kubernetes
4 participants