-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: accept new SLO specs using the filesystem HTTP api #914
base: main
Are you sure you want to change the base?
Conversation
@metalmatze would appreciate your thoughts on this, thank you. |
Thank you for the contribution, and I am sorry for the long silence. I'm still wondering if we should have this as part of Pyrra directly or if this will be a separate component people can install. I would only add this to Pyrra by disabling this feature by default. I fear that opening this up by default isn't a sane default without any authn/authz. |
Thanks! Sounds pretty reasonable though, suppose putting it behind a runtime switch shouldn't be too hard 👍 |
@harmw wondering if you had any time to action this. |
runtime switch is added 😅 |
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.
Excellent changes in the last commits!
I have some last concerns about the API. Especially the uploading files API.
There are some golangci-lint errors. They will disappear if you rebase to the latest main
.
Thank you for your contribution thus far!
filesystem.go
Outdated
} | ||
defer f.Close() | ||
|
||
io.Copy(f, file) |
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.
This is pretty pragmatic and should be ok for a proof-of-concept.
Quickly, this will become a foot gun where the API will accept any file and write the contents to disk.
We want to ensure the sent data is correct and has a JSON/YAML objective configuration.
A helper function to read from a file and then unmarshal and validate the content already exists with:
Lines 387 to 404 in 174b026
func objectiveFromFile(file string) (v1alpha1.ServiceLevelObjective, slo.Objective, error) { | |
bytes, err := os.ReadFile(file) | |
if err != nil { | |
return v1alpha1.ServiceLevelObjective{}, slo.Objective{}, fmt.Errorf("failed to read file %q: %w", file, err) | |
} | |
var config v1alpha1.ServiceLevelObjective | |
if err := yaml.UnmarshalStrict(bytes, &config); err != nil { | |
return v1alpha1.ServiceLevelObjective{}, slo.Objective{}, fmt.Errorf("failed to unmarshal objective %q: %w", file, err) | |
} | |
objective, err := config.Internal() | |
if err != nil { | |
return v1alpha1.ServiceLevelObjective{}, slo.Objective{}, fmt.Errorf("failed to get objective %q: %w", file, err) | |
} | |
return config, objective, nil | |
} |
If we change that function to take a io.Reader
we can pass both a os.File
(for the existing use case) or a bytes.Buffer
which we would write the HTTP payload content into to that helper function.
-func objectiveFromFile(file string) (v1alpha1.ServiceLevelObjective, slo.Objective, error) {
+func objectiveFromReader(content io.Reader) (v1alpha1.ServiceLevelObjective, slo.Objective, error) {
It's probably easiest to then marshal the parsed config struct back into YAML and write that to disk.
Let me know if you need any more pointers or anything else. I'm happy to help!
@metalmatze quick update: just need (time) to resolve the file handling part 😅 |
Implement an HTTP endpoint at which new SLO specs may be published when using the filesystem operating mode.
Accept files over HTTP at
/specs/create
and writes them to disk, creates new rules and reloads prometheus.Available (bare) endpoints:
Sadly, the fsnotify watcher never managed to see any of the newly uploaded files so I had to force the rules creation a bit 🤔
It seems to work, likely a little rough on the edgesincludes tests now