Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 4 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,6 @@ linters:
exclusions:
generated: lax
presets: [comments, common-false-positives, legacy, std-error-handling]
rules:
- linters:
- modernize
text: .*
paths: [third_party, builtin$, examples$]
warn-unused: true
settings:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,8 @@ spec:
- type
x-kubernetes-list-type: map
type: object
required:
- spec
type: object
served: true
storage: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1157,6 +1157,8 @@ spec:
- type
x-kubernetes-list-type: map
type: object
required:
- spec
type: object
served: true
storage: true
Expand Down
16 changes: 10 additions & 6 deletions pkg/apis/policy/v1alpha1/types_certificaterequestpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,23 @@ var CertificateRequestPolicyKind = "CertificateRequestPolicy"
// makes decisions on whether applicable CertificateRequests should be approved
// or denied.
type CertificateRequestPolicy struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
metav1.TypeMeta `json:",inline"`
// +optional
metav1.ObjectMeta `json:"metadata"`
Copy link
Member Author

Choose a reason for hiding this comment

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

In this KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/2335-vanilla-crd-openapi-subset-structural-schemas#metadata, it looks like the metadata field is not required.

I think it is fine to always have this field in the marshalled resource.


Spec CertificateRequestPolicySpec `json:"spec,omitempty"`
Status CertificateRequestPolicyStatus `json:"status,omitempty"`
Spec CertificateRequestPolicySpec `json:"spec"`
Copy link
Member Author

Choose a reason for hiding this comment

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

The spec probably should not have been optional.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we should do this, but it's technically a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

That should not be an issue imo, a CertificateRequestPolicy without Spec should have resulted in an error before too.

// +optional
Status CertificateRequestPolicyStatus `json:"status,omitzero"`
Copy link
Member Author

Choose a reason for hiding this comment

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

@erikgb I believe in the marshalled object, it would be cleaner to not have a status field if it is empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree.

}

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// CertificateRequestPolicyList is a list of CertificateRequestPolicies.
type CertificateRequestPolicyList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []CertificateRequestPolicy `json:"items"`
// +optional
metav1.ListMeta `json:"metadata"`

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Unintentional change?

Copy link
Member Author

Choose a reason for hiding this comment

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

what is unintentional? the extra newline?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, thanks, just verifying. I think it is OK.

Items []CertificateRequestPolicy `json:"items"`
}

// CertificateRequestPolicySpec defines the desired state of
Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/approver/constraints/evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (c constraints) Evaluate(_ context.Context, policy *policyapi.CertificateRe

// decodePublicKey will return the algorithm and size of the given public key.
// If the public key cannot be decoded, an error is returned.
func decodePublicKey(pub interface{}) (cmapi.PrivateKeyAlgorithm, int, error) {
func decodePublicKey(pub any) (cmapi.PrivateKeyAlgorithm, int, error) {
switch pubKey := pub.(type) {
case *rsa.PublicKey:
return cmapi.RSAKeyAlgorithm, pubKey.N.BitLen(), nil
Expand Down
8 changes: 4 additions & 4 deletions pkg/internal/approver/validation/serviceaccount.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ func ServiceAccountLib() cel.EnvOption {
}

// ConvertToNative implements ref.Val.ConvertToNative.
func (sa ServiceAccount) ConvertToNative(typeDesc reflect.Type) (interface{}, error) {
if reflect.TypeOf(sa).AssignableTo(typeDesc) {
func (sa ServiceAccount) ConvertToNative(typeDesc reflect.Type) (any, error) {
if reflect.TypeFor[ServiceAccount]().AssignableTo(typeDesc) {
return sa, nil
}
if reflect.TypeOf("").AssignableTo(typeDesc) {
if reflect.TypeFor[string]().AssignableTo(typeDesc) {
return serviceaccount.MakeUsername(sa.Namespace, sa.Name), nil
}
return nil, fmt.Errorf("type conversion error from 'serviceaccount' to '%v'", typeDesc)
Expand Down Expand Up @@ -77,7 +77,7 @@ func (sa ServiceAccount) Type() ref.Type {
}

// Value implements ref.Val.Value.
func (sa ServiceAccount) Value() interface{} {
func (sa ServiceAccount) Value() any {
return sa
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/internal/approver/validation/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (v *validator) Validate(value string, request cmapi.CertificateRequest) (bo
return false, errors.New("must compile first")
}

vars := map[string]interface{}{
vars := map[string]any{
varSelf: value,
varRequest: &CertificateRequest{
Name: request.GetName(),
Expand Down
11 changes: 2 additions & 9 deletions pkg/internal/webhook/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"
"fmt"
"slices"
"sort"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -75,15 +76,7 @@ func (v *validator) validate(ctx context.Context, obj runtime.Object) (admission
// Ensure no plugin has been defined which is not registered.
var unrecognisedNames []string
for name := range policy.Spec.Plugins {
var found bool
for _, known := range v.registeredPlugins {
if name == known {
found = true
break
}
}

if !found {
if !slices.Contains(v.registeredPlugins, name) {
unrecognisedNames = append(unrecognisedNames, name)
}
}
Expand Down