Drop legacy validation logic for certificates API

This commit is contained in:
Jordan Liggitt 2021-08-09 12:37:34 -04:00
parent befffd1565
commit b1d344db44
4 changed files with 175 additions and 318 deletions

View File

@ -25,14 +25,12 @@ import (
v1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/apimachinery/pkg/util/sets"
utilvalidation "k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/util/validation/field"
utilcert "k8s.io/client-go/util/cert"
"k8s.io/kubernetes/pkg/apis/certificates"
certificatesv1beta1 "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
apivalidation "k8s.io/kubernetes/pkg/apis/core/validation"
)
@ -139,8 +137,8 @@ func ValidateCertificateRequestName(name string, prefix bool) []string {
return nil
}
func ValidateCertificateSigningRequestCreate(csr *certificates.CertificateSigningRequest, version schema.GroupVersion) field.ErrorList {
opts := getValidationOptions(version, csr, nil)
func ValidateCertificateSigningRequestCreate(csr *certificates.CertificateSigningRequest) field.ErrorList {
opts := getValidationOptions(csr, nil)
return validateCertificateSigningRequest(csr, opts)
}
@ -347,19 +345,19 @@ func ValidateCertificateSigningRequestSignerName(fldPath *field.Path, signerName
return el
}
func ValidateCertificateSigningRequestUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest, version schema.GroupVersion) field.ErrorList {
opts := getValidationOptions(version, newCSR, oldCSR)
func ValidateCertificateSigningRequestUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest) field.ErrorList {
opts := getValidationOptions(newCSR, oldCSR)
return validateCertificateSigningRequestUpdate(newCSR, oldCSR, opts)
}
func ValidateCertificateSigningRequestStatusUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest, version schema.GroupVersion) field.ErrorList {
opts := getValidationOptions(version, newCSR, oldCSR)
func ValidateCertificateSigningRequestStatusUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest) field.ErrorList {
opts := getValidationOptions(newCSR, oldCSR)
opts.allowSettingCertificate = true
return validateCertificateSigningRequestUpdate(newCSR, oldCSR, opts)
}
func ValidateCertificateSigningRequestApprovalUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest, version schema.GroupVersion) field.ErrorList {
opts := getValidationOptions(version, newCSR, oldCSR)
func ValidateCertificateSigningRequestApprovalUpdate(newCSR, oldCSR *certificates.CertificateSigningRequest) field.ErrorList {
opts := getValidationOptions(newCSR, oldCSR)
opts.allowSettingApprovalConditions = true
return validateCertificateSigningRequestUpdate(newCSR, oldCSR, opts)
}
@ -420,24 +418,19 @@ func findConditions(csr *certificates.CertificateSigningRequest, conditionType c
// compatible with the specified version and existing CSR.
// oldCSR may be nil if this is a create request.
// validation options related to subresource-specific capabilities are set to false.
func getValidationOptions(version schema.GroupVersion, newCSR, oldCSR *certificates.CertificateSigningRequest) certificateValidationOptions {
func getValidationOptions(newCSR, oldCSR *certificates.CertificateSigningRequest) certificateValidationOptions {
return certificateValidationOptions{
allowResettingCertificate: allowResettingCertificate(version),
allowResettingCertificate: false,
allowBothApprovedAndDenied: allowBothApprovedAndDenied(oldCSR),
allowLegacySignerName: allowLegacySignerName(version, oldCSR),
allowDuplicateConditionTypes: allowDuplicateConditionTypes(version, oldCSR),
allowEmptyConditionType: allowEmptyConditionType(version, oldCSR),
allowArbitraryCertificate: allowArbitraryCertificate(version, newCSR, oldCSR),
allowDuplicateUsages: allowDuplicateUsages(version, oldCSR),
allowUnknownUsages: allowUnknownUsages(version, oldCSR),
allowLegacySignerName: allowLegacySignerName(oldCSR),
allowDuplicateConditionTypes: allowDuplicateConditionTypes(oldCSR),
allowEmptyConditionType: allowEmptyConditionType(oldCSR),
allowArbitraryCertificate: allowArbitraryCertificate(newCSR, oldCSR),
allowDuplicateUsages: allowDuplicateUsages(oldCSR),
allowUnknownUsages: allowUnknownUsages(oldCSR),
}
}
func allowResettingCertificate(version schema.GroupVersion) bool {
// compatibility with v1beta1
return version == certificatesv1beta1.SchemeGroupVersion
}
func allowBothApprovedAndDenied(oldCSR *certificates.CertificateSigningRequest) bool {
if oldCSR == nil {
return false
@ -455,10 +448,8 @@ func allowBothApprovedAndDenied(oldCSR *certificates.CertificateSigningRequest)
return approved && denied
}
func allowLegacySignerName(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
func allowLegacySignerName(oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case oldCSR != nil && oldCSR.Spec.SignerName == certificates.LegacyUnknownSignerName:
return true // compatibility with existing data
default:
@ -466,10 +457,8 @@ func allowLegacySignerName(version schema.GroupVersion, oldCSR *certificates.Cer
}
}
func allowDuplicateConditionTypes(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
func allowDuplicateConditionTypes(oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case oldCSR != nil && hasDuplicateConditionTypes(oldCSR):
return true // compatibility with existing data
default:
@ -487,10 +476,8 @@ func hasDuplicateConditionTypes(csr *certificates.CertificateSigningRequest) boo
return false
}
func allowEmptyConditionType(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
func allowEmptyConditionType(oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case oldCSR != nil && hasEmptyConditionType(oldCSR):
return true // compatibility with existing data
default:
@ -506,10 +493,8 @@ func hasEmptyConditionType(csr *certificates.CertificateSigningRequest) bool {
return false
}
func allowArbitraryCertificate(version schema.GroupVersion, newCSR, oldCSR *certificates.CertificateSigningRequest) bool {
func allowArbitraryCertificate(newCSR, oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case newCSR != nil && oldCSR != nil && bytes.Equal(newCSR.Status.Certificate, oldCSR.Status.Certificate):
return true // tolerate updates that don't touch status.certificate
case oldCSR != nil && validateCertificate(oldCSR.Status.Certificate) != nil:
@ -519,10 +504,8 @@ func allowArbitraryCertificate(version schema.GroupVersion, newCSR, oldCSR *cert
}
}
func allowUnknownUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
func allowUnknownUsages(oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case oldCSR != nil && hasUnknownUsage(oldCSR.Spec.Usages):
return true // compatibility with existing data
default:
@ -539,10 +522,8 @@ func hasUnknownUsage(usages []certificates.KeyUsage) bool {
return false
}
func allowDuplicateUsages(version schema.GroupVersion, oldCSR *certificates.CertificateSigningRequest) bool {
func allowDuplicateUsages(oldCSR *certificates.CertificateSigningRequest) bool {
switch {
case version == certificatesv1beta1.SchemeGroupVersion:
return true // compatibility with v1beta1
case oldCSR != nil && hasDuplicateUsage(oldCSR.Spec.Usages):
return true // compatibility with existing data
default:

View File

@ -29,12 +29,10 @@ import (
"time"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/client-go/util/certificate/csr"
capi "k8s.io/kubernetes/pkg/apis/certificates"
capiv1beta1 "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
"k8s.io/kubernetes/pkg/apis/core"
"k8s.io/utils/pointer"
)
@ -54,7 +52,6 @@ func TestValidateCertificateSigningRequestCreate(t *testing.T) {
maxLengthSignerName := fmt.Sprintf("%s/%s.%s", maxLengthFQDN, repeatString("a", 63), repeatString("a", 253))
tests := map[string]struct {
csr capi.CertificateSigningRequest
gv schema.GroupVersion
errs field.ErrorList
}{
"CSR with empty request data should fail": {
@ -342,19 +339,7 @@ func TestValidateCertificateSigningRequestCreate(t *testing.T) {
field.Required(specPath.Child("usages"), "usages must be provided"),
},
},
"unknown and duplicate usages - v1beta1": {
gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1beta1"},
csr: capi.CertificateSigningRequest{
ObjectMeta: validObjectMeta,
Spec: capi.CertificateSigningRequestSpec{
Usages: []capi.KeyUsage{"unknown", "unknown"},
Request: newCSRPEM(t),
SignerName: validSignerName,
},
},
},
"unknown and duplicate usages - v1": {
gv: schema.GroupVersion{Group: capi.SchemeGroupVersion.Group, Version: "v1"},
"unknown and duplicate usages": {
csr: capi.CertificateSigningRequest{
ObjectMeta: validObjectMeta,
Spec: capi.CertificateSigningRequestSpec{
@ -372,7 +357,7 @@ func TestValidateCertificateSigningRequestCreate(t *testing.T) {
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
el := ValidateCertificateSigningRequestCreate(&test.csr, test.gv)
el := ValidateCertificateSigningRequestCreate(&test.csr)
if !reflect.DeepEqual(el, test.errs) {
t.Errorf("returned and expected errors did not match - expected\n%v\nbut got\n%v", test.errs.ToAggregate(), el.ToAggregate())
}
@ -420,59 +405,23 @@ func newCSRPEM(t *testing.T) []byte {
func Test_getValidationOptions(t *testing.T) {
tests := []struct {
name string
version schema.GroupVersion
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
want certificateValidationOptions
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
want certificateValidationOptions
}{
{
name: "v1beta1 compatible create",
version: capiv1beta1.SchemeGroupVersion,
oldCSR: nil,
want: certificateValidationOptions{
allowResettingCertificate: true,
allowBothApprovedAndDenied: false,
allowLegacySignerName: true,
allowDuplicateConditionTypes: true,
allowEmptyConditionType: true,
allowArbitraryCertificate: true,
allowUnknownUsages: true,
allowDuplicateUsages: true,
},
name: "strict create",
oldCSR: nil,
want: certificateValidationOptions{},
},
{
name: "v1 strict create",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
oldCSR: nil,
want: certificateValidationOptions{},
name: "strict update",
oldCSR: &capi.CertificateSigningRequest{},
want: certificateValidationOptions{},
},
{
name: "v1beta1 compatible update",
version: capiv1beta1.SchemeGroupVersion,
oldCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved}, {Type: capi.CertificateDenied}},
}},
want: certificateValidationOptions{
allowResettingCertificate: true,
allowBothApprovedAndDenied: true, // existing object has both approved and denied
allowLegacySignerName: true,
allowDuplicateConditionTypes: true,
allowEmptyConditionType: true,
allowArbitraryCertificate: true,
allowUnknownUsages: true,
allowDuplicateUsages: true,
},
},
{
name: "v1 strict update",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
oldCSR: &capi.CertificateSigningRequest{},
want: certificateValidationOptions{},
},
{
name: "v1 compatible update, approved+denied",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
name: "compatible update, approved+denied",
oldCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved}, {Type: capi.CertificateDenied}},
}},
@ -481,16 +430,14 @@ func Test_getValidationOptions(t *testing.T) {
},
},
{
name: "v1 compatible update, legacy signerName",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{SignerName: capi.LegacyUnknownSignerName}},
name: "compatible update, legacy signerName",
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{SignerName: capi.LegacyUnknownSignerName}},
want: certificateValidationOptions{
allowLegacySignerName: true,
},
},
{
name: "v1 compatible update, duplicate condition types",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
name: "compatible update, duplicate condition types",
oldCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved}, {Type: capi.CertificateApproved}},
}},
@ -499,8 +446,7 @@ func Test_getValidationOptions(t *testing.T) {
},
},
{
name: "v1 compatible update, empty condition types",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
name: "compatible update, empty condition types",
oldCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{}},
}},
@ -509,8 +455,7 @@ func Test_getValidationOptions(t *testing.T) {
},
},
{
name: "v1 compatible update, no diff to certificate",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
name: "compatible update, no diff to certificate",
newCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Certificate: validCertificate,
}},
@ -522,8 +467,7 @@ func Test_getValidationOptions(t *testing.T) {
},
},
{
name: "v1 compatible update, existing invalid certificate",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
name: "compatible update, existing invalid certificate",
newCSR: &capi.CertificateSigningRequest{Status: capi.CertificateSigningRequestStatus{
Certificate: []byte(`new - no PEM blocks`),
}},
@ -535,17 +479,15 @@ func Test_getValidationOptions(t *testing.T) {
},
},
{
name: "v1 compatible update, existing unknown usages",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"unknown"}}},
name: "compatible update, existing unknown usages",
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"unknown"}}},
want: certificateValidationOptions{
allowUnknownUsages: true,
},
},
{
name: "v1 compatible update, existing duplicate usages",
version: schema.GroupVersion{Group: "certificates.k8s.io", Version: "v1"},
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"any", "any"}}},
name: "compatible update, existing duplicate usages",
oldCSR: &capi.CertificateSigningRequest{Spec: capi.CertificateSigningRequestSpec{Usages: []capi.KeyUsage{"any", "any"}}},
want: certificateValidationOptions{
allowDuplicateUsages: true,
},
@ -553,7 +495,7 @@ func Test_getValidationOptions(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := getValidationOptions(tt.version, tt.newCSR, tt.oldCSR); !reflect.DeepEqual(got, tt.want) {
if got := getValidationOptions(tt.newCSR, tt.oldCSR); !reflect.DeepEqual(got, tt.want) {
t.Errorf("got %#v\nwant %#v", got, tt.want)
}
})
@ -574,10 +516,10 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
}
tests := []struct {
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
versionErrs map[string][]string
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
errs []string
}{
{
name: "no-op",
@ -595,9 +537,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not add a condition of type "Approved"`},
"v1beta1": {`status.conditions: Forbidden: updates may not add a condition of type "Approved"`},
errs: []string{
`status.conditions: Forbidden: updates may not add a condition of type "Approved"`,
},
},
{
@ -606,9 +547,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`,
},
},
{
@ -617,9 +557,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateDenied, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not add a condition of type "Denied"`},
"v1beta1": {`status.conditions: Forbidden: updates may not add a condition of type "Denied"`},
errs: []string{
`status.conditions: Forbidden: updates may not add a condition of type "Denied"`,
},
},
{
@ -628,9 +567,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateDenied, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`,
},
},
{
@ -638,8 +576,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
errs: []string{},
},
{
name: "remove Failed condition",
@ -647,9 +585,8 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`,
},
},
{
@ -658,29 +595,26 @@ func TestValidateCertificateSigningRequestUpdate(t *testing.T) {
Certificate: validCertificate,
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.certificate: Forbidden: updates may not set certificate content`},
"v1beta1": {`status.certificate: Forbidden: updates may not set certificate content`},
errs: []string{
`status.certificate: Forbidden: updates may not set certificate content`,
},
},
}
for _, tt := range tests {
for _, version := range []string{"v1", "v1beta1"} {
t.Run(tt.name+"_"+version, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestUpdate(tt.newCSR, tt.oldCSR, schema.GroupVersion{Group: capi.GroupName, Version: version}) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.versionErrs[version]...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
t.Run(tt.name, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestUpdate(tt.newCSR, tt.oldCSR) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.errs...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
}
@ -698,10 +632,10 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
}
tests := []struct {
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
versionErrs map[string][]string
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
errs []string
}{
{
name: "no-op",
@ -732,9 +666,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not add a condition of type "Approved"`},
"v1beta1": {`status.conditions: Forbidden: updates may not add a condition of type "Approved"`},
errs: []string{
`status.conditions: Forbidden: updates may not add a condition of type "Approved"`,
},
},
{
@ -743,9 +676,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`,
},
},
{
@ -754,9 +686,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateDenied, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not add a condition of type "Denied"`},
"v1beta1": {`status.conditions: Forbidden: updates may not add a condition of type "Denied"`},
errs: []string{
`status.conditions: Forbidden: updates may not add a condition of type "Denied"`,
},
},
{
@ -765,9 +696,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateDenied, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`,
},
},
{
@ -775,8 +705,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
errs: []string{},
},
{
name: "remove Failed condition",
@ -784,9 +714,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`,
},
},
{
@ -794,8 +723,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Certificate: validCertificate,
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
errs: []string{},
},
{
name: "set invalid certificate",
@ -803,8 +732,8 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
Certificate: invalidCertificateNoPEM,
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.certificate: Invalid value: "<certificate data>": must contain at least one CERTIFICATE PEM block`},
errs: []string{
`status.certificate: Invalid value: "<certificate data>": must contain at least one CERTIFICATE PEM block`,
},
},
{
@ -815,28 +744,26 @@ func TestValidateCertificateSigningRequestStatusUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Certificate: invalidCertificateNoPEM,
}},
versionErrs: map[string][]string{
"v1": {`status.certificate: Forbidden: updates may not modify existing certificate content`},
errs: []string{
`status.certificate: Forbidden: updates may not modify existing certificate content`,
},
},
}
for _, tt := range tests {
for _, version := range []string{"v1", "v1beta1"} {
t.Run(tt.name+"_"+version, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestStatusUpdate(tt.newCSR, tt.oldCSR, schema.GroupVersion{Group: capi.GroupName, Version: version}) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.versionErrs[version]...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
t.Run(tt.name, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestStatusUpdate(tt.newCSR, tt.oldCSR) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.errs...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
}
@ -854,10 +781,10 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
}
tests := []struct {
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
versionErrs map[string][]string
name string
newCSR *capi.CertificateSigningRequest
oldCSR *capi.CertificateSigningRequest
errs []string
}{
{
name: "no-op",
@ -882,9 +809,8 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateApproved, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Approved"`,
},
},
{
@ -900,9 +826,8 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateDenied, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Denied"`,
},
},
{
@ -910,8 +835,8 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
newCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMeta, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
errs: []string{},
},
{
name: "remove Failed condition",
@ -919,9 +844,8 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec, Status: capi.CertificateSigningRequestStatus{
Conditions: []capi.CertificateSigningRequestCondition{{Type: capi.CertificateFailed, Status: core.ConditionTrue}},
}},
versionErrs: map[string][]string{
"v1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
"v1beta1": {`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`},
errs: []string{
`status.conditions: Forbidden: updates may not remove a condition of type "Failed"`,
},
},
{
@ -930,29 +854,26 @@ func TestValidateCertificateSigningRequestApprovalUpdate(t *testing.T) {
Certificate: validCertificate,
}},
oldCSR: &capi.CertificateSigningRequest{ObjectMeta: validUpdateMetaWithFinalizers, Spec: validSpec},
versionErrs: map[string][]string{
"v1": {`status.certificate: Forbidden: updates may not set certificate content`},
"v1beta1": {`status.certificate: Forbidden: updates may not set certificate content`},
errs: []string{
`status.certificate: Forbidden: updates may not set certificate content`,
},
},
}
for _, tt := range tests {
for _, version := range []string{"v1", "v1beta1"} {
t.Run(tt.name+"_"+version, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestApprovalUpdate(tt.newCSR, tt.oldCSR, schema.GroupVersion{Group: capi.GroupName, Version: version}) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.versionErrs[version]...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
t.Run(tt.name, func(t *testing.T) {
gotErrs := sets.NewString()
for _, err := range ValidateCertificateSigningRequestApprovalUpdate(tt.newCSR, tt.oldCSR) {
gotErrs.Insert(err.Error())
}
wantErrs := sets.NewString(tt.errs...)
for _, missing := range wantErrs.Difference(gotErrs).List() {
t.Errorf("missing expected error: %s", missing)
}
for _, unexpected := range gotErrs.Difference(wantErrs).List() {
t.Errorf("unexpected error: %s", unexpected)
}
})
}
}

View File

@ -20,12 +20,10 @@ import (
"context"
"fmt"
certificatesv1beta1 "k8s.io/api/certificates/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/apiserver/pkg/registry/generic"
@ -120,7 +118,7 @@ func (csrStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object
// Validate validates a new CSR. Validation must check for a correct signature.
func (csrStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
csr := obj.(*certificates.CertificateSigningRequest)
return validation.ValidateCertificateSigningRequestCreate(csr, requestGroupVersion(ctx))
return validation.ValidateCertificateSigningRequestCreate(csr)
}
// WarningsOnCreate returns warnings for the creation of the given object.
@ -133,7 +131,7 @@ func (csrStrategy) Canonicalize(obj runtime.Object) {}
func (csrStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
oldCSR := old.(*certificates.CertificateSigningRequest)
newCSR := obj.(*certificates.CertificateSigningRequest)
return validation.ValidateCertificateSigningRequestUpdate(newCSR, oldCSR, requestGroupVersion(ctx))
return validation.ValidateCertificateSigningRequestUpdate(newCSR, oldCSR)
}
// WarningsOnUpdate returns warnings for the given update.
@ -181,20 +179,11 @@ func (csrStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.
// Updating /status should not modify spec
newCSR.Spec = oldCSR.Spec
switch requestGroupVersion(ctx) {
case certificatesv1beta1.SchemeGroupVersion:
// Specifically preserve existing Approved/Denied conditions.
// If we cannot (if the status update attempted to add/remove Approved/Denied conditions), revert to old conditions for backwards compatibility.
if !preserveConditionInstances(newCSR, oldCSR, certificates.CertificateApproved) || !preserveConditionInstances(newCSR, oldCSR, certificates.CertificateDenied) {
newCSR.Status.Conditions = oldCSR.Status.Conditions
}
default:
// Specifically preserve existing Approved/Denied conditions.
// Adding/removing Approved/Denied conditions will cause these to fail,
// and the change in Approved/Denied conditions will produce a validation error
preserveConditionInstances(newCSR, oldCSR, certificates.CertificateApproved)
preserveConditionInstances(newCSR, oldCSR, certificates.CertificateDenied)
}
// Specifically preserve existing Approved/Denied conditions.
// Adding/removing Approved/Denied conditions will cause these to fail,
// and the change in Approved/Denied conditions will produce a validation error
preserveConditionInstances(newCSR, oldCSR, certificates.CertificateApproved)
preserveConditionInstances(newCSR, oldCSR, certificates.CertificateDenied)
populateConditionTimestamps(newCSR, oldCSR)
}
@ -255,7 +244,7 @@ func populateConditionTimestamps(newCSR, oldCSR *certificates.CertificateSigning
}
func (csrStatusStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
return validation.ValidateCertificateSigningRequestStatusUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest), requestGroupVersion(ctx))
return validation.ValidateCertificateSigningRequestStatusUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest))
}
// WarningsOnUpdate returns warnings for the given update.
@ -307,7 +296,7 @@ func (csrApprovalStrategy) PrepareForUpdate(ctx context.Context, obj, old runtim
}
func (csrApprovalStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
return validation.ValidateCertificateSigningRequestApprovalUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest), requestGroupVersion(ctx))
return validation.ValidateCertificateSigningRequestApprovalUpdate(obj.(*certificates.CertificateSigningRequest), old.(*certificates.CertificateSigningRequest))
}
// WarningsOnUpdate returns warnings for the given update.
@ -332,11 +321,3 @@ func SelectableFields(obj *certificates.CertificateSigningRequest) fields.Set {
}
return generic.MergeFieldsSets(objectMetaFieldsSet, csrSpecificFieldsSet)
}
// requestGroupVersion returns the group/version associated with the given context, or a zero-value group/version
func requestGroupVersion(ctx context.Context) schema.GroupVersion {
if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
return schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
}
return schema.GroupVersion{}
}

View File

@ -174,19 +174,16 @@ func TestStatusUpdate(t *testing.T) {
}()
tests := []struct {
name string
newObj *certapi.CertificateSigningRequest
oldObj *certapi.CertificateSigningRequest
expectedObjs map[string]*certapi.CertificateSigningRequest
name string
newObj *certapi.CertificateSigningRequest
oldObj *certapi.CertificateSigningRequest
expectedObj *certapi.CertificateSigningRequest
}{
{
name: "no-op",
newObj: &certapi.CertificateSigningRequest{},
oldObj: &certapi.CertificateSigningRequest{},
expectedObjs: map[string]*certapi.CertificateSigningRequest{
"v1": {},
"v1beta1": {},
},
name: "no-op",
newObj: &certapi.CertificateSigningRequest{},
oldObj: &certapi.CertificateSigningRequest{},
expectedObj: &certapi.CertificateSigningRequest{},
},
{
name: "adding failed condition to existing approved/denied conditions",
@ -211,29 +208,15 @@ func TestStatusUpdate(t *testing.T) {
},
},
},
expectedObjs: map[string]*certapi.CertificateSigningRequest{
expectedObj: &certapi.CertificateSigningRequest{
// preserve existing Approved/Denied conditions
"v1": {
Status: certapi.CertificateSigningRequestStatus{
Conditions: []certapi.CertificateSigningRequestCondition{
{Type: certapi.CertificateFailed, LastUpdateTime: now, LastTransitionTime: now},
{Type: certapi.CertificateDenied, LastUpdateTime: now, LastTransitionTime: later, Reason: "because1"},
{Type: certapi.CertificateApproved, LastUpdateTime: now, LastTransitionTime: later, Reason: "because2"},
{Type: certapi.CertificateDenied, LastUpdateTime: later, LastTransitionTime: later, Reason: "because3"},
{Type: certapi.CertificateApproved, LastUpdateTime: later, LastTransitionTime: later, Reason: "because4"},
},
},
},
// preserve existing Approved/Denied conditions
"v1beta1": {
Status: certapi.CertificateSigningRequestStatus{
Conditions: []certapi.CertificateSigningRequestCondition{
{Type: certapi.CertificateFailed, LastUpdateTime: now, LastTransitionTime: now},
{Type: certapi.CertificateDenied, LastUpdateTime: now, LastTransitionTime: later, Reason: "because1"},
{Type: certapi.CertificateApproved, LastUpdateTime: now, LastTransitionTime: later, Reason: "because2"},
{Type: certapi.CertificateDenied, LastUpdateTime: later, LastTransitionTime: later, Reason: "because3"},
{Type: certapi.CertificateApproved, LastUpdateTime: later, LastTransitionTime: later, Reason: "because4"},
},
Status: certapi.CertificateSigningRequestStatus{
Conditions: []certapi.CertificateSigningRequestCondition{
{Type: certapi.CertificateFailed, LastUpdateTime: now, LastTransitionTime: now},
{Type: certapi.CertificateDenied, LastUpdateTime: now, LastTransitionTime: later, Reason: "because1"},
{Type: certapi.CertificateApproved, LastUpdateTime: now, LastTransitionTime: later, Reason: "because2"},
{Type: certapi.CertificateDenied, LastUpdateTime: later, LastTransitionTime: later, Reason: "because3"},
{Type: certapi.CertificateApproved, LastUpdateTime: later, LastTransitionTime: later, Reason: "because4"},
},
},
},
@ -248,33 +231,24 @@ func TestStatusUpdate(t *testing.T) {
},
},
oldObj: &certapi.CertificateSigningRequest{},
expectedObjs: map[string]*certapi.CertificateSigningRequest{
expectedObj: &certapi.CertificateSigningRequest{
// preserved submitted conditions if existing Approved/Denied conditions could not be copied over (will fail validation)
"v1": {
Status: certapi.CertificateSigningRequestStatus{
Conditions: []certapi.CertificateSigningRequestCondition{
{Type: certapi.CertificateApproved, LastUpdateTime: now, LastTransitionTime: now},
},
Status: certapi.CertificateSigningRequestStatus{
Conditions: []certapi.CertificateSigningRequestCondition{
{Type: certapi.CertificateApproved, LastUpdateTime: now, LastTransitionTime: now},
},
},
// reset conditions to existing conditions if Approved/Denied conditions could not be copied over
"v1beta1": {
Status: certapi.CertificateSigningRequestStatus{},
},
},
},
}
for _, tt := range tests {
for _, version := range []string{"v1", "v1beta1"} {
t.Run(tt.name+"_"+version, func(t *testing.T) {
ctx := genericapirequest.WithRequestInfo(context.TODO(), &genericapirequest.RequestInfo{APIGroup: "certificates.k8s.io", APIVersion: version})
obj := tt.newObj.DeepCopy()
StatusStrategy.PrepareForUpdate(ctx, obj, tt.oldObj.DeepCopy())
if !reflect.DeepEqual(obj, tt.expectedObjs[version]) {
t.Errorf("object diff: %s", diff.ObjectDiff(obj, tt.expectedObjs[version]))
}
})
}
t.Run(tt.name, func(t *testing.T) {
obj := tt.newObj.DeepCopy()
StatusStrategy.PrepareForUpdate(context.TODO(), obj, tt.oldObj.DeepCopy())
if !reflect.DeepEqual(obj, tt.expectedObj) {
t.Errorf("object diff: %s", diff.ObjectDiff(obj, tt.expectedObj))
}
})
}
}