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.

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 15, 2023
Merged

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

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
55 changes: 19 additions & 36 deletions cmd/tetra/common/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,10 @@ import (
"syscall"
"time"

"github.com/sirupsen/logrus"

"github.com/cilium/tetragon/api/v1/tetragon"
"github.com/cilium/tetragon/pkg/defaults"
"github.com/cilium/tetragon/pkg/logger"
"github.com/spf13/viper"
"github.com/sirupsen/logrus"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
)
Expand All @@ -25,7 +23,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
Expand All @@ -41,43 +39,28 @@ func getActiveServAddr(fname string) (string, error) {
}

func connect(ctx context.Context) (*grpc.ClientConn, string, error) {
connCtx, connCancel := context.WithTimeout(ctx, viper.GetDuration(KeyTimeout))
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 viper.IsSet(KeyServerAddress) == false {
// server-address was not set by user, try the tetragon-info.json file
serverAddr, err = getActiveServAddr(defaults.InitInfoFile)
if err == nil && serverAddr != "" {
conn, err = grpc.DialContext(connCtx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
}
// Handle both errors
// 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 == "" {
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 :)?

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,
"server-address": serverAddr,
}).WithError(err).Debugf("Failed to connect to server")
"InitInfoFile": defaults.InitInfoFile,
"ServerAddress": ServerAddress,
}).Debug("connect to server using address in info file")
}
}
if conn == nil {
// Try the server-address prameter
serverAddr = viper.GetString(KeyServerAddress)
conn, err = grpc.DialContext(connCtx, serverAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock())
}

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)) {
Expand All @@ -93,7 +76,7 @@ func CliRunErr(fn func(ctx context.Context, cli tetragon.FineGuidanceSensorsClie
for {
conn, serverAddr, err = connect(ctx)
if err != nil {
if attempts < viper.GetInt(KeyRetries) {
if attempts < Retries {
// Exponential backoff
attempts++
logger.GetLogger().WithField("server-address", serverAddr).WithField("attempts", attempts).WithError(err).Error("Connection attempt failed, retrying...")
Expand Down Expand Up @@ -144,7 +127,7 @@ func NewConnectedClient() ConnectedClient {
for {
c.conn, serverAddr, err = connect(c.Ctx)
if err != nil {
if attempts < viper.GetInt(KeyRetries) {
if attempts < Retries {
// Exponential backoff
attempts++
logger.GetLogger().WithField("server-address", serverAddr).WithField("attempts", attempts).WithError(err).Error("Connection attempt failed, retrying...")
Expand Down
13 changes: 13 additions & 0 deletions cmd/tetra/common/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

package common

import "time"

const (
KeyColor = "color" // string
KeyDebug = "debug" // bool
Expand All @@ -12,3 +14,14 @@ const (
KeyTimeout = "timeout" // duration
KeyRetries = "retries" // int
)

const (
defaultServerAddress = "localhost:54321"
)

var (
Debug bool
ServerAddress string
Timeout time.Duration
Retries int
)
3 changes: 1 addition & 2 deletions cmd/tetra/getevents/getevents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/cilium/tetragon/pkg/encoder"
"github.com/cilium/tetragon/pkg/logger"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/types/known/fieldmaskpb"
Expand Down Expand Up @@ -184,7 +183,7 @@ func New() *cobra.Command {
fi, _ := os.Stdin.Stat()
if fi.Mode()&os.ModeNamedPipe != 0 {
// read events from stdin
getEvents(context.Background(), newIOReaderClient(os.Stdin, viper.GetBool("debug")))
getEvents(context.Background(), newIOReaderClient(os.Stdin, common.Debug))
return
}
// connect to server
Expand Down
12 changes: 5 additions & 7 deletions cmd/tetra/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/cilium/tetragon/pkg/logger"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"github.com/spf13/viper"
)

var (
Expand All @@ -32,7 +31,7 @@ func New() *cobra.Command {
cmd.Help()
},
PersistentPreRun: func(cmd *cobra.Command, args []string) {
if viper.GetBool(common.KeyDebug) {
if common.Debug {
logger.DefaultLogger.SetLevel(logrus.DebugLevel)
}
},
Expand All @@ -42,10 +41,9 @@ func New() *cobra.Command {

addCommands(rootCmd)
flags := rootCmd.PersistentFlags()
flags.BoolP(common.KeyDebug, "d", false, "Enable debug messages")
flags.String(common.KeyServerAddress, "localhost:54321", "gRPC server address")
flags.Duration(common.KeyTimeout, 10*time.Second, "Connection timeout")
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?

flags.DurationVar(&common.Timeout, common.KeyTimeout, 10*time.Second, "Connection timeout")
flags.IntVar(&common.Retries, common.KeyRetries, 0, "Connection retries with exponential backoff")
return rootCmd
}
Loading