-
Notifications
You must be signed in to change notification settings - Fork 16
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
Comprehensive overhaul of Harvester and Server enhancements #171
Comprehensive overhaul of Harvester and Server enhancements #171
Conversation
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
// LogAndRespondWithError logs the error and returns an HTTP error. | ||
func LogAndRespondWithError(logger logrus.FieldLogger, err error, errorMessage string, statusCode int) error { | ||
if err != nil { | ||
logger.Errorf("%s: %v", errorMessage, err) |
Check failure
Code scanning / CodeQL
Log entries created from user input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is sanitized.
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments/questions, Max
pkg/common/cryptoutil/certs.go
Outdated
func ParseCertificates(pemBytes []byte) ([]*x509.Certificate, error) { | ||
var certs []*x509.Certificate | ||
block, rest := pem.Decode(pemBytes) | ||
fmt.Println("block", block, "rest", rest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
pkg/common/entity/entities.go
Outdated
@@ -10,19 +10,17 @@ import ( | |||
type ConsentStatus string | |||
|
|||
const ( | |||
ConsentStatusAccepted ConsentStatus = "accepted" | |||
ConsentStatusAccepted ConsentStatus = "approved" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConsentStatusAccepted ConsentStatus = "approved" | |
ConsentStatusApproved ConsentStatus = "approved" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
return fmt.Errorf("failed to fetch federated bundles from SPIRE Server: %w", err) | ||
} | ||
|
||
galadrielCtx, cancel := context.WithTimeout(ctx, galadrielCallTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity question, having 2 defer cancel
which would trigger first, does that cancel the whole context ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, reusing the same variable for the cancel is a mistake.
|
||
// Check if the bundle is the same as the last one fetched | ||
if s.lastSpireBundle != nil && s.lastSpireBundle.Equal(bundle) { | ||
return nil // No new bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what you think about putting a log here to indicate that there is no new bundle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That specific log line might be redundant, as we have a Debug
log statement that says "Checking..." before the action takes place, and another one that states "New bundle..." when it fetches a new bundle, and an Info
log statement indicating "Uploaded...". I think these log lines provide sufficient visibility into the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, sounds good
} | ||
|
||
func (p *jwtProvider) setToken(jwt string) { | ||
p.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not p.mu.RLock
in here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setToken
function modifies the shared jwt token. Therefore, we need to ensure that no other goroutines are reading or writing the jwt token at the same time. A full lock, i.e, Lock()
, accomplishes this by blocking both reads and writes to the shared resource. On the other hand, RLock
only blocks write operations but allows multiple concurrent read operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for clarifying!
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hats off!!! @maxlambrecht
pkg/common/cryptoutil/certs.go
Outdated
func ParseCertificates(pemBytes []byte) ([]*x509.Certificate, error) { | ||
var certs []*x509.Certificate | ||
block, rest := pem.Decode(pemBytes) | ||
fmt.Println("block", block, "rest", rest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was a leftover, already removed.
@@ -0,0 +1 @@ | |||
package bundlemanager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//TODO?
const kidHeader = "kid" | ||
const ( | ||
kidHeader = "kid" | ||
defaultJWTTTL = 10 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this would be better to have them as a configuration. It could give the users of Galadriel the opportunity to align with internal policies they may have for their JWTs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a note to address this later on.
return nil | ||
} | ||
|
||
// isOnboarded Check if the client has been onboarded by checking if there is a JWT token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to check if the JWT has not expired as well? If there is no token or if the token is expired, we need new onboarding, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have enhanced the client's constructor function such that it now attempts to renew the token during initialization. This way, if it fails to procure a renewed token from the server, the creation process will fail, preventing the Harvester from starting. In case of such a failure, the Harvester will log stating that the Harvester was unable to establish a connection with the server using the stored token. The error message will suggest the need for re-onboarding the Harvester using a fresh join token.
pkg/server/endpoints/endpoints.go
Outdated
} | ||
err = server.Close() | ||
if err != nil { | ||
e.Logger.WithError(err).Error("Error closing Echo Server") | ||
} | ||
<-errChan | ||
e.Logger.Info("TCP Server stopped") | ||
log.Info("TCP Server stopped") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TPC Listener stopped
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Signed-off-by: Max Lambrecht <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished my second round of the review. Amazing work, thanks Max!
Pull request check list
Summary of Changes
This Pull Request presents a comprehensive overhaul of the Harvester and several improvements to the Server. The enhancements presented in this PR mainly concern:
Additional Improvements
The Harvester now requests a new JWT token every half an hour and stores it on disk. This resilience-improving measure enables the Harvester to be restarted without needing to re-onboard it with a join token.
Additional Notes
Noteworthy changes to the CLI and configuration files are planned to be covered in a subsequent PR. There's also an acknowledgment of certain packages lacking comprehensive tests - this will be addressed in upcoming follow-up PRs.
Looking Ahead
The aforementioned enhancements in this PR reflect our ongoing commitment to evolve and refine the Harvester.
Which issue this pull requests fixes