-
Notifications
You must be signed in to change notification settings - Fork 173
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
Add validation to TargetPath and ValuesKey #520
Conversation
@@ -65,6 +67,11 @@ import ( | |||
"github.com/fluxcd/helm-controller/internal/util" | |||
) | |||
|
|||
var ( | |||
targetPathMaxLen = 250 | |||
targetPathRegex = regexp.MustCompilePOSIX(`^([a-zA-Z0-9_.\\\/]|\[[0-9]{1,5}\])+$`) |
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.
We could set these in the CRD as kubebuilder annotations, for the Kubernetes API to perform the validation instead of us. If the validation fails for in-cluster objects, users could fix them in Git.
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.
Updated the PR accordingly. The only downside of this approach is that existing objects which are made invalid by the new CRD will need to be abide by the new validation before the API Server allows them to be deleted.
That being said, the new rules are largely to formalise the expected format and length, and are not expected to break existing setups.
The kubeconfig.key could use the same validation. |
Let's do that as a 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.
Thanks a lot for clarifying the context of this change!
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
Thanks @pjbgf
name string | ||
resources []runtime.Object | ||
references []v2.ValuesReference | ||
values string |
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 doesn't seem to be used in any of the cases below.
067854e
to
451795a
Compare
Formalises the API requirements around TargetPath and ValuesKey, which were the two fields missing validation within ValuesReference. In both cases the validation was introduced at CRD level, so that the apiserver will enforce it. ValuesKey must be a valid Data Key. Therefore the same logic used by upstream Kubernetes is reused here to ensure a valid key is being used. For TargetPath a loose regex is being used to largely represent the expected format. A max length of 250 is now being enforced. This is a breaking change, as invalid TargetPath and ValuesKey will now be rejected by the apiserver, instead of being accepted and potentially failing at reconciliation time. Signed-off-by: Paulo Gomes <[email protected]>
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!
Formalises the API requirements around TargetPath and ValuesKey,
which were the two fields missing validation within ValuesReference.
In both cases the validation was introduced at CRD level, so that
the apiserver will enforce it.
ValuesKey must be a valid Data Key. Therefore the same logic used by
upstream Kubernetes is reused here to ensure a valid key is being used.
For TargetPath a loose regex is being used to largely represent the
expected format. A max length of 250 is now being enforced.
This is a breaking change, as invalid TargetPath and ValuesKey will now
be rejected by the apiserver, instead of being accepted and potentially
failing at reconciliation time.