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
refactor: effective tls policies reconciler #927
refactor: effective tls policies reconciler #927
Changes from all commits
caa597f
c248ddd
d864361
6a6df6a
f129407
0e4befc
2f3104a
895713a
3688bcb
08ee224
e31f816
c103363
e154fb4
ae206ee
c986a25
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.
Can we be more positive in our checks, doing
if ok
instead of `if !ok. I also wonder if it is better to have the more common action listed first. So if we do more creates that should be listed first but if we do more creates that should be.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.
Sure but whether to do
!ok
orok
first depends also on whether wecreate
orupdate
first, so if we want to be more positive in our check hereif ok
, this would meanupdate
is first 🤔Not sure which action do we expect more of. Maybe @mikenairn can answer on this
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 wouldn't expect too many updates of a Certificate resource after creation. I don't have strong opinions on this, but for what its worth i did
if ok
in the dnspolicy equivalent task, and deal with updates first.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.
if ok { update }
/if !ok { create }
. Does it matter? The number of jump instructions in the final compiled code is literally the same I think.If
!ok
feels like being negative (it doesn't to me BTW), then perhaps we could rename the variable tomissing
. Is it better? I.e.if missing { create }
.IMO, if it doesn't have any obvious downside in performance, the important thing is that the code is readable. In my head (and it could be only me), I read this in terms of chronology and progressive use cases.
The first thing that happens is having nothing, 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.
Thanks guys 👍 I don't think it matters in fairness unless people really feel it hinders readability. My preference is to keep it as is tbh as I also prefer dealing with the
create
first followed byupdate
action. If we feel strongly about this, maybe this can be done in a separate where this can be dealt with in the entire codebaseThere 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.
The cleaning up of orphaned certs gives me an odd feeling, which I have yet to put my finger on. Had the same feeling with the DNS records. The question I ask find asking my self is what does the cleaning up of orphaned certs have to do with TLS polices and their reconcile. I have this feeling that this clean up should be structured as a Task in the Workflow over being part of the TLS policy reconcile. There seems to be a mixing of concerns. I understand how this mixing of concerns is ingrained in us from using the controller runtime. This currently a feeling that this is a code smell, but yet I don't have a good though on what to do about it.
One thing that I don't know at time of writing is how do certs become orphaned.
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 believe this is because there is a indirect link between policies and the subresouce (certificates in this case) and so gives the expected state of the cluster. Though true, this probably can be done as a separate task, that I can look into if you feel strongly about this.
A cert can be orphaned when a gateway listener is removed, or is the target ref of a tls policy is changed to another gateway. In both of these cases the cert(s) is created and will be orphaned since the policy still exists and still valid
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.
Indeed we need to cleanup certs that remained orphan due to, e.g., a listener being removed from a gateway ceteris paribus.
Let's keep this as-is now since it does the job. Maybe in the future we can look for this pattern across the reconcilers (not only this one) and try to improve them all by having "janitors" as separate tasks that can run in parallel to other tasks.
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 am happy with leaving this for now. It is some thing that can be done in the future.