Skip to content

Commit 0be5a60

Browse files
cert-manager-prow[bot]cert-manager-prow[bot]
authored
Merge pull request cert-manager#8174 from cert-manager-bot/cherry-pick-8160-to-release-1.18
[release-1.18] Add defaulting to Certificate - CertificateRequest comparison
2 parents 6caa368 + a257208 commit 0be5a60

File tree

2 files changed

+252
-2
lines changed

2 files changed

+252
-2
lines changed

‎pkg/util/pki/match.go‎

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,10 @@ import (
2727
"encoding/asn1"
2828
"fmt"
2929
"net"
30-
"reflect"
3130

3231
"k8s.io/apimachinery/pkg/util/sets"
3332

33+
"github.com/cert-manager/cert-manager/pkg/apis/certmanager"
3434
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
3535
"github.com/cert-manager/cert-manager/pkg/util"
3636
)
@@ -224,7 +224,28 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
224224
req.Spec.Duration.Duration != spec.Duration.Duration {
225225
violations = append(violations, "spec.duration")
226226
}
227-
if !reflect.DeepEqual(req.Spec.IssuerRef, spec.IssuerRef) {
227+
// RequestMatchesSpec compares the IssuerRef in the CertificateRequest and
228+
// CertificateSpec, regardless of any differences which are solely due to
229+
// the presence or absence of default group (cert-manager.io) and kind (Issuer).
230+
//
231+
// We do not want to re-issue the Certificate if the user explicitly adds
232+
// the default issuer group and kind.
233+
// Nor do we want to re-issue if the user removes the default issuer group and kind.
234+
//
235+
// And we want to avoid re-issuing if a future version of the cert-manager
236+
// CRDs introduces API defaults for issuerRef group and kind. Specifically,
237+
// we want to gracefully handle a situation where the platform admin
238+
// upgrades the CRDs to a version that has defaults, but not the controller.
239+
// In that situation, when the CRDs are upgraded, the controller
240+
// re-establishes its watches and refreshes its caches with updated Certificates
241+
// and CertificateRequests, containing the new API defaults. But this
242+
// doesn't happen transactionally, so the updated Certificates may start
243+
// being reconciled before the cached CertificateRequests have been updated
244+
// and there will be a mis-match if the Certificate has the default
245+
// group/kind set but the CertificateRequest does not.
246+
if req.Spec.IssuerRef.Name != spec.IssuerRef.Name ||
247+
!issuerKindsEqual(req.Spec.IssuerRef.Kind, spec.IssuerRef.Kind) ||
248+
!issuerGroupsEqual(req.Spec.IssuerRef.Group, spec.IssuerRef.Group) {
228249
violations = append(violations, "spec.issuerRef")
229250
}
230251

@@ -233,6 +254,41 @@ func RequestMatchesSpec(req *cmapi.CertificateRequest, spec cmapi.CertificateSpe
233254
return violations, nil
234255
}
235256

257+
// These defaults are also applied at runtime by the cert-manager
258+
// CertificateRequest controller.
259+
const (
260+
// defaultIssuerKind is the default value for an IssuerRef's kind field
261+
// if it is not specified.
262+
defaultIssuerKind = cmapi.IssuerKind
263+
// defaultIssuerGroup is the default value for an IssuerRef's group field
264+
// if it is not specified.
265+
defaultIssuerGroup = certmanager.GroupName
266+
)
267+
268+
// issuerKindsEqual returns true if the two issuer reference kinds are equal,
269+
// taking into account the defaulting of the kind to "Issuer".
270+
func issuerKindsEqual(l, r string) bool {
271+
if l == "" {
272+
l = defaultIssuerKind
273+
}
274+
if r == "" {
275+
r = defaultIssuerKind
276+
}
277+
return l == r
278+
}
279+
280+
// issuerGroupsEqual returns true if the two issuer reference groups are equal,
281+
// taking into account defaulting of the group to "cert-manager.io".
282+
func issuerGroupsEqual(l, r string) bool {
283+
if l == "" {
284+
l = defaultIssuerGroup
285+
}
286+
if r == "" {
287+
r = defaultIssuerGroup
288+
}
289+
return l == r
290+
}
291+
236292
func matchOtherNames(extension []pkix.Extension, specOtherNames []cmapi.OtherName) (bool, error) {
237293
x509SANExtension, err := extractSANExtension(extension)
238294
if err != nil {

‎pkg/util/pki/match_test.go‎

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,12 @@ import (
2424
"reflect"
2525
"testing"
2626

27+
"github.com/stretchr/testify/assert"
28+
"github.com/stretchr/testify/require"
2729
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2830

2931
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
32+
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
3033
"github.com/cert-manager/cert-manager/pkg/util/pki"
3134
"github.com/cert-manager/cert-manager/test/unit/gen"
3235
)
@@ -290,6 +293,197 @@ func TestRequestMatchesSpecSubject(t *testing.T) {
290293
}
291294
}
292295

296+
// RequestMatchesSpecIssuerRef tests that RequestMatchesSpec correctly compares
297+
// the IssuerRef in the CertificateRequest and CertificateSpec, regardless of
298+
// any differences which are solely due to the presence or absence of default
299+
// group and kind.
300+
//
301+
// For example, we do not want to re-issue the Certificate if the user
302+
// explicitly adds the default issuer group and kind. Nor do we want to re-issue
303+
// if the user removes the default issuer group and kind.
304+
//
305+
// And we want to avoid re-issuing if a future version of the cert-manager
306+
// CRDs introduces API defaults for issuerRef group and kind. Specifically,
307+
// we want to gracefully handle a situation where the platform admin
308+
// upgrades the CRDs to a version that has defaults, but not the controller.
309+
// In that situation, when the CRDs are upgraded, the controller
310+
// re-establishes its watches and refreshes its caches with updated Certificates
311+
// and CertificateRequests, containing the new API defaults. But this
312+
// doesn't happen transactionally, so the updated Certificates may start
313+
// being reconciled before the cached CertificateRequests have been updated
314+
// and there will be a mis-match if the Certificate has the default
315+
// group/kind set but the CertificateRequest does not.
316+
func TestRequestMatchesSpecIssuerRef(t *testing.T) {
317+
type testCase struct {
318+
crSpec *cmapi.CertificateRequest
319+
certSpec cmapi.CertificateSpec
320+
err string
321+
violations []string
322+
}
323+
324+
tests := map[string]testCase{
325+
"should not report any violation if Certificate issuerRef matches the CertificateRequest's": {
326+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
327+
CommonName: "dummy-common-name",
328+
IssuerRef: cmmeta.ObjectReference{
329+
Name: "test-issuer",
330+
Kind: "Issuer",
331+
Group: "cert-manager.io",
332+
},
333+
}}),
334+
certSpec: cmapi.CertificateSpec{
335+
CommonName: "dummy-common-name",
336+
IssuerRef: cmmeta.ObjectReference{
337+
Name: "test-issuer",
338+
Kind: "Issuer",
339+
Group: "cert-manager.io",
340+
},
341+
},
342+
err: "",
343+
},
344+
"should not report any violation if both Certificate and CertificateRequest issuerRef Kind and Group are empty": {
345+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
346+
CommonName: "dummy-common-name",
347+
IssuerRef: cmmeta.ObjectReference{
348+
Name: "test-issuer",
349+
},
350+
}}),
351+
certSpec: cmapi.CertificateSpec{
352+
CommonName: "dummy-common-name",
353+
IssuerRef: cmmeta.ObjectReference{
354+
Name: "test-issuer",
355+
},
356+
},
357+
err: "",
358+
},
359+
"should not report any violation if Certificate issuerRef Kind and Group are defaulted and CertificateRequest issuerRef Group is empty": {
360+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
361+
CommonName: "dummy-common-name",
362+
IssuerRef: cmmeta.ObjectReference{
363+
Name: "test-issuer",
364+
Kind: "Issuer",
365+
},
366+
}}),
367+
certSpec: cmapi.CertificateSpec{
368+
CommonName: "dummy-common-name",
369+
IssuerRef: cmmeta.ObjectReference{
370+
Name: "test-issuer",
371+
Kind: "Issuer",
372+
Group: "cert-manager.io",
373+
},
374+
},
375+
err: "",
376+
},
377+
"should not report any violation if Certificate issuerRef Kind and Group are defaulted and CertificateRequest issuerRef Kind is empty": {
378+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
379+
CommonName: "dummy-common-name",
380+
IssuerRef: cmmeta.ObjectReference{
381+
Name: "test-issuer",
382+
Group: "cert-manager.io",
383+
},
384+
}}),
385+
certSpec: cmapi.CertificateSpec{
386+
CommonName: "dummy-common-name",
387+
IssuerRef: cmmeta.ObjectReference{
388+
Name: "test-issuer",
389+
Kind: "Issuer",
390+
Group: "cert-manager.io",
391+
},
392+
},
393+
err: "",
394+
},
395+
"should report violation if Certificate issuerRef name mismatches the CertificateRequest's": {
396+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
397+
CommonName: "dummy-common-name",
398+
IssuerRef: cmmeta.ObjectReference{
399+
Name: "test-issuer",
400+
Kind: "Issuer",
401+
Group: "cert-manager.io",
402+
},
403+
}}),
404+
certSpec: cmapi.CertificateSpec{
405+
CommonName: "dummy-common-name",
406+
IssuerRef: cmmeta.ObjectReference{
407+
Name: "different-issuer",
408+
Kind: "Issuer",
409+
Group: "cert-manager.io",
410+
},
411+
},
412+
err: "",
413+
violations: []string{"spec.issuerRef"},
414+
},
415+
"should not report any violation if Certificate issuerRef Kind and Group are defaulted and CertificateRequest's are empty": {
416+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
417+
CommonName: "dummy-common-name",
418+
IssuerRef: cmmeta.ObjectReference{
419+
Name: "test-issuer",
420+
},
421+
}}),
422+
certSpec: cmapi.CertificateSpec{
423+
CommonName: "dummy-common-name",
424+
IssuerRef: cmmeta.ObjectReference{
425+
Name: "test-issuer",
426+
Kind: "Issuer",
427+
Group: "cert-manager.io",
428+
},
429+
},
430+
err: "",
431+
},
432+
"should report violation if Certificate issuerRef Kind mismatches the CertificateRequest's (defaulted vs non-defaulted)": {
433+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
434+
CommonName: "dummy-common-name",
435+
IssuerRef: cmmeta.ObjectReference{
436+
Name: "test-issuer",
437+
Kind: "ClusterIssuer",
438+
Group: "cert-manager.io",
439+
},
440+
}}),
441+
certSpec: cmapi.CertificateSpec{
442+
CommonName: "dummy-common-name",
443+
IssuerRef: cmmeta.ObjectReference{
444+
Name: "test-issuer",
445+
Kind: "Issuer",
446+
Group: "cert-manager.io",
447+
},
448+
},
449+
err: "",
450+
violations: []string{"spec.issuerRef"},
451+
},
452+
"should report violation if Certificate issuerRef Group mismatches the CertificateRequest's (defaulted vs non-defaulted)": {
453+
crSpec: mustBuildCertificateRequest(t, &cmapi.Certificate{Spec: cmapi.CertificateSpec{
454+
CommonName: "dummy-common-name",
455+
IssuerRef: cmmeta.ObjectReference{
456+
Name: "test-issuer",
457+
Kind: "Issuer",
458+
Group: "different-group.io",
459+
},
460+
}}),
461+
certSpec: cmapi.CertificateSpec{
462+
CommonName: "dummy-common-name",
463+
IssuerRef: cmmeta.ObjectReference{
464+
Name: "test-issuer",
465+
Kind: "Issuer",
466+
Group: "cert-manager.io",
467+
},
468+
},
469+
err: "",
470+
violations: []string{"spec.issuerRef"},
471+
},
472+
}
473+
for name, test := range tests {
474+
t.Run(name, func(t *testing.T) {
475+
violations, err := pki.RequestMatchesSpec(test.crSpec, test.certSpec)
476+
if test.err != "" {
477+
assert.EqualError(t, err, test.err)
478+
assert.Empty(t, violations)
479+
return
480+
}
481+
require.NoError(t, err)
482+
assert.Equal(t, test.violations, violations)
483+
})
484+
}
485+
}
486+
293487
func TestFuzzyX509AltNamesMatchSpec(t *testing.T) {
294488
tests := map[string]struct {
295489
x509 *x509.Certificate

0 commit comments

Comments
 (0)