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

✨ add health check for webhook server #1588

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
21 changes: 21 additions & 0 deletions pkg/webhook/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
kscheme "k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/controller-runtime/pkg/certwatcher"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
)
Expand Down Expand Up @@ -87,6 +88,10 @@ type Server struct {
// defaultingOnce ensures that the default fields are only ever set once.
defaultingOnce sync.Once

// started is set to true immediately before the server is started
// and thus can be used to check if the server has been started
started bool

// mu protects access to the webhook map & setFields for Start, Register, etc
mu sync.Mutex
}
Expand Down Expand Up @@ -272,6 +277,9 @@ func (s *Server) Start(ctx context.Context) error {
close(idleConnsClosed)
}()

s.mu.Lock()
s.started = true
s.mu.Unlock()
if err := srv.Serve(listener); err != nil && err != http.ErrServerClosed {
return err
}
Expand All @@ -280,6 +288,19 @@ func (s *Server) Start(ctx context.Context) error {
return nil
}

// StartedChecker returns an healthz.Checker which is healthy after the
// server has been started.
func (s *Server) StartedChecker() healthz.Checker {
Copy link
Member Author

@sbueringer sbueringer Jul 9, 2021

Choose a reason for hiding this comment

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

I can also move this func into the healthz package. The consequence would be that either the healthz package has a dependency to the webhook package or I would have to add an interface with a Started() bool func.

Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe also tcp ping the server?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup definitely. Should we then still keep the started flag?

Or is the idea to combine that, so we only start pinging after started is true?

I'm not sure if it's worth it keeping the started field around.

Copy link
Member

Choose a reason for hiding this comment

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

It can make sense to only start tcp-pinging the port only after the started field is set to true

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, will do.

return func(req *http.Request) error {
s.mu.Lock()
defer s.mu.Unlock()
if !s.started {
return fmt.Errorf("webhook server has not been started yet")
}
return nil
}
}

// InjectFunc injects the field setter into the server.
func (s *Server) InjectFunc(f inject.Func) error {
s.setFields = f
Expand Down
2 changes: 2 additions & 0 deletions pkg/webhook/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ var _ = Describe("Webhook Server", func() {
return ioutil.ReadAll(resp.Body)
}).Should(Equal([]byte("gadzooks!")))

Expect(server.StartedChecker()(nil)).To(Succeed())

ctxCancel()
Eventually(doneCh, "4s").Should(BeClosed())
})
Expand Down