-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
implement https mime #850
implement https mime #850
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #850 +/- ##
==========================================
- Coverage 81.59% 81.58% -0.01%
==========================================
Files 159 159
Lines 9013 9021 +8
==========================================
+ Hits 7354 7360 +6
- Misses 1412 1414 +2
Partials 247 247
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
internal/auditlog/https_writer.go
Outdated
if len(h.contentType) == 0 { | ||
// we only do this once | ||
// formatter is immutable in runtime | ||
h.contentType = "application/octet-stream" |
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.
Curious about this and not just text/plain
// we create a test http server | ||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
w.WriteHeader(http.StatusOK) | ||
if r.ContentLength == 0 { | ||
t.Fatal("ContentLength is 0") | ||
} | ||
if ct := r.Header.Get("Content-Type"); !strings.HasPrefix(ct, "application/octet-stream") { |
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.
Is HasPrefix accurate? Can't we do direct comparison equality?
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.
Im most cases yes, but at some point it would be right to use application/json;encoding=utf-8, like the automatic mime detection does
internal/auditlog/https_writer.go
Outdated
formatter plugintypes.AuditLogFormatter | ||
url string | ||
client *http.Client | ||
contentType string |
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 think content type should be on the formatter, not https writer. Since it's still experimental API, we can change formatter, how about making it an actual type with Format()
and ContentType()
methods?
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.
Brilliant. Let's do it.
Any movement on this @jptosso ? |
Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
Co-authored-by: José Carlos Chávez <jcchavezs@gmail.com>
RegisterFormatter("json", noopFormater) | ||
RegisterFormatter("jsonlegacy", noopFormater) | ||
RegisterFormatter("native", nativeFormatter) | ||
RegisterFormatter("json", &noopFormatter{}) |
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.
Shall we turn this into a variable instead of instantiating this every time? cc @anuraaga
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.
Since this is only called once think it's fine
This PR implements MIME for audit log formatters by transforming the function into an interface implementation, going from:
into
This way, the HTTPS audit log writer is aware of the mime type of the formatter.