Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix webhook feature gate #6093

Merged
merged 4 commits into from May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -72,7 +72,7 @@ spec:
- --secure-port={{ .Values.webhook.securePort }}
{{- end }}
{{- if .Values.featureGates }}
- --feature-gates={{ .Values.featureGates }}
- --feature-gates={{ .Values.webhook.featureGates }}
{{- end }}
{{- $tlsConfig := default $config.tlsConfig "" }}
{{ if or (not $config.tlsConfig) (and (not $tlsConfig.dynamic) (not $tlsConfig.filesystem) ) -}}
Expand Down
6 changes: 5 additions & 1 deletion deploy/charts/cert-manager/values.yaml
Expand Up @@ -70,7 +70,7 @@ podDisruptionBudget:
# or a percentage value (e.g. 25%)

# Comma separated list of feature gates that should be enabled on the
# controller pod & webhook pod.
# controller pod.
featureGates: ""
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the type of this field to a map? That is an improvement over the current string & it will notify our users that something has to be changed.

Copy link
Member

Choose a reason for hiding this comment

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

We can build the string by using {{ join "," ... }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it will notify our users that something has to be changed

what do you mean by this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it'd be preferable that it was the same as the same one for controller which is already string and probably shouldn't be changed


# The maximum number of challenges that can be scheduled as 'processing' at once
Expand Down Expand Up @@ -341,6 +341,10 @@ webhook:
# Path to a file containing a WebhookConfiguration object used to configure the webhook
# - --config=<path-to-config-file>

# Comma separated list of feature gates that should be enabled on the
# webhok pod.
inteon marked this conversation as resolved.
Show resolved Hide resolved
featureGates: ""

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: it is not strictly required to create this new field in this PR. I chose to do so as we're potentially breaking users and it feels nicer to give them an option to reconfigure using a field like this rather than messing around with webhook.extraArgs

resources: {}
# requests:
# cpu: 10m
Expand Down
4 changes: 4 additions & 0 deletions internal/cainjector/feature/features.go
Expand Up @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// feature contains cainjector feature gate setup code. Do not import this
// package into any code that's shared with other components to prevent
// overwriting other component's featue gates, see i.e
// https://github.com/cert-manager/cert-manager/issues/6011
package feature

import (
Expand Down
4 changes: 4 additions & 0 deletions internal/controller/feature/features.go
Expand Up @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// feature contains controller's feature gate setup functionality. Do not import
// this package into any code that's shared with other components to prevent
// overwriting other component's featue gates, see i.e
// https://github.com/cert-manager/cert-manager/issues/6011
package feature

import (
Expand Down
4 changes: 4 additions & 0 deletions internal/webhook/feature/features.go
Expand Up @@ -14,6 +14,10 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

// feature contains webhook's feature gate setup functionality. Do not import
// this package into any code that's shared with other components to prevent
// overwriting other component's featue gates, see i.e
// https://github.com/cert-manager/cert-manager/issues/6011
package feature

import (
Expand Down
2 changes: 1 addition & 1 deletion make/e2e-setup.mk
Expand Up @@ -270,7 +270,7 @@ e2e-setup-certmanager: $(BINDIR)/cert-manager.tgz $(foreach binaryname,controlle
--set installCRDs=true \
--set featureGates="$(feature_gates_controller)" \
--set "extraArgs={--kube-api-qps=9000,--kube-api-burst=9000,--concurrent-workers=200}" \
--set "webhook.extraArgs={--feature-gates=$(feature_gates_webhook)}" \
--set webhook.featureGates="$(feature_gates_webhook)" \
--set "cainjector.extraArgs={--feature-gates=$(feature_gates_cainjector)}" \
--set "dns01RecursiveNameservers=$(SERVICE_IP_PREFIX).16:53" \
--set "dns01RecursiveNameserversOnly=true" \
Expand Down
Expand Up @@ -349,7 +349,8 @@ func (c *controller) deleteRequestsNotMatchingSpec(ctx context.Context, crt *cma

func (c *controller) createNewCertificateRequest(ctx context.Context, crt *cmapi.Certificate, pk crypto.Signer, nextRevision int, nextPrivateKeySecretName string) error {
log := logf.FromContext(ctx)
x509CSR, err := pki.GenerateCSR(crt)

x509CSR, err := pki.GenerateCSR(crt, pki.WithUseLiteralSubject(utilfeature.DefaultMutableFeatureGate.Enabled(feature.LiteralCertificateSubject)))
if err != nil {
log.Error(err, "Failed to generate CSR - will not retry")
return nil
Expand Down
59 changes: 34 additions & 25 deletions pkg/util/pki/csr.go
Expand Up @@ -30,10 +30,8 @@ import (
"net/url"
"strings"

"github.com/cert-manager/cert-manager/internal/controller/feature"
apiutil "github.com/cert-manager/cert-manager/pkg/api/util"
v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
)

func IPAddressesForCertificate(crt *v1.Certificate) []net.IP {
Expand Down Expand Up @@ -165,6 +163,7 @@ func BuildCertManagerKeyUsages(ku x509.KeyUsage, eku []x509.ExtKeyUsage) []v1.Ke

type generateCSROptions struct {
EncodeBasicConstraintsInRequest bool
UseLiteralSubject bool
}

type GenerateCSROption func(*generateCSROptions)
Expand All @@ -178,21 +177,36 @@ func WithEncodeBasicConstraintsInRequest(encode bool) GenerateCSROption {
}
}

func WithUseLiteralSubject(useLiteralSubject bool) GenerateCSROption {
return func(o *generateCSROptions) {
o.UseLiteralSubject = useLiteralSubject
}
}

// GenerateCSR will generate a new *x509.CertificateRequest template to be used
// by issuers that utilise CSRs to obtain Certificates.
// The CSR will not be signed, and should be passed to either EncodeCSR or
// to the x509.CreateCertificateRequest function.
func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.CertificateRequest, error) {
opts := &generateCSROptions{
EncodeBasicConstraintsInRequest: false,
UseLiteralSubject: false,
}
for _, opt := range optFuncs {
opt(opts)
}

commonName, err := extractCommonName(crt.Spec)
if err != nil {
return nil, err
var (
commonName = crt.Spec.CommonName
err error
)

if opts.UseLiteralSubject {
commonName, err = extractCommonNameFromLiteralSubject(crt.Spec)
if err != nil {
return nil, err
}

}

iPAddresses := IPAddressesForCertificate(crt)
Expand Down Expand Up @@ -250,7 +264,7 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert
ExtraExtensions: extraExtensions,
}

if isLiteralCertificateSubjectEnabled() && len(crt.Spec.LiteralSubject) > 0 {
if opts.UseLiteralSubject && len(crt.Spec.LiteralSubject) > 0 {
inteon marked this conversation as resolved.
Show resolved Hide resolved
rawSubject, err := ParseSubjectStringToRawDERBytes(crt.Spec.LiteralSubject)
if err != nil {
return nil, err
Expand Down Expand Up @@ -449,30 +463,25 @@ func SignatureAlgorithm(crt *v1.Certificate) (x509.PublicKeyAlgorithm, x509.Sign
return pubKeyAlgo, sigAlgo, nil
}

func extractCommonName(spec v1.CertificateSpec) (string, error) {
var commonName = spec.CommonName
if isLiteralCertificateSubjectEnabled() && len(spec.LiteralSubject) > 0 {
commonName = ""
sequence, err := UnmarshalSubjectStringToRDNSequence(spec.LiteralSubject)
if err != nil {
return "", err
}
func extractCommonNameFromLiteralSubject(spec v1.CertificateSpec) (string, error) {
if spec.LiteralSubject == "" {
return spec.CommonName, nil
}
commonName := ""
sequence, err := UnmarshalSubjectStringToRDNSequence(spec.LiteralSubject)
if err != nil {
return "", err
}

for _, rdns := range sequence {
for _, atv := range rdns {
if atv.Type.Equal(OIDConstants.CommonName) {
if str, ok := atv.Value.(string); ok {
commonName = str
}
for _, rdns := range sequence {
for _, atv := range rdns {
if atv.Type.Equal(OIDConstants.CommonName) {
if str, ok := atv.Value.(string); ok {
commonName = str
}
}
}
}

return commonName, nil

}

func isLiteralCertificateSubjectEnabled() bool {
return utilfeature.DefaultFeatureGate.Enabled(feature.LiteralCertificateSubject)
}
5 changes: 1 addition & 4 deletions pkg/util/pki/csr_test.go
Expand Up @@ -29,12 +29,9 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
featuregatetesting "k8s.io/component-base/featuregate/testing"

"github.com/cert-manager/cert-manager/internal/controller/feature"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
"github.com/cert-manager/cert-manager/pkg/util"
utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
)

func buildCertificate(cn string, dnsNames ...string) *cmapi.Certificate {
Expand Down Expand Up @@ -614,10 +611,10 @@ func TestGenerateCSR(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, feature.LiteralCertificateSubject, tt.literalCertificateSubjectFeatureEnabled)()
got, err := GenerateCSR(
tt.crt,
WithEncodeBasicConstraintsInRequest(tt.basicConstraintsFeatureEnabled),
WithUseLiteralSubject(tt.literalCertificateSubjectFeatureEnabled),
)
if (err != nil) != tt.wantErr {
t.Errorf("GenerateCSR() error = %v, wantErr %v", err, tt.wantErr)
Expand Down