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

[BOP-39] Add arg checking #2

Closed
wants to merge 1 commit into from
Closed

[BOP-39] Add arg checking #2

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
65 changes: 55 additions & 10 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package main

import (
"flag"
"net"
"os"

helmv1 "github.com/k3s-io/helm-controller/pkg/apis/helm.cattle.io/v1"
Expand Down Expand Up @@ -37,13 +38,18 @@ func init() {
//+kubebuilder:scaffold:scheme
}

type args struct {
metricsAddr string
enableLeaderElection bool
probeAddr string
}

func main() {
var metricsAddr string
var enableLeaderElection bool
var probeAddr string
flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.")
flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.")
flag.BoolVar(&enableLeaderElection, "leader-elect", false,
args := args{}

flag.StringVar(&args.metricsAddr, "metrics-bind-address", "8080", "The address the metric endpoint binds to.")
flag.StringVar(&args.probeAddr, "health-probe-bind-address", "8081", "The address the probe endpoint binds to.")
flag.BoolVar(&args.enableLeaderElection, "leader-elect", false,
"Enable leader election for controller manager. "+
"Enabling this will ensure there is only one active controller manager.")
opts := zap.Options{
Expand All @@ -52,14 +58,23 @@ func main() {
opts.BindFlags(flag.CommandLine)
flag.Parse()

checkArgs(&args)

ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
config, err := ctrl.GetConfig()
Copy link
Contributor Author

@nwneisen nwneisen Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This still prints a large stack trace when it fails

if err != nil {
setupLog.Error(err, "unable to get kubeconfig")
flag.Usage()
os.Exit(1)
}

mgr, err := ctrl.NewManager(config, ctrl.Options{
Scheme: scheme,
MetricsBindAddress: metricsAddr,
MetricsBindAddress: ":" + args.metricsAddr,
Port: 9443,
HealthProbeBindAddress: probeAddr,
LeaderElection: enableLeaderElection,
HealthProbeBindAddress: ":" + args.probeAddr,
LeaderElection: args.enableLeaderElection,
LeaderElectionID: "a3fc41e4.mirantis.com",
// LeaderElectionReleaseOnCancel defines if the leader should step down voluntarily
// when the Manager ends. This requires the binary to immediately end when the
Expand Down Expand Up @@ -116,3 +131,33 @@ func main() {
os.Exit(1)
}
}

// checkArgs checks that the arguments are valid
func checkArgs(args *args) {
// Check that the metrics port is available
ln, err := net.Listen("tcp", ":"+args.metricsAddr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to check if the port is available here. I think these ports are internal container ports. What do you think?

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 added the check just because I was getting a stack trace on my system due to it already being used and figured we could provide something nicer

2023-10-31T12:53:45-06:00       INFO    controller-runtime.metrics      Metrics server is starting to listen    {"addr": ":8080"}
2023-10-31T12:53:45-06:00       ERROR   controller-runtime.metrics      metrics server failed to listen. You may want to disable the metrics server or use another port if it is due to conflicts       {"error": "error listening on :8080: listen tcp :8080: bind: address already in use"}
sigs.k8s.io/controller-runtime/pkg/metrics.NewListener
        /home/nneisen/.gvm/pkgsets/go1.20.5/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/metrics/listener.go:48
sigs.k8s.io/controller-runtime/pkg/manager.New
        /home/nneisen/.gvm/pkgsets/go1.20.5/global/pkg/mod/sigs.k8s.io/[email protected]/pkg/manager/manager.go:407
main.main
        /home/nneisen/code/boundless-operator/main.go:57
runtime.main
        /home/nneisen/.gvm/gos/go1.20.5/src/runtime/proc.go:250
2023-10-31T12:53:45-06:00       ERROR   setup   unable to start manager {"error": "error listening on :8080: listen tcp :8080: bind: address already in use"}
main.main
        /home/nneisen/code/boundless-operator/main.go:77
runtime.main
        /home/nneisen/.gvm/gos/go1.20.5/src/runtime/proc.go:250

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using setupLog.Error() still gives me a stack trace. I originally tested with fmt.Printf and allows for a one line suggestion on how to fix it

if err != nil {
setupLog.Error(err, "unable to listen on port", "port", args.metricsAddr)
flag.Usage()
os.Exit(1)
}
err = ln.Close()
if err != nil {
setupLog.Error(err, "unable to close port", "port", args.metricsAddr)
os.Exit(1)
}

// Check that the probe port is available
ln, err = net.Listen("tcp", ":"+args.probeAddr)
if err != nil {
setupLog.Error(err, "unable to listen on port", "port", args.probeAddr)
flag.Usage()
os.Exit(1)
}
err = ln.Close()
if err != nil {
setupLog.Error(err, "unable to close port", "port", args.probeAddr)
os.Exit(1)
}

}