Skip to content

Conversation

@ardaguclu
Copy link
Member

This PR is the continuation of #153. First commit basically is from #153. Second commit introduces the middleware.

This middleware;

  • logs requests if the log-level is 5 or higher
  • will check the authorization header and return accordingly, if bearer token is not found.

@ardaguclu
Copy link
Member Author

/cc @manusa

options := []server.StreamableHTTPOption{
server.WithHTTPContextFunc(contextFunc),
server.WithStreamableHTTPServer(httpServer),
server.WithStateLess(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.

This is not related to the changes in this PR. I updated this, because I think we want stateless streamable http server. If it is suggested that this change should be in a different PR, I can remove it from here.

@ardaguclu ardaguclu marked this pull request as ready for review July 2, 2025 12:06
@manusa
Copy link
Member

manusa commented Jul 2, 2025

  • logs requests if the log-level is 5 or higher

I'm not sure if this is aligned with the --log-level config description: Sets the logging level (values from 0-9). Similar to kubectl logging levels.

According to the kubectl quick reference table shouldn't the value be 6 instead?

@ardaguclu
Copy link
Member Author

I think, it is not related but hard to say for sure.

@@ -0,0 +1,61 @@
package middleware
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this scope is too narrow, what do you think about a broader http package to include everything related to our customized http 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.

Firstly, I renamed it to http to be honest. But this name collides with the native Go http package. But I can rename it to, if you want to. And we can use package name as internalhttp.

Copy link
Member Author

@ardaguclu ardaguclu Jul 2, 2025

Choose a reason for hiding this comment

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

I'm fine whatever you decide.

Copy link
Member

Choose a reason for hiding this comment

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

Your call, we can always change the name in the future.

For me it's uncomfortable to see the http server logic in root.go and I think that we should probably move that somewhere else (hence the broader scope for this package, to cover everything related to http/internalhttp).

In any case, this can be covered in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to http, because it is indeed better. We can move server logic under http in a followup PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll move server logic in http package, when I wire the sig terms to 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.

Opened this #164

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

besides the nitty comments, everything else looks good :)

@manusa manusa added this to the 0.1.0 milestone Jul 2, 2025 — with automated-tasks
@manusa manusa self-requested a review July 2, 2025 13:07
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit 524e4f5 into containers:main Jul 2, 2025
5 checks passed
@ardaguclu ardaguclu deleted the audit-log branch July 2, 2025 13:11
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.

2 participants