-
Notifications
You must be signed in to change notification settings - Fork 52
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
validate drpolicy s3 profiles and scheduling interval #310
Conversation
Signed-off-by: hatfieldbrian <bhatfiel@redhat.com>
|
||
if condition := util.DrpolicyValidatedConditionGet(drpolicy); condition != nil { | ||
if condition.Status == metav1.ConditionTrue { | ||
log.Info(`valid -> 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.
Curious; why do you use backticks instead of quotes? Backticks are for handling special characters. https://golang.org/ref/spec#String_literals
To keep the code common, I suggest using "" unless necessary.
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 just discovered backticks. I chose them since these strings do not require interpretation. One might say that double quotes are for handling special characters, e.g. newlines, tabs, etc.
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.
@hatfieldbrian are you planning to change the backticks to double quotes or not? The more I look at it, the more I want it to be changed given that they are not providing any special addition.
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 willing to adopt this style for the future. Since this pull request has already been merged I opened #317 to track changing the code merged in this pull request.
// No requeue, as there is no reconcile till user changes desired spec to a valid value | ||
return ctrl.Result{}, nil | ||
} | ||
|
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.
Probably we should not remove this from VRG?
I agree with not having multiple different implementations for validating schedule in different controllers and instead better to use a single implementation probably done in utils. However, I think that it is still better to have a validation check for schedule in VRG. It is because, VRG spec has a field for schedulingInterval. Hence in this case VRG would be doing validation of its own spec field. For DRPC it would be correct to remove the validation of schedule from there as it was validating the schedule from DRPolicy spec and DRPC spec in itself does not have the schedulingInterval field.
Let me know your thoughts 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.
vrg.spec.schedulingInterval format should still be validated. The change to vrg's types.go file should cause it to be validated in the create/update path. The same change was made to drpolicy's types.go file. A test case was added for drpolicy but not duplicated for vrg.
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.
Approving, as we can fix the comments subsequently (minor changes). The backtick that @BenamarMk points out is confusing as it is rarely used. (again if not changed now, can be a subsequent patch @BenamarMk ?)
err = r.validateSchedule(drPolicy) | ||
if err != nil { | ||
return ctrl.Result{}, err | ||
if err := rmnutil.DrpolicyValidated(drPolicy); err != nil { |
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.
Note for later: We need to update DRPC status with some information from this point for user consumption of errors.
) | ||
|
||
if condition := util.DrpolicyValidatedConditionGet(drpolicy); condition != nil { | ||
if condition.Status == metav1.ConditionTrue { |
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.
(future) If ramen config changed (which is not detected or reconciled, hence assume it was done async). We will not rerun the validation. Should we consider running this every time, or at least when the generation of the condition and the resource are different?
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 agree it should be possible to transition from valid to invalid if say an s3 profile stored in ramen config, or the secret that is points to changes. It seems the right way to do that is by drpolicy watching for changes to a cluster's ramen config and s3 secrets referenced by an s3 profile referenced in the drpolicy.
Or it could become invalid if a drpolicy changed to point to a different s3 profile, which would result in a drpolicy reconcile call, but not a revalidation. Though, I understand drpolicy modification is not supported at this point and it or parts of it may be made immutable. For example, removing clusters could result in managed cluster vrg clusterroles being leaked.
However, say s3 access permission is revoked for the user specified in an s3 profile's secret. This seems to be a way for the s3 store connectivity to become invalid without a change to any kubernetes resources and hence no normal way for drpolicy reconciler to be run.
But I do think the point is to validate a drpolicy's s3 profiles and so previously valid ones should be re-validated on any changes to them that drpolicy can be made aware of by normal means.
A workaround to revalidate is to delete and recreate a drpolicy, or if it is mutable, delete its validated condition or modify its validated condition's state to false or unknown which are both considered "invalid".
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.
#319 tracks this issue
return client.Status().Update(ctx, drpolicy) | ||
} | ||
conditionSetFalse = func(reason string, err error) error { | ||
log.Info(`invalid -> invalid`) |
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 a prior S3 profile was incorrect and was subsequently corrected, the error message won't change for the next incorrect profile?
NOTE: If we decide to update the condition, we should take care not to update the same messages and just change the timestamp, as this has caused frequent reconciles with VRG in the past.
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.
You're right. It should change. Also, good point about updating conditionally when the message changes. Issue #318 tracks 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.
@hatfieldbrian we'll merge this as is. But please change the backticks later
Syncing latest changes from upstream main for ramen
This pull request:
ObjectStoreGetter
andObjectStorer
interfaces and their methods so they can be implemented in a different package, e.g.controllers_test
, but leaves the s3 implementations' methods not directly callable outside ofcontrollers
as their types,s3ObjectStoreGetter
ands3ObjectStore
, are not exported.Close #304