-
Notifications
You must be signed in to change notification settings - Fork 59
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
API: Add new AdmissionControl service (experimental for now) #983
Conversation
@@ -217,7 +217,7 @@ message CheckHealthResponse { | |||
} | |||
|
|||
//----------------------------------------------------------------- | |||
// Stream -- EXPERIMENTAL and is subject to change until 8.0 |
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.
seems like we can remove this comment 🤣
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.
Are you sure 😄
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.
Some initial comments
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.
Sorry it took me some time to review this and grafana/grafana#87718. This is acceptable I think. However, it's still a bit hard for me to understand the full picture when grafana changes only implements MutateAdmisson
. When/who calls ValidateAdmission
?
Either I would suggest a PR for the grafana-app-sdk making use of these changes or either maybe begin with MutateAdmission
and iterate from there?
The ConvertObject
feels especially blurry adding/exposing when there's no direct use case currently that I can see, but I do understand it's needed eventually but maybe easier to handle as a separate change. To limit scope/review, could be useful/easier with smaller PR's/less changes. Thoughts?
Some nits/questions and some more tests would be nice in general.
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.
More or less same comment than @marefr. The usage / definition of the hooks is a bit confusing for me still.
backend/admission.go
Outdated
ValidateAdmission(context.Context, *AdmissionRequest) (*AdmissionResponse, error) | ||
MutateAdmission(context.Context, *AdmissionRequest) (*AdmissionResponse, error) | ||
ConvertObject(context.Context, *ConversionRequest) (*AdmissionResponse, error) |
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.
Two things are confusing here:
ValidateAdmission
andMutateAdmission
have the same signature, which is confusing. For example, the validation hook is allowing to mutate the object, something I would not expect from a validation step. Should those requests / responses be different?MutateAdmission
andConvertObject
can also be confused since both allows you to modify the object in the request (and both have the same response struct) so it's not clear if a client should use one or the other.
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.
These APIs really follow the k8s conventions -- and can eventually be wired up instead of setting up we webservice for each one 😃
When configuring the resource callbacks you choose if you want a simple "validate" -- eg, yes no, or "mutate" that will modify the body. "convert" is called when you need to return the value in a flavor different than what is saved
backend/admission.go
Outdated
// AdmissionRequest contains information from a kubernetes Admission request and decoded object(s). | ||
// See: https://github.com/kubernetes/kubernetes/blob/v1.30.0/pkg/apis/admission/types.go#L41 | ||
// And: https://github.com/grafana/grafana-app-sdk/blob/main/resource/admission.go#L14 |
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 it's a bit weird that we are referencing Kubernetes code here in this package. It's okay if we apply the same concepts in this "legacy" package but comments should be self-explanatory, without pointing to code that does not apply to the traditional plugin workflow.
backend/admission.go
Outdated
TargetVersion string `json:"target_version,omitempty"` | ||
} | ||
|
||
// See https://github.com/kubernetes/kubernetes/blob/v1.30.0/pkg/apis/admission/types.go#L118 |
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 link only says "AdmissionResponse describes an admission response" which is not that helpful
// "proto", "InstanceSettingsRequest", protoWalker.ZeroValueFieldCount, protoWalker.FieldCount) | ||
// } | ||
|
||
// TODO?? |
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 guess this still need some work
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.
ya --- reluctant to do so much handcrafted mess while still finding the right API structure 😛
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.
then you can delete it for now?
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 add the tests before merge -- they just require hand checking each property...
Now that I think we are close to the final structure -- i'll add them
@marefr I am sympathetic to that view, but given how much boilerplate code this is, the challenge is getting the plumbing right, and we know we can iterate on the message bodies -- this is all a new "service" so not introducing any breaking changes. The challenge of adding this much boilerplate in more PRs is a bit daunting 😬 By adding the core service here (that mimics well known apis!) the later PRs can focus on the usage something more than proto plumbing |
Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
@marefr @andresmgot -- I updated the message so each function returns its own type. Hopefully more clear now. Regarding if we should split this into more PRs -- given how much is plumbing, it seems better to just get it all wired up and then iterate more with real usage. There is a 100% chance that the messages will need to evolve to fit all the usage we want, but the three functions are pretty well established patterns. |
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 had a comment here but I don't know if I ended up not sending it: Should this be defined in the experimental
package? Since it's experimental and it can/will include breaking changes.
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 don't believe there is any way to do that -- since it all has to hook into the grpc bits.
For the "stream" handler -- we had a similar process. It evolved a lot for ~6months before it becoming stable (and finally removing the warning in this PR -- doooh)
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 suppose this particular file could be 🤔 but that is not too helpful given that the grpc ones can not
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.
@marefr suggested it could be implemented like the other funky grpc plugins, but I fear that would make working with datasources annoyingly difficult. The approach proposed here lets us iterate on a good solution using our existing infrastructure, and only affects people who implement the new AdmissionControl service
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.
yes, I am not talking about the grpc stuff but only these files / structs. From the plugin / grafana perspective, people would need to import this as experimental.AdmissionHandler
which reflects better its status vs backend.AdmissionHandler
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.
hymmm -- tried it, but it gets pretty silly with circular references -- the adapters are private and need to be in the backend package; everything needs access to PluginContext. There are fixes, but all are ugly. Would require something like moving (or duplicating?) PluginContext out of backend and making the adapters/serve.go public 🤷🏻
What about naming it ExperimentalAdmissionHandler
? If that lets us move forward then seems ok to me.
I'll add more comments screaming it is experimental but open to suggestions
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, then it's fine. Thanks for checking.
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.
LGTM great work this is in a good enough shape to iterate from 💥
} | ||
|
||
// Check if an object can be admitted | ||
message ValidationResponse { |
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 still needs some additional fields I suppose to be able to report back what/why validation failed, but guess it can be made in a follow up as well/when needed?
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.
Some additional nits/suggestions, but no blockers really
// This is modeled after the kubernetes model for admission controllers | ||
// Since grafana 11.1, this feature is under active development and will continue to evolve in 2024 | ||
// This may also be replaced with a more native kubernetes solution that does not work with existing tooling | ||
type AdmissionHandler interface { |
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.
Nit. One thing that would be nice/useful is if a plugin doesn't have to implement all of these and return not implemented/custom errors - better if SDK can provide good defaults if not implemented. For example always return allowed = 1 for validation if not implemented. Then there's no risk of calling validate when creating/updating a datasource and not having to check for unimplemented errors.
type ValidateAdmissionHandler interface {
// ValidateAdmission is a simple yes|no check if an object can be saved
ValidateAdmission(context.Context, *AdmissionRequest) (*ValidationResponse, error)
}
// ValidateAdmissionFunc implements ValidateAdmissionHandler.
type ValidateAdmissionFunc func(context.Context, *AdmissionRequest) (*ValidationResponse, error)
func (f ValidateAdmissionFunc) ValidateAdmission(ctx context.Context, req *AdmissionRequest) (*ValidationResponse, error) {
return f(ctx, req)
}
type MutateAdmissionHandler interface {
// MutateAdmission converts the input into an object that can be saved, or rejects the request
MutateAdmission(context.Context, *AdmissionRequest) (*MutationResponse, error)
}
// MutateAdmissionFunc implements MutateAdmissionHandler.
type MutateAdmissionFunc func(context.Context, *AdmissionRequest) (*MutationResponse, error)
func (f MutateAdmissionFunc) MutateAdmission(ctx context.Context, req *AdmissionRequest) (*MutationResponse, error) {
return f(ctx, req)
}
type ConvertObjectHandler interface {
// ConvertObject is called to covert objects between different versions
ConvertObject(context.Context, *ConversionRequest) (*ConversionResponse, error)
}
// ConvertObjectFunc implements ConvertObjectHandler.
type ConvertObjectFunc func(context.Context, *ConversionRequest) (*ConversionResponse, error)
func (f ConvertObjectFunc) ConvertObject(ctx context.Context, req *ConversionRequest) (*ConversionResponse, error) {
return f(ctx, req)
}
type AdmissionHandler interface {
ValidateAdmissionHandler
MutateAdmissionHandler
ConvertObjectHandler
}
// to be used in ManageOpts
type AdmissionHandlers struct {
ValidateAdmission ValidateAdmissionHandler
MutateAdmission MutateAdmissionHandler
ConvertObject ConvertObjectHandler
}
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.
interesting -- lets find an approach in a follow up PR. Right now the clients will implement everything so you do not know if an implementation exists or not. The Conversion service will only be needed if multiple versions exist -- so that is a good canidate
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.
Sounds good 👍
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.
In k8s land you subscribe to webhooks which clearly indicates which one of validate/mutate your interest in so something similar from plugin dev perspective was what I had in mind
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>
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.
No more comments from my side, let's see how this goes 👍
This adds an initial attempt at implementing validating/mutating hooks for plugin instance settings. This approach lets us use the existing settings structure while we evolve the k8s resource representation.
This service originally had a more generic storage/admission hook interface, but I think that should be tackled independently.
In grafana/grafana#87718 this is used to validate datasource settings before save