Bug: Fix Certificate's AdditionalOutputFormat in admission within Webhook #4814

JoshVanL · 2022-02-01T17:29:42Z

This PR fixes a bug where the Certificate's additionalOutputFormats' field was only ever being validated by the webhook at admission time if the privateKey` on the Certificate field was also set. The webhook was also incorrectly using the controllers feature set.

Webhook component providing API validation, mutation and conversion functionality for cert-manager (canary) ()
...
      --feature-gates mapStringBool                  A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
                AdditionalCertificateOutputFormats=true|false (ALPHA - default=false)
                AllAlpha=true|false (ALPHA - default=false)
                AllBeta=true|false (BETA - default=false)
                ExperimentalCertificateSigningRequestControllers=true|false (ALPHA - default=false)
                ExperimentalGatewayAPISupport=true|false (ALPHA - default=false)
                ValidateCAA=true|false (ALPHA - default=false)

->

$ ./webhook --help
Webhook component providing API validation, mutation and conversion functionality for cert-manager (canary) ()
...
      --feature-gates mapStringBool                  A set of key=value pairs that describe feature gates for alpha/experimental features. Options are:
                AdditionalCertificateOutputFormats=true|false (ALPHA - default=false)
                AllAlpha=true|false (ALPHA - default=false)
                AllBeta=true|false (BETA - default=false)
...

This PR should be backported, and a patch released published.

I'll leave the cherry-pick command for another maintainer.

/kind bug
/priority critical-urgent
/milestone v1.8

ACTION REQUIRED: The field `additionalOutputFormats`, which is available as an alpha feature on Certificate resources, is now correctly validated. Previously, it would only get validated when the `privateKey` field was set on the Certificate. If you are using the `additionalOutputFormats` field, you will want to add the feature gate `AdditionalCertificateOutputFormats` to both the webhook and the controller. Previously, you only needed to set `AdditionalCertificateOutputFormats` on the controller. If the feature gate is missing on either the controller or the webhook, you won't be able to use the `additionalOutputFormat` field.

@JoshVanL

@JoshVanL owner of feature in controller. Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

feature set Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

webhook set. Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

munnerz · 2022-02-01T17:36:46Z

internal/apis/certmanager/validation/certificate.go

+
+	// Ensure the set of output formats is unique, keyed on "Type".
+	aofSet := sets.NewString()
+	for _, val := range crt.AdditionalOutputFormats {


May just be me, but does this read easier if we switch it around to be:

func validateAdditionalOutputFormats(crt *internalcmapi.CertificateSpec, fldPath *field.Path) field.ErrorList { if utilfeature.DefaultFeatureGate.Enabled(feature.AdditionalCertificateOutputFormats) { var el field.ErrorList // .. perform new validation code .. // return el } else if len(crt.AdditionalOutputFormats) > 0 { // .. return forbidden .. // return el } }

It took me a little while to realise that we'd not validate the Type field if the feature gate is disabled. Perhaps one instance where an else in Go is useful? (or maybe I just need to read more carefully 😄

This was actually intentional. I wanted to have the feature gate check to be the "first" thing in the function- both so it is clear that check doesn't get lost or mangled in other logic, and when/if the feature is graduated we only need to delete that one code block above.

jetstack-bot · 2022-02-01T17:37:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL, munnerz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoshVanL,munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

munnerz · 2022-02-01T17:38:29Z

This also needs cherrypicking to v1.7 right?

munnerz · 2022-02-01T17:38:37Z

/lgtm

munnerz · 2022-02-01T17:39:09Z

/hold feel free to unhold when you're ready

JoshVanL · 2022-02-01T17:51:00Z

I'll leave the cherry-pick command for another maintainer.

This also needs cherrypicking to v1.7 right?

@munnerz Indeed, I wasn't sure what the current process was for cherry picking, but vaguely remember that it was better for someone other than the author to do the command (?). Not sure why.. will do the command myself anyways.

/cherry-pick v1.7
/hold cancel

Note to reader: this AdditionalOutputFormats feature did not exist in v1.6, so doesn't need to be backported there.

jetstack-bot · 2022-02-01T17:51:01Z

@JoshVanL: once the present PR merges, I will cherry-pick it on top of v1.7 in a new PR and assign it to you.

In response to this:

I'll leave the cherry-pick command for another maintainer.

This also needs cherrypicking to v1.7 right?

@munnerz Indeed, I wasn't sure what the current process was for cherry picking, but vaguely remember that it was better for someone other than the author to do the command (?). Not sure why.. will do the command myself anyways.

/cherry-pick v1.7
/hold cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jetstack-bot · 2022-02-01T18:07:30Z

@JoshVanL: cannot checkout v1.7: error checking out v1.7: exit status 1. output: error: pathspec 'v1.7' did not match any file(s) known to git

In response to this:

I'll leave the cherry-pick command for another maintainer.

This also needs cherrypicking to v1.7 right?

@munnerz Indeed, I wasn't sure what the current process was for cherry picking, but vaguely remember that it was better for someone other than the author to do the command (?). Not sure why.. will do the command myself anyways.

/cherry-pick v1.7
/hold cancel

Note to reader: this AdditionalOutputFormats feature did not exist in v1.6, so doesn't need to be backported there.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

JoshVanL · 2022-02-01T18:08:09Z

/cherry-pick release-1.7

jetstack-bot · 2022-02-01T18:08:45Z

@JoshVanL: new pull request created: #4816

In response to this:

/cherry-pick release-1.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

maelvls · 2022-04-05T16:30:21Z

This is a breaking change, right? I feel like we should notify people that are using additionalOutputFormats. Should I add ACTION REQUIRED? @JoshVanL

JoshVanL · 2022-04-05T17:02:43Z

This is a breaking change, right? I feel like we should notify people that are using additionalOutputFormats. Should I add ACTION REQUIRED? @JoshVanL

@maelvls it is not a breaking change in the API, it is a bug fix. No action is required from end users.

JoshVanL added 3 commits February 1, 2022 17:02

Add AdditionalCertificateOutputFormats feature to webhook set. Make

1cf0688

@JoshVanL owner of feature in controller. Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Fix AdditationOutputFormat validation, and adds unit tests. Use correct

8b219a4

feature set Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

Update AdditionalOutputFormats comment to reflect addition of feature to

35fba36

webhook set. Signed-off-by: joshvanl <vleeuwenjoshua@gmail.com>

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Feb 1, 2022

jetstack-bot added this to the v1.8 milestone Feb 1, 2022

munnerz reviewed Feb 1, 2022

View reviewed changes

munnerz approved these changes Feb 1, 2022

View reviewed changes

jetstack-bot assigned munnerz Feb 1, 2022

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2022

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2022

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2022

jetstack-bot merged commit ee8ec69 into cert-manager:master Feb 1, 2022

jetstack-bot mentioned this pull request Feb 1, 2022

[release-1.7] Bug: Fix Certificate's AdditionalOutputFormat in admission within Webhook #4816

Merged

jetstack-bot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 5, 2022

Bug: Fix Certificate's AdditionalOutputFormat in admission within Webhook #4814

Bug: Fix Certificate's AdditionalOutputFormat in admission within Webhook #4814

Uh oh!

Conversation

JoshVanL commented Feb 1, 2022 • edited by maelvls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

munnerz Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

JoshVanL Feb 1, 2022

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Feb 1, 2022

Uh oh!

munnerz commented Feb 1, 2022

Uh oh!

munnerz commented Feb 1, 2022

Uh oh!

munnerz commented Feb 1, 2022

Uh oh!

JoshVanL commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetstack-bot commented Feb 1, 2022

Uh oh!

jetstack-bot commented Feb 1, 2022

Uh oh!

JoshVanL commented Feb 1, 2022

Uh oh!

jetstack-bot commented Feb 1, 2022

Uh oh!

maelvls commented Apr 5, 2022

Uh oh!

JoshVanL commented Apr 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JoshVanL commented Feb 1, 2022 •

edited by maelvls

Loading

JoshVanL commented Feb 1, 2022 •

edited

Loading