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 HTTP health server (closes #115) #227

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

keeganwitt
Copy link
Contributor

@keeganwitt keeganwitt commented Dec 7, 2024

fixes #115

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
@keeganwitt keeganwitt marked this pull request as ready for review December 7, 2024 22:18
@keeganwitt
Copy link
Contributor Author

I'm not sure if I did this the right way. We also might want more checks than I've implemented.

@faisal-memon faisal-memon added this to the 0.10.0 milestone Dec 9, 2024
@faisal-memon
Copy link
Collaborator

Thanks @keeganwitt for submitting this.

cmd/spiffe-helper/config/config.go Outdated Show resolved Hide resolved
cmd/spiffe-helper/main.go Outdated Show resolved Hide resolved
cmd/spiffe-helper/main.go Outdated Show resolved Hide resolved
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
…k configs

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
pkg/health/health.go Outdated Show resolved Hide resolved
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Error: can't load config: the Go language version (go1.22) used to build golangci-lint is lower than the targeted Go version (1.23.3)

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Makefile Outdated Show resolved Hide resolved
keeganwitt and others added 4 commits December 16, 2024 16:45
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Co-authored-by: Faisal Memon <fymemon@yahoo.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
"github.com/spiffe/spiffe-helper/pkg/sidecar"
)

func StartHealthServer(hclConfig *config.Config, log logrus.FieldLogger, sidecar *sidecar.Sidecar) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than pass the full config here can we just take in the health check config? Can you create a Config struct in this directory and have main populate and pass that in 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.

If we did, we'd also need to pass DaemonMode as a separate argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It can just be part of the same config struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So create a new HealthServerConfig struct?

Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Signed-off-by: Keegan Witt <keeganwitt@gmail.com>
Comment on lines +174 to +186
svidFile := path.Join(s.config.CertDir, s.config.SVIDFileName)
svidKeyFile := path.Join(s.config.CertDir, s.config.SVIDKeyFileName)
svidBundleFile := path.Join(s.config.CertDir, s.config.SVIDBundleFileName)
if err := disk.WriteX509Context(svidResponse, s.config.AddIntermediatesToBundle, s.config.IncludeFederatedDomains, s.config.CertDir, s.config.SVIDFileName, s.config.SVIDKeyFileName, s.config.SVIDBundleFileName, s.config.CertFileMode, s.config.KeyFileMode); err != nil {
s.config.Log.WithError(err).Error("Unable to dump bundle")
s.fileWritesSuccess[svidFile] = false
s.fileWritesSuccess[svidKeyFile] = false
s.fileWritesSuccess[svidBundleFile] = false
return
}
s.fileWritesSuccess[svidFile] = true
s.fileWritesSuccess[svidKeyFile] = true
s.fileWritesSuccess[svidBundleFile] = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything here is either all true or all false. I think we can just consolidate to a single variable like x509WriteSuccess

Copy link
Contributor Author

@keeganwitt keeganwitt Dec 18, 2024

Choose a reason for hiding this comment

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

Are you proposing the JSON returned would no longer be a map of filenames to boolean, but instead something like

{
  "x509WriteSuccess": true
  "jwtWriteStatus": {
    "file1.jwt": true,
    "file2.jwt": false
  }
}

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes Sidecar Usecase
3 participants