refactor the source spec validation #1122
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
nan-yu
force-pushed
the
validation
branch
2 times, most recently
from
b6924e5
to
1a77d5a
Compare
nan-yu
force-pushed
the
validation
branch
2 times, most recently
from
73e8d8c
to
bc97cf7
Compare
| @@ -591,7 +591,7 @@ func TestManagingReconciler(t *testing.T) { | |||
|
|
|||
| // test case 7: the reconciler-manager should mount the git-creds volumes again if the auth type requires a git secret | |||
| nt.T.Log("Switch the auth type gcpserviceaccount to ssh") | |||
| nt.MustMergePatch(rs, `{"spec":{"git":{"auth":"ssh","secretRef":{"name":"git-creds"}}}}`) | |||
There was a problem hiding this comment.
I agree with this stricter validation, but just wanted to point out that this could be temporarily disruptive for some users if they have gcpServiceAccountEmail set for other auth types
There was a problem hiding this comment.
I was also thinking about it. It sounds like we don't have a unified rule to validate the spec.
Currently, the secret ref is allowed only when it is required by certain auth types, but gcpServiceAccountEmail doesn't have such requirement.
On the other hand, spec.git/spec.oci/spec.helm can co-exist even it is not the corresponding source type.
For secret ref, it makes sense to be restrictive because the presence will trigger a copy of the Secret in the c-m-s Namespace. Due to the restriction, this PR makes a consistent behavior between secret ref and gcpServiceAccountEmail. From that perspective, I feel it is okay to ask users to update their configs to follow the same rule.
There was a problem hiding this comment.
Making validation stricter is considered breaking. Agreed that this is inconsistent, but we should try our best to avoid breaking users. Unless allowing gcpServiceAccountEmail has huge downside, we should just keep allowing it.
There was a problem hiding this comment.
What about also loosening the restriction on the secretRef validation? Basically, any field that is not needed will be ignored.
That won't break any users, but it makes all fields be validated in a consistent way.
There was a problem hiding this comment.
Loosening it has the downside of causing the Secret to be copied over to c-m-s as you mentioned, but I think that might be fine if we prefer keeping the rule consistent.
There was a problem hiding this comment.
We can copy it over only when it is used by certain auth types.
There was a problem hiding this comment.
Assuming this is going to be cherry picked, I don't think we need to alter the behavior with this change. I think it would be preferable to minimize the changes for the cherry pick
If desired we can change the behavior for the next minor release
There was a problem hiding this comment.
Yes, I was going to say it doesn't need to be cherrypicked if we don't add more validations.
This can wait until the next minor release.
nan-yu
force-pushed
the
validation
branch
5 times, most recently
from
3d11c4f
to
c0a746a
Compare
When fields in RSync spec is not used, some are allowed to be present, such as `spec.git`, `spec.oci`, `spec.helm`, and `spec.xxx.gcpServiceAccountEmail`. However, `spec.xxx.secretRef` is not allowed when it is not used. The reconciler-manager already checks if it is the proper auth type before copying to the config-management-system Namespace, so it is okay to loosen the validation for this field. This commit also refactors the auth types to reduce code duplication.
This commit covers more failure scenarios with the redudant fields validaiton.