Kubernetes Config Update: Prioritize InClusterConfig function #3584

Kalaiselvi84 · 2024-01-09T22:32:17Z

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3582

Special notes for your reviewer:

vendor/k8s.io/client-go/tools/clientcmd/client_config.go

agones-bot · 2024-01-09T22:41:14Z

Build Failed 😱

Build Id: 79f320f4-3551-4c3d-8e89-17592630bc46

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-10T00:19:47Z

Build Failed 😱

Build Id: 3b3f4d7c-36a3-4026-9286-5d164fd1131d

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-10T02:55:07Z

Build Failed 😱

Build Id: 399e2dc6-4959-42c4-91da-d069509a856f

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

pkg/util/k8sconfig/k8s_client_config.go

markmandel · 2024-01-10T03:53:46Z

pkg/util/k8sconfig/k8s_client_config.go

+
+// InClusterBuildConfig is a helper function that builds configs by trying the InClusterConfig().
+// If InClusterConfig is unsuccessful, it falls back to BuildConfigFromFlags.
+func InClusterBuildConfig(masterURL, kubeconfigPath string) (*restclient.Config, error) {


Suggested change

func InClusterBuildConfig(masterURL, kubeconfigPath string) (*restclient.Config, error) {

func InClusterBuildConfig(kubeconfigPath string) (*restclient.Config, error) {

Since we never use a masterURL other than "" ?

vendor/k8s.io/client-go/tools/clientcmd/client_config.go

agones-bot · 2024-01-10T21:00:42Z

Build Succeeded 👏

Build Id: d19c02b8-8e6f-4387-8a4d-e70b001ab137

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://e762019-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-e762019-amd64

agones-bot · 2024-01-12T03:45:39Z

Build Succeeded 👏

Build Id: c2c52f0e-d2b3-4301-99ab-c436a68e4cf6

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://87698de-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-87698de-amd64

agones-bot · 2024-01-18T21:12:34Z

Build Succeeded 👏

Build Id: 5baea74b-7bf5-4912-9492-1ef422c6e09a

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://6ec1577-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-6ec1577-amd64

markmandel

Other than a small nit, this LGTM.

@roberthbailey any issues on your end?

markmandel · 2024-01-23T01:30:39Z

pkg/util/runtime/runtime.go

+// or a kubeconfigPath. These parameters are passed in as command line flags for cluster
+// components. If neither masterUrl or kubeconfigPath are passed,
+// the function logs a warning and falls back to a default configuration
+func BuildCustomConfigFromFlags(masterURL, kubeconfigPath string) (*restclient.Config, error) {


Small thing, but since this function isn't used anywhere else - any reason not to have the content be inline with InClusterBuildConfig(...) ?

Merged the two functions and removed the masterURL from the return statement. Now the return statement is:

return clientcmd.NewNonInteractiveDeferredLoadingClientConfig( &clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath}, &clientcmd.ConfigOverrides{}).ClientConfig()

Would this approach look good?

… pr-3582

agones-bot · 2024-01-23T06:22:32Z

Build Failed 😱

Build Id: 9b818f54-8bf0-47f9-8725-0c14d1513ec8

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-23T18:48:41Z

Build Succeeded 👏

Build Id: 1010687c-89d7-4139-81f8-1af3bf7aecaa

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://233fa64-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-233fa64-amd64

agones-bot · 2024-01-24T20:51:36Z

Build Succeeded 👏

Build Id: 0a658c43-ca58-4d2b-8f31-81aa73184865

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://0779346-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-0779346-amd64

markmandel

Approach looks good - just saw one thing that needs tweaking.

pkg/util/runtime/runtime.go

… pr-3582

markmandel · 2024-01-24T22:32:34Z

cmd/controller/main.go

-	// if the kubeconfig fails BuildConfigFromFlags will try in cluster config
-	clientConf, err := clientcmd.BuildConfigFromFlags("", ctlConf.KubeConfig)
+	// if the kubeconfig fails InClusterBuildConfig will try in cluster config
+	clientConf, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), ctlConf.KubeConfig)


Suggested change

clientConf, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), ctlConf.KubeConfig)

clientConf, err := runtime.InClusterBuildConfig(logger, ctlConf.KubeConfig)

Should be sufficient, and same for the other instances where there is already a global logger in place.

markmandel · 2024-01-24T22:35:43Z

test/e2e/framework/framework.go

@@ -81,7 +81,8 @@ type Framework struct {
 }

 func newFramework(kubeconfig string, qps float32, burst int) (*Framework, error) {
-	config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
+	logger := logrus.New()


Suggested change

logger := logrus.New()

logger = runtime.NewLoggerWithSource("framework")

Just to be consistent.

markmandel · 2024-01-24T22:35:55Z

test/e2e/framework/framework.go

@@ -81,7 +81,8 @@ type Framework struct {
 }

 func newFramework(kubeconfig string, qps float32, burst int) (*Framework, error) {
-	config, err := clientcmd.BuildConfigFromFlags("", kubeconfig)
+	logger := logrus.New()
+	config, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), kubeconfig)


Suggested change

config, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), kubeconfig)

config, err := runtime.InClusterBuildConfig(logger, kubeconfig)

markmandel · 2024-01-24T22:37:32Z

test/eviction/evictpod.go

@@ -49,8 +51,9 @@ func main() {
 	if *pod == "" {
 		log.Fatal("--pod must be non-empty")
 	}
+	logger := logrus.New()


Suggested change

logger := logrus.New()

logger = runtime.NewLoggerWithSource("main")

To be consistent. Might even switch out the log. below to logger to be nice and clean.

All the changes are in place per your suggestion👍

agones-bot · 2024-01-24T23:24:57Z

Build Failed 😱

Build Id: 809bb667-f608-4832-a100-c82ea7d3f4a2

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

agones-bot · 2024-01-26T00:52:20Z

Build Succeeded 👏

Build Id: 137b4f01-5272-47aa-aa55-e17ac978d6fb

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://4d2b365-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-4d2b365-amd64

markmandel · 2024-01-26T00:54:35Z

test/eviction/evictpod.go

@@ -45,22 +45,23 @@ func main() {
 	namespace := flag.String("namespace", "default", "Namespace (defaults to `default`)")
 	pod := flag.String("pod", "", "Pod name (required)")
 	flag.Parse()
+	logger := runtime.NewLoggerWithSource("main")


Suggested change

logger := runtime.NewLoggerWithSource("main")

logger := runtime.NewLoggerWithSource("evictpod")

Minor of minor tweak (since the file is "evictpod" not "main"), but otherwise, this looks good to go for me.

markmandel

LGTM!

agones-bot · 2024-01-26T02:21:45Z

Build Succeeded 👏

Build Id: 1cd9f46f-acb2-4a02-8770-076166695025

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

https://f81235c-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/3584/head:pr_3584 && git checkout pr_3584
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.38.0-dev-f81235c-amd64

Kubernetes Config Update: Prioritize InClusterConfig function

1df9352

Kalaiselvi84 requested review from markmandel and roberthbailey January 9, 2024 22:32

github-actions bot added kind/feature New features for Agones size/S labels Jan 9, 2024

roberthbailey reviewed Jan 9, 2024

View reviewed changes

vendor/k8s.io/client-go/tools/clientcmd/client_config.go Outdated Show resolved Hide resolved

Kalaiselvi84 marked this pull request as draft January 9, 2024 23:30

Create k8sconfig in pkg/util

fd080aa

Fix lint

ff6cfb9

markmandel reviewed Jan 10, 2024

View reviewed changes

Kalaiselvi84 added 3 commits January 10, 2024 19:49

Review changes

99ac4a8

small change

9348aff

Merge branch 'main' into pr-3582

e762019

Merge branch 'main' into pr-3582

87698de

Kalaiselvi84 marked this pull request as ready for review January 12, 2024 02:10

Merge branch 'main' into pr-3582

6ec1577

markmandel reviewed Jan 23, 2024

View reviewed changes

Kalaiselvi84 added 4 commits January 23, 2024 05:05

Combine two methods

148705d

Merge branch 'pr-3582' of https://github.com/Kalaiselvi84/agones into…

e281f60

… pr-3582

small tweak

cab2c17

Merge branch 'main' into pr-3582

233fa64

Merge branch 'main' into pr-3582

0779346

markmandel reviewed Jan 24, 2024

View reviewed changes

pkg/util/runtime/runtime.go Outdated Show resolved Hide resolved

pkg/util/runtime/runtime.go Outdated Show resolved Hide resolved

Kalaiselvi84 added 2 commits January 24, 2024 22:04

use logrus library

383602c

Merge branch 'pr-3582' of https://github.com/Kalaiselvi84/agones into…

7d2bce1

… pr-3582

github-actions bot added the size/M label Jan 24, 2024

markmandel reviewed Jan 24, 2024

View reviewed changes

Kalaiselvi84 added 2 commits January 24, 2024 23:02

review changes

14182a7

Merge branch 'main' into pr-3582

4d2b365

markmandel reviewed Jan 26, 2024

View reviewed changes

Kalaiselvi84 added 2 commits January 26, 2024 01:16

Merge branch 'main' into pr-3582

5194641

small tweak

f81235c

markmandel approved these changes Jan 26, 2024

View reviewed changes

markmandel merged commit f369bc2 into googleforgames:main Jan 26, 2024
4 checks passed

Kalaiselvi84 deleted the pr-3582 branch March 15, 2024 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubernetes Config Update: Prioritize InClusterConfig function #3584

Kubernetes Config Update: Prioritize InClusterConfig function #3584

Kalaiselvi84 commented Jan 9, 2024

agones-bot commented Jan 9, 2024

agones-bot commented Jan 10, 2024

agones-bot commented Jan 10, 2024

markmandel Jan 10, 2024

agones-bot commented Jan 10, 2024

agones-bot commented Jan 12, 2024

agones-bot commented Jan 18, 2024

markmandel left a comment

markmandel Jan 23, 2024

Kalaiselvi84 Jan 23, 2024

agones-bot commented Jan 23, 2024

agones-bot commented Jan 23, 2024

agones-bot commented Jan 24, 2024

markmandel left a comment

markmandel Jan 24, 2024

markmandel Jan 24, 2024

markmandel Jan 24, 2024

markmandel Jan 24, 2024

Kalaiselvi84 Jan 24, 2024

agones-bot commented Jan 24, 2024

agones-bot commented Jan 26, 2024

markmandel Jan 26, 2024

markmandel left a comment

agones-bot commented Jan 26, 2024

	func InClusterBuildConfig(masterURL, kubeconfigPath string) (*restclient.Config, error) {
	func InClusterBuildConfig(kubeconfigPath string) (*restclient.Config, error) {

	clientConf, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), ctlConf.KubeConfig)
	clientConf, err := runtime.InClusterBuildConfig(logger, ctlConf.KubeConfig)

	logger := logrus.New()
	logger = runtime.NewLoggerWithSource("framework")

	config, err := runtime.InClusterBuildConfig(logger.WithFields(logrus.Fields{}), kubeconfig)
	config, err := runtime.InClusterBuildConfig(logger, kubeconfig)

	logger := logrus.New()
	logger = runtime.NewLoggerWithSource("main")

	logger := runtime.NewLoggerWithSource("main")
	logger := runtime.NewLoggerWithSource("evictpod")

Kubernetes Config Update: Prioritize InClusterConfig function #3584

Kubernetes Config Update: Prioritize InClusterConfig function #3584

Conversation

Kalaiselvi84 commented Jan 9, 2024

agones-bot commented Jan 9, 2024

agones-bot commented Jan 10, 2024

agones-bot commented Jan 10, 2024

Choose a reason for hiding this comment

agones-bot commented Jan 10, 2024

agones-bot commented Jan 12, 2024

agones-bot commented Jan 18, 2024

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jan 23, 2024

agones-bot commented Jan 23, 2024

agones-bot commented Jan 24, 2024

markmandel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agones-bot commented Jan 24, 2024

agones-bot commented Jan 26, 2024

Choose a reason for hiding this comment

markmandel left a comment

Choose a reason for hiding this comment

agones-bot commented Jan 26, 2024