Skip to content
This repository has been archived by the owner on Jun 25, 2024. It is now read-only.

Validate fields of OpenstackDataPlaneServiceCert in OpenStackDataPlaneServiceSpec #852

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −6
Draft

Validate fields of OpenstackDataPlaneServiceCert in OpenStackDataPlaneServiceSpec #852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/bases/dataplane.openstack.org_openstackdataplaneservices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ spec:
issuer:
type: string
keyUsages:
default:
- key encipherment
- digital signature
- server auth
items:
enum:
- signing
Expand Down
3 changes: 2 additions & 1 deletion api/v1beta1/openstackdataplaneservice_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ type OpenstackDataPlaneServiceCert struct {

// KeyUsages to be added to the issued cert
// +kubebuilder:validation:Optional
KeyUsages []certmgrv1.KeyUsage `json:"keyUsages,omitempty" yaml:"keyUsages,omitempty"`
// +kubebuilder:default={"key encipherment","digital signature","server auth"}
KeyUsages []certmgrv1.KeyUsage `json:"keyUsages" yaml:"keyUsages"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the default explicit is good. I thought though, that when you mentioned this tightening of the spec would include turning the Contents field into an enum - with "dnsnames" and "ips" as the two possible options. That would result in validation errors in the same way that KeyUsages currently does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contents is list, not a string, enum wouldn't work there because you need list a approved values. Hence the ValidateContents method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but we can -- and that what I thought you wanted to do -- redefine Contents as a list of enums - just like keyusages is a list of certmgrv1.KeyUsage. We can define a new enum of CertificateContents for example.

Copy link
Contributor Author

@jpodivin jpodivin Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That isn't really the point. Kubebuilder enum [0] should be used in cases when field must have only one of the values from approved a list. Not when the field is supposed to be a list of approved values.

Enum validation just doesn't fit our use case here, and we shouldn't try to change the API just to make it fit.
Putting the validation in a function is, imho, a cleaner approach.
[0] https://book.kubebuilder.io/reference/markers/crd-validation.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe enum wasn't the right thing here. My point is that we expect contents to be one of a set of approved values. KeyUsages (https://github.com/cert-manager/cert-manager/blob/eb9f8880fdf9a02c2f85e73cc08617c350717378/pkg/api/util/usages.go#L26) is a similar thing, and we can use an equivalent structure to make that explicit - and thereby enforce it in the same way that KeyUsages are enforced.


// EDPMRoleServiceName is the value of the <role>_service_name variable from
// the edpm-ansible role where this certificate is used. For example if the
Expand Down
42 changes: 38 additions & 4 deletions api/v1beta1/openstackdataplaneservice_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ limitations under the License.
package v1beta1

import (
"fmt"

"golang.org/x/exp/slices"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
Expand Down Expand Up @@ -78,9 +81,15 @@ func (r *OpenStackDataPlaneService) ValidateCreate() (admission.Warnings, error)
}

func (r *OpenStackDataPlaneServiceSpec) ValidateCreate() field.ErrorList {
// TODO(user): fill in your validation logic upon object creation.
var errs field.ErrorList

return field.ErrorList{}
if r.TLSCerts != nil {
for _, v := range r.TLSCerts {
errs = append(errs, v.ValidateContents()...)
}
}

return errs
}

func (r *OpenStackDataPlaneService) ValidateUpdate(original runtime.Object) (admission.Warnings, error) {
Expand All @@ -99,9 +108,15 @@ func (r *OpenStackDataPlaneService) ValidateUpdate(original runtime.Object) (adm
}

func (r *OpenStackDataPlaneServiceSpec) ValidateUpdate() field.ErrorList {
// TODO(user): fill in your validation logic upon object creation.
var errs field.ErrorList

return field.ErrorList{}
if r.TLSCerts != nil {
for _, v := range r.TLSCerts {
errs = append(errs, v.ValidateContents()...)
}
}

return errs
}

func (r *OpenStackDataPlaneService) ValidateDelete() (admission.Warnings, error) {
Expand All @@ -125,3 +140,22 @@ func (r *OpenStackDataPlaneServiceSpec) ValidateDelete() field.ErrorList {

return field.ErrorList{}
}

func (r *OpenstackDataPlaneServiceCert) ValidateContents() field.ErrorList {

var errs field.ErrorList
// "dnsnames" and "ips" are only allowed usages
allowedContents := []string{
"dnsnames",
"ips",
}
for _, val := range r.Contents {

if !slices.Contains(allowedContents, val) {
errs = append(errs, field.Invalid(field.NewPath("spec.tlsCert.Contents"),
r.KeyUsages,
fmt.Sprintf("error validating contents of TLSCert, %s, only valid contents are %v ", val, allowedContents)))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um -- this validation function is all confused. You are defining the allowed contents for "Contents" and checking that - while putting up an error message associated with KeyUsages. That said - if we changed Contents to be an enum - with the appropriate default - would this whole function be necessary at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because it started is function for keyusages. I wanted to see the test results, before rewriting the comment's, logs etc.

return errs
}
4 changes: 4 additions & 0 deletions config/crd/bases/dataplane.openstack.org_openstackdataplaneservices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ spec:
issuer:
type: string
keyUsages:
default:
- key encipherment
- digital signature
- server auth
items:
enum:
- signing
Expand Down
2 changes: 1 addition & 1 deletion docs/assemblies/custom_resources.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ OpenstackDataPlaneServiceCert defines the property of a TLS cert issued for a da
| keyUsages
| KeyUsages to be added to the issued cert
| []certmgrv1.KeyUsage
| false
| true
| edpmRoleServiceName
| EDPMRoleServiceName is the value of the +++<role>+++_service_name variable from the edpm-ansible role where this certificate is used. For example if the certificate is for edpm_ovn from edpm-ansible, EDPMRoleServiceName must be ovn, which matches the edpm_ovn_service_name variable from the role. If not set, OpenStackDataPlaneService.Spec.EDPMServiceType is used. If OpenStackDataPlaneService.Spec.EDPMServiceType is not set, then OpenStackDataPlaneService.Name is used.+++</role>+++
Expand Down
Loading