Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce validating admission webhook (experimental) #1133
Introduce validating admission webhook (experimental) #1133
Changes from 10 commits
231db59
f0721d5
b768149
ac1c3bf
285c10e
55a893b
8712590
fc12fe8
1f4664e
021d64d
97961a7
5876758
3ea53ed
7be7f92
d92acd9
3ce028e
286c2da
6fc2d8a
596dac7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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'm curious why you changed CRD installation messages to past tense and after the event... and left all the manager manages as logs prior to the event?
also seems like something that should have been on separate pr.
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.
hmm which messages? I don't see any change on this line
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: these variable are called
ok
by conventionThere 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 method is adopted from the controller-runtime but since we're the one calling it, you could pass
kudo.Validatior
directly and avoid the casting?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.
yeah I need runtime.object here to be able to get GVK
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's
ok
then ;)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.
Right right, it's ok 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'm not sure we need the
generate
component. I also didn't know path for what until diving into the code. It's the API path. DoespathFor(gvk)
make sense? orapiPath(gvk)
? orresourcePath(gvk)
?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.
All of that is adopted from the controller-runtime. It might not be ideal but I'd rather keep it this way: easier to bump once cr changes things.
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.
all these methods actually map to the exact same methods in controller-runtime. https://github.com/kubernetes-sigs/controller-runtime/blob/dc8357113a904bf02721efcde5d92937be39031c/pkg/builder/webhook.go#L163 as the names are not totally out of this world bad, I would probably stick with them, it can make it easier updating or migrating back to their implementation if they extend it in the future
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 reason to introduce another logger, I guess?
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 line would be my biggest pain point:
IsRunning
is defined asExecutionInProgress || ExecutionPending || ErrorStatus
. Why wouldn't a user be able to interrupt Pending or _Error_ed plans? Maybe theError
is due to some misconfiguration and will be fixed by a parameter update?InProgress
: let's say a Deployment can't get healthy because a Pod needs too many resources (not available in the cluster). As a user, I would like to update.Params.Memory
to fix the problem.I guess we need to talk about interruptable plans e.g.
deploy
can be interrupted whilebackup/restore
not? And even that is dependant on the specific operator.P.S. I'd be ok with merging this PR without this line and figure out the exact rules in a followup PR
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 the question is - how do you interrupt? I am not sure that interrupting by just overriding something in spec is the way to go. I would rather interrupt by explicitly triggering manual plan execution with some force parameter. Than all that will be handled via some extension on KEP-20 and all this will be ok...
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 an extensive comment here which I unfortunately lost. I'll try to recreate it.)
In my experience one of the main sources of confusion for operators of SDK services are the implicit and opaque (sometimes undocumented) dependencies and conflicts between plans.
One thing that we'll need to think about (probably not in this PR) is how to deal with:
For example, this SDK test shows a conflict scenario between the "deploy" and "recovery" plans in an SDK service.
The plan execution section of the SDK developer guide has some more (but not all) details about plan behavior.
Feedback from folks like @kaiwalyajoshi and @takirala might also be invaluable when thinking about improving plan behavior in the future and preventing past mistakes.
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.
Regarding interrupting plan executions, I think it does make sense. The SDK has
a concept of pausing plans. I think it'd be also useful to think about rolling
back plan executions.
From the SDK developer guide:
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
"list of webhooks exposed" isn't phrased corrected. "to be exposed" is closer. It is odd to add the "(...)" to a cli output or to specify limitations that will need to be edited and hopefully not forgotten.
The info I expect a user to be concerned with... is... is it the name of the class? does it include the package name? where can I find the mapping of strings -> webhooks.
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.
hmm I really wanted to help the user to know what to do here because if you know what are the supported options you have to go to the docs or code (?) and that's not very nice experience. I think the CLI should help you do this right. I rephrased it a bit.
Yes we will need to update it with every new webhook but this is the best documentation we have and the best documentation we can get to the user so I think we should be fine with it and just really take care of this docs and keep it up to date. We won't certainly add new webhook every week...
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.
WebhookManifests
likely needs to passed a options with a slice... or another parameter of slice of options.