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

Replace the use of viper.bind flags with variables binding #1119

Conversation

Jack-R-lantern
Copy link
Contributor

@Jack-R-lantern Jack-R-lantern commented Jun 26, 2023

That PR is the one that changes viper.BindPFlags.

Fixed #1108

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch 3 times, most recently from 88a3f1a to 92e5bc0 Compare July 2, 2023 13:50
@Jack-R-lantern Jack-R-lantern marked this pull request as ready for review July 2, 2023 15:30
@Jack-R-lantern Jack-R-lantern requested a review from a team as a code owner July 2, 2023 15:30
@mtardy mtardy linked an issue Jul 10, 2023 that may be closed by this pull request
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch 2 times, most recently from 1867823 to 5ff6b0a Compare July 11, 2023 14:32
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch 2 times, most recently from 7c6cd05 to 9120d72 Compare August 27, 2023 15:40
@kkourt kkourt added the needs-rebase This PR needs to be rebased because it has merge conflicts. label Aug 30, 2023
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch 6 times, most recently from 4575667 to 34d4741 Compare September 12, 2023 21:22
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch 3 times, most recently from eb66afb to 8c058d3 Compare September 17, 2023 08:15
@lambdanis lambdanis added release-note/bug This PR fixes an issue in a previous release of Tetragon. area/userspace Related to userspace Tetragon logic and removed needs-rebase This PR needs to be rebased because it has merge conflicts. labels Sep 18, 2023
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from 5e225f9 to afcf6c3 Compare October 27, 2023 13:46
@kkourt kkourt requested a review from mtardy October 27, 2023 14:29
@kkourt
Copy link
Contributor

kkourt commented Oct 27, 2023

@mtardy could you please take a look? You created the issue, so I'm guessing you have context.

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from afcf6c3 to efbf1ae Compare October 28, 2023 02:35
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks indeed it removes the use of viper.Bind which is cool, thanks. However I have a few comments:

Comment on lines 19 to 26
Color string
Debug bool
Output string
Tty string
ServerAddress string
Timeout time.Duration
Retries int
Copy link
Member

Choose a reason for hiding this comment

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

It looks like some of those vars are unused, could we keep only the var used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure i will remove unused variable

@mtardy
Copy link
Member

mtardy commented Oct 31, 2023

Can you rebase your PR on top of main as well so that the tests on vmtests will pass, I merged a PR to fix this recently :)

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from efbf1ae to 47d8dda Compare November 1, 2023 04:39
@Jack-R-lantern
Copy link
Contributor Author

Jack-R-lantern commented Nov 1, 2023

@mtardy
While checking the code, I noticed that for ServerAddress, using viper.IsSet.
For this, I changed it from the existing default value localhost:54321 to empty string

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from 47d8dda to 04e4c26 Compare November 1, 2023 11:18
Copy link

netlify bot commented Nov 1, 2023

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 8883a21
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/6553f3c170894e0008a80049
😎 Deploy Preview https://deploy-preview-1119--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from 04e4c26 to 282588f Compare November 1, 2023 11:39
@mtardy mtardy added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/bug This PR fixes an issue in a previous release of Tetragon. labels Nov 2, 2023
mtardy
mtardy previously approved these changes Nov 2, 2023
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks it looks good! Thank you for adding the feedback :)

@@ -73,7 +72,7 @@ func connect(ctx context.Context) (*grpc.ClientConn, string, error) {
}
if conn == nil {
// Try the server-address prameter
serverAddr = viper.GetString(KeyServerAddress)
serverAddr = ServerAddress
Copy link
Member

Choose a reason for hiding this comment

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

just as a note we could remove this useless variable but I'm okay to merge this as is! :)

flags.Int(common.KeyRetries, 0, "Connection retries with exponential backoff")
viper.BindPFlags(flags)
flags.BoolVarP(&common.Debug, common.KeyDebug, "d", false, "Enable debug messages")
flags.StringVar(&common.ServerAddress, common.KeyServerAddress, "", "gRPC server address")
Copy link
Member

Choose a reason for hiding this comment

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

ah sorry we will need a last fix, I just noticed by reading carefully that the default value localhost:54321 value was removed here, not sure this is intentional?

@mtardy mtardy dismissed their stale review November 2, 2023 10:05

I'm sorry there is just a little default value removed that might not be intentional!

@@ -57,7 +56,7 @@ func connect(ctx context.Context) (*grpc.ClientConn, string, error) {
// context anyway.
// If that address is set try it, if it fails for any reason then retry
// last time with the server address.
if viper.IsSet(KeyServerAddress) == false {
if ServerAddress == "" {
Copy link
Member

Choose a reason for hiding this comment

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

aaaah maybe you removed the default value because of this. I think @tixxdz worked on this, what do you think? Should we remove the default value localhost:54321 from the flag, see this https://github.com/cilium/tetragon/pull/1119/files#r1379864446.

Copy link
Member

Choose a reason for hiding this comment

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

sorry to delay this PR again @Jack-R-lantern, will just need a look from Djalal because he worked on the default gRPC address to put so let him just ack this :)

Copy link
Member

Choose a reason for hiding this comment

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

as far as I understand it would not be okay to remove the default since for most tetra users, tetragon might not run locally and the file (tetragon-info.json) it's trying to read will not be available. But with the default value, this part of the code will never be triggered. Maybe we could try to read the file containing the server address and fall back to the default localhost:54321 value on file read error?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't tested it but I think viper.IsSet returned false for the default value. So we would need to test this and to get the same behavior as before we should change some stuff here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do some more thinking about that code as well

Copy link
Member

Choose a reason for hiding this comment

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

@tixxdz could you take a look at this one :)?

@Jack-R-lantern
Copy link
Contributor Author

@mtardy I changed the code flow, what do you think?

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from 27c4035 to b87372b Compare November 3, 2023 05:44
@mtardy mtardy requested review from tixxdz and removed request for jrfastab November 9, 2023 09:07
Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Really sorry for the delays here, Djalal seems busy, so I took a look and I find the code too complicated for what it wants to achieve, not your fault but the old code + this change are hard to read, so I suggest something like that:

diff --git a/cmd/tetra/common/client.go b/cmd/tetra/common/client.go
index 8d6d032e..237734a0 100644
--- a/cmd/tetra/common/client.go
+++ b/cmd/tetra/common/client.go
@@ -24,7 +24,7 @@ type daemonInfo struct {
 	ServerAddr string `json:"server_address"`
 }
 
-func getActiveServAddr(fname string) (string, error) {
+func readActiveServerAddress(fname string) (string, error) {
 	f, err := os.Open(fname)
 	if err != nil {
 		return "", err
@@ -43,42 +43,25 @@ func connect(ctx context.Context) (*grpc.ClientConn, string, error) {
 	connCtx, connCancel := context.WithTimeout(ctx, Timeout)
 	defer connCancel()
 
-	var conn *grpc.ClientConn
-	var serverAddr string
-	var err error
-
-	// The client cli can run remotely so to support most cases transparently
-	// Check if the server address was set
-	//   - If yes: use it directly, users know better
-	//   - If no: then try the default tetragon-info.json file to find the best
-	//       address if possible (could be unix socket). This also covers the
-	//       case that default address is localhost so we are operating in localhost
-	//       context anyway.
-	//       If that address is set try it, if it fails for any reason then retry
-	//       last time with the server address.
-	if ServerAddress != "" {
-		serverAddr = ServerAddress
-		conn, err = grpc.DialContext(connCtx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
-	} else {
-		serverAddr, err = getActiveServAddr(defaults.InitInfoFile)
-		if err == nil {
-			if serverAddr == "" {
-				serverAddr = defaultServerAddress
-			}
-			conn, err = grpc.DialContext(connCtx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
-			if err != nil {
-				logger.GetLogger().WithFields(logrus.Fields{
-					"InitInfoFile":   defaults.InitInfoFile,
-					"server-address": serverAddr,
-				}).WithError(err).Debugf("Failed to connect to server")
-			}
-		}
-		if conn == nil {
-			serverAddr = defaultServerAddress
-			conn, err = grpc.DialContext(connCtx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
+	// resolve ServerAdress: if flag set by user, use it, otherwise try to read
+	// it from tetragon-info.json, if it doesn't exist, just use default value
+	if ServerAddress == "" {
+		var err error
+		ServerAddress, err = readActiveServerAddress(defaults.InitInfoFile)
+		// if address could not be found in tetragon-info.json file, use default
+		if err != nil {
+			ServerAddress = defaultServerAddress
+			logger.GetLogger().WithField("ServerAddress", ServerAddress).Debug("connect to server using default value")
+		} else {
+			logger.GetLogger().WithFields(logrus.Fields{
+				"InitInfoFile":  defaults.InitInfoFile,
+				"ServerAddress": ServerAddress,
+			}).Debug("connect to server using address in info file")
 		}
 	}
-	return conn, serverAddr, err
+
+	conn, err := grpc.DialContext(connCtx, ServerAddress, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
+	return conn, ServerAddress, err
 }
 
 func CliRunErr(fn func(ctx context.Context, cli tetragon.FineGuidanceSensorsClient), fnErr func(err error)) {

You can save this into a file and git apply it to your branch, it should work

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from b87372b to d079e6f Compare November 14, 2023 11:42
@Jack-R-lantern
Copy link
Contributor Author

Jack-R-lantern commented Nov 14, 2023

@mtardy
I'm fine. You don't have to worry about it.
I'll change it
thanks

Copy link
Member

@mtardy mtardy left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your patience, let's merge this, Djalal can always revert if anything is not good but I tested it and it's great, thanks!

I would have squashed the commits but it's okay we can go like that :)

@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from d079e6f to 8883a21 Compare November 14, 2023 22:25
@Jack-R-lantern
Copy link
Contributor Author

When I thought of 'revert', 'squash' seems to be a must.
I'll reflect it in the PR, thanks for the review.

- remove cmd/tetra/main.go viper.BindPFlags
- replace viper.Get -> Common.XXX
- add cmd/tetra/common/flags.go variable
- change connect logic

Signed-off-by: Jack-R-lantern <[email protected]>
@Jack-R-lantern Jack-R-lantern force-pushed the ISSUE-1108/Replace-the-use-of-viper.BindFlags-with-variables-binding branch from 8883a21 to 532b013 Compare November 14, 2023 22:32
@mtardy
Copy link
Member

mtardy commented Nov 15, 2023

Thanks again for your patience and work :)

@mtardy mtardy merged commit c4ea0e7 into cilium:main Nov 15, 2023
29 checks passed
@lambdanis lambdanis changed the title Issue 1108/replace the use of viper.bind flags with variables binding Replace the use of viper.bind flags with variables binding Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/userspace Related to userspace Tetragon logic release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace the use of viper.BindPFlags with variables binding
4 participants