squakez commented on code in PR #6818:
URL: https://github.com/apache/camel-k/pull/6818#discussion_r4026774270
##########
pkg/trait/ingress.go:
##########
@@ -124,11 +132,38 @@ func (t *ingressTrait) Apply(e *Environment) error {
ingress.Spec.IngressClassName = &t.IngressClassName
}
- if len(t.TLSHosts) > 0 && t.TLSSecretName != "" {
+ tlsHosts := t.TLSHosts
+ secretName := t.TLSSecretName
+
+ // The cert-manager path only activates when the user has not manually
provided a
+ // secret name. It may fall back to t.Host so that auto-discovery also
works for the
+ // common single-host case, without changing the pre-existing manual
TLS behavior
+ // (which requires TLSHosts to be set explicitly and never considers
t.Host).
+ if secretName == "" {
Review Comment:
I think we miss a check on the `auto` parameter. If disabled on purpose, we
should not proceed. Ideally the check has to be done in the `Configure`. And in
fact, you can use in general the `Configure` func to compute any configuration
that you will later use in `Apply` by storing private scoped variables.
##########
pkg/trait/ingress.go:
##########
@@ -147,6 +182,85 @@ func (t *ingressTrait) Apply(e *Environment) error {
return nil
}
+// resolveCertManagerIssuer determines which cert-manager Issuer or
ClusterIssuer
+// annotation to apply to the Ingress, if any. It returns an empty issuerName
when
+// no annotation should be applied (cert-manager auto-discovery is disabled,
cert-manager
+// is not installed, or no issuer is found). A forced TLSIssuerName is
verified to exist
+// and returns an error if it does not; auto-discovery degrades to a no-op
instead.
+func (t *ingressTrait) resolveCertManagerIssuer(e *Environment)
(annotationKey, issuerName string, err error) {
+ namespace := e.Integration.Namespace
+
+ if t.TLSIssuerName != "" {
+ installed, err := certmanager.IsInstalled(e.Client)
Review Comment:
Trait should have also a client attached, `t.Client` iirc.
##########
pkg/trait/ingress.go:
##########
@@ -147,6 +182,85 @@ func (t *ingressTrait) Apply(e *Environment) error {
return nil
}
+// resolveCertManagerIssuer determines which cert-manager Issuer or
ClusterIssuer
+// annotation to apply to the Ingress, if any. It returns an empty issuerName
when
+// no annotation should be applied (cert-manager auto-discovery is disabled,
cert-manager
+// is not installed, or no issuer is found). A forced TLSIssuerName is
verified to exist
+// and returns an error if it does not; auto-discovery degrades to a no-op
instead.
+func (t *ingressTrait) resolveCertManagerIssuer(e *Environment)
(annotationKey, issuerName string, err error) {
+ namespace := e.Integration.Namespace
+
+ if t.TLSIssuerName != "" {
+ installed, err := certmanager.IsInstalled(e.Client)
+ if err != nil {
+ return "", "", err
+ }
+ if !installed {
+ return "", "", fmt.Errorf("cert-manager is not
installed but tlsIssuerName %q was set", t.TLSIssuerName)
+ }
+
+ kind := t.TLSIssuerKind
+ if kind == "" {
+ kind = "ClusterIssuer"
+ }
+
+ switch kind {
+ case "Issuer":
+ exists, err := certmanager.GetIssuer(e.Ctx, e.Client,
namespace, t.TLSIssuerName)
+ if err != nil {
+ return "", "", err
+ }
+ if !exists {
+ return "", "", fmt.Errorf("issuer %q not found
in namespace %q", t.TLSIssuerName, namespace)
+ }
+
+ return certmanager.AnnotationIssuer, t.TLSIssuerName,
nil
+ case "ClusterIssuer":
+ exists, err := certmanager.GetClusterIssuer(e.Ctx,
e.Client, t.TLSIssuerName)
+ if err != nil {
+ return "", "", err
+ }
+ if !exists {
+ return "", "", fmt.Errorf("clusterissuer %q not
found", t.TLSIssuerName)
+ }
+
+ return certmanager.AnnotationClusterIssuer,
t.TLSIssuerName, nil
+ default:
+ return "", "", fmt.Errorf("invalid tlsIssuerKind %q:
must be %q or %q", kind, "Issuer", "ClusterIssuer")
+ }
+ }
+
+ if !ptr.Deref(t.TLSCertManagerAuto, false) {
Review Comment:
I see this is here, but IMO it does not make sense to do all the logic
(involving API calls) if this is not set. Better do a guard if at the beginning
or in the `Configure` func.
##########
pkg/util/certmanager/enabled.go:
##########
@@ -0,0 +1,163 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package certmanager
+
+import (
+ "context"
+ "sort"
+
+ k8serrors "k8s.io/apimachinery/pkg/api/errors"
+ "k8s.io/apimachinery/pkg/api/meta"
+ "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+ "k8s.io/apimachinery/pkg/runtime/schema"
+ "k8s.io/client-go/kubernetes"
+ ctrl "sigs.k8s.io/controller-runtime/pkg/client"
+
+ kubernetesutil "github.com/apache/camel-k/v2/pkg/util/kubernetes"
+)
+
+const (
+ // CertManagerAPIGroup is the API group for cert-manager.
+ CertManagerAPIGroup = "cert-manager.io"
+ // CertManagerAPIVersion is the current API version for cert-manager.
+ CertManagerAPIVersion = "v1"
+
+ // AnnotationClusterIssuer is the Ingress annotation to specify a
ClusterIssuer.
+ AnnotationClusterIssuer = "cert-manager.io/cluster-issuer"
+ // AnnotationIssuer is the Ingress annotation to specify a namespaced
Issuer.
+ AnnotationIssuer = "cert-manager.io/issuer"
+)
+
+var (
+ // ClusterIssuerGVK is the GroupVersionKind for cert-manager
ClusterIssuer.
+ ClusterIssuerGVK = schema.GroupVersionKind{
+ Group: CertManagerAPIGroup,
+ Version: CertManagerAPIVersion,
+ Kind: "ClusterIssuer",
+ }
+
+ // IssuerGVK is the GroupVersionKind for cert-manager Issuer.
+ IssuerGVK = schema.GroupVersionKind{
+ Group: CertManagerAPIGroup,
+ Version: CertManagerAPIVersion,
+ Kind: "Issuer",
+ }
+)
+
+func isResourceNotFoundError(err error) bool {
+ if err == nil {
+ return false
+ }
+
+ return k8serrors.IsNotFound(err) || meta.IsNoMatchError(err) ||
kubernetesutil.IsUnknownAPIError(err)
+}
+
+// IsInstalled returns true if connected to a cluster with cert-manager
installed.
+func IsInstalled(c kubernetes.Interface) (bool, error) {
Review Comment:
I think we need to change a little bit this one, also for performance
reason. Ideally we can copy the same logic we have done in the twin Camel
Monitor Operator for resources such as Grafana or Prometheus:
https://github.com/camel-tooling/camel-monitor-operator/blob/03c71a6e3540433915e132980c864b55c7c18bb5/pkg/util/kubernetes/discovery.go#L26
And for performance reasons we should do this check only once and store in
the reconciler and later cascading to the controller and the rest of the logic,
or, at least in the env_platform.go as a variable that is turned on/off only at
bootstrap time. See
https://github.com/camel-tooling/camel-monitor-operator/blob/03c71a6e3540433915e132980c864b55c7c18bb5/pkg/controller/camel_monitor/camel_monitor_controller.go#L82-L83
##########
pkg/util/certmanager/enabled.go:
##########
@@ -0,0 +1,163 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one or more
+contributor license agreements. See the NOTICE file distributed with
+this work for additional information regarding copyright ownership.
+The ASF licenses this file to You under the Apache License, Version 2.0
+(the "License"); you may not use this file except in compliance with
+the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package certmanager
+
+import (
+ "context"
+ "sort"
+
+ k8serrors "k8s.io/apimachinery/pkg/api/errors"
+ "k8s.io/apimachinery/pkg/api/meta"
+ "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+ "k8s.io/apimachinery/pkg/runtime/schema"
+ "k8s.io/client-go/kubernetes"
+ ctrl "sigs.k8s.io/controller-runtime/pkg/client"
+
+ kubernetesutil "github.com/apache/camel-k/v2/pkg/util/kubernetes"
+)
+
+const (
+ // CertManagerAPIGroup is the API group for cert-manager.
+ CertManagerAPIGroup = "cert-manager.io"
+ // CertManagerAPIVersion is the current API version for cert-manager.
+ CertManagerAPIVersion = "v1"
+
+ // AnnotationClusterIssuer is the Ingress annotation to specify a
ClusterIssuer.
+ AnnotationClusterIssuer = "cert-manager.io/cluster-issuer"
+ // AnnotationIssuer is the Ingress annotation to specify a namespaced
Issuer.
+ AnnotationIssuer = "cert-manager.io/issuer"
+)
+
+var (
+ // ClusterIssuerGVK is the GroupVersionKind for cert-manager
ClusterIssuer.
+ ClusterIssuerGVK = schema.GroupVersionKind{
+ Group: CertManagerAPIGroup,
+ Version: CertManagerAPIVersion,
+ Kind: "ClusterIssuer",
+ }
+
+ // IssuerGVK is the GroupVersionKind for cert-manager Issuer.
+ IssuerGVK = schema.GroupVersionKind{
+ Group: CertManagerAPIGroup,
+ Version: CertManagerAPIVersion,
+ Kind: "Issuer",
+ }
+)
+
+func isResourceNotFoundError(err error) bool {
+ if err == nil {
+ return false
+ }
+
+ return k8serrors.IsNotFound(err) || meta.IsNoMatchError(err) ||
kubernetesutil.IsUnknownAPIError(err)
+}
+
+// IsInstalled returns true if connected to a cluster with cert-manager
installed.
+func IsInstalled(c kubernetes.Interface) (bool, error) {
+ _, err :=
c.Discovery().ServerResourcesForGroupVersion(schema.GroupVersion{
+ Group: CertManagerAPIGroup,
+ Version: CertManagerAPIVersion,
+ }.String())
+ if isResourceNotFoundError(err) {
+ return false, nil
+ } else if err != nil {
+ return false, err
+ }
+
+ return true, nil
+}
+
+// ListClusterIssuers returns all ClusterIssuer names available in the cluster.
+func ListClusterIssuers(ctx context.Context, c ctrl.Reader) ([]string, error) {
+ list := &unstructured.UnstructuredList{}
Review Comment:
We should not deal with so many unstructured type. The risk is to have too
many runtime errors. Consider to use any existing API, or duck type an API like
we're doing, for example, for Keda type:
https://github.com/apache/camel-k/blob/main/pkg/apis/duck/keda/v1alpha1/duck_types.go
##########
pkg/trait/ingress_test.go:
##########
@@ -335,6 +339,144 @@ func
TestConfigureTLSWithoutSecretNameIngressTraitWDoesSucceed(t *testing.T) {
assert.Equal(t, "service-name(hostname) -> service-name(http)",
conditions[0].Message)
}
+func TestApplyIngressTraitCertManagerAutoNotInstalledDoesNoop(t *testing.T) {
+ ingressTrait, environment := createNominalIngressTest()
+ ingressTrait.TLSCertManagerAuto = ptr.To(true)
+ environment.Ctx = context.Background()
+ fakeClient, err := internal.NewFakeClient()
+ require.NoError(t, err)
+ environment.Client = fakeClient
+
+ err = ingressTrait.Apply(environment)
+
+ require.NoError(t, err)
+ environment.Resources.Visit(func(resource runtime.Object) {
+ if ingress, ok := resource.(*networkingv1.Ingress); ok {
+ assert.Nil(t, ingress.Spec.TLS)
+ assert.NotContains(t, ingress.Annotations,
certmanager.AnnotationClusterIssuer)
+ assert.NotContains(t, ingress.Annotations,
certmanager.AnnotationIssuer)
+ }
+ })
+}
+
+func TestApplyIngressTraitCertManagerAutoNoIssuerDoesNoop(t *testing.T) {
+ ingressTrait, environment := createNominalIngressTest()
+ ingressTrait.TLSCertManagerAuto = ptr.To(true)
+ environment.Ctx = context.Background()
+ fakeClient, err := internal.NewFakeClient()
+ require.NoError(t, err)
+ fakeClient.(*internal.FakeClient).EnableCertManagerDiscovery()
+ environment.Client = fakeClient
+
+ err = ingressTrait.Apply(environment)
+
+ require.NoError(t, err)
+ environment.Resources.Visit(func(resource runtime.Object) {
+ if ingress, ok := resource.(*networkingv1.Ingress); ok {
+ assert.Nil(t, ingress.Spec.TLS)
+ assert.NotContains(t, ingress.Annotations,
certmanager.AnnotationClusterIssuer)
+ }
+ })
+}
+
+func TestApplyIngressTraitCertManagerAutoClusterIssuerFoundDoesSucceed(t
*testing.T) {
+ ingressTrait, environment := createNominalIngressTest()
+ ingressTrait.TLSCertManagerAuto = ptr.To(true)
+ environment.Ctx = context.Background()
+
+ clusterIssuer := newClusterIssuer("letsencrypt-prod")
+ fakeClient, err := internal.NewFakeClient(clusterIssuer)
+ require.NoError(t, err)
+ fakeClient.(*internal.FakeClient).EnableCertManagerDiscovery()
+ environment.Client = fakeClient
+
+ err = ingressTrait.Apply(environment)
+
+ require.NoError(t, err)
+ environment.Resources.Visit(func(resource runtime.Object) {
+ if ingress, ok := resource.(*networkingv1.Ingress); ok {
+ assert.Equal(t, "letsencrypt-prod",
ingress.Annotations[certmanager.AnnotationClusterIssuer])
+ require.NotNil(t, ingress.Spec.TLS)
+ assert.Equal(t, []string{"hostname"},
ingress.Spec.TLS[0].Hosts)
+ assert.Equal(t, "service-name-tls",
ingress.Spec.TLS[0].SecretName)
+ }
+ })
+}
+
+func TestApplyIngressTraitForcedIssuerExistsDoesSucceed(t *testing.T) {
+ ingressTrait, environment := createNominalIngressTest()
+ ingressTrait.TLSIssuerName = "my-issuer"
+ ingressTrait.TLSIssuerKind = "Issuer"
+ environment.Ctx = context.Background()
+ environment.Integration.Namespace = "namespace"
+
+ issuer := newIssuer("my-issuer", "namespace")
+ fakeClient, err := internal.NewFakeClient(issuer)
+ require.NoError(t, err)
+ fakeClient.(*internal.FakeClient).EnableCertManagerDiscovery()
Review Comment:
Is it needed? cannot we instead have it enabled by default?
##########
pkg/trait/ingress.go:
##########
@@ -147,6 +182,85 @@ func (t *ingressTrait) Apply(e *Environment) error {
return nil
}
+// resolveCertManagerIssuer determines which cert-manager Issuer or
ClusterIssuer
+// annotation to apply to the Ingress, if any. It returns an empty issuerName
when
+// no annotation should be applied (cert-manager auto-discovery is disabled,
cert-manager
+// is not installed, or no issuer is found). A forced TLSIssuerName is
verified to exist
+// and returns an error if it does not; auto-discovery degrades to a no-op
instead.
+func (t *ingressTrait) resolveCertManagerIssuer(e *Environment)
(annotationKey, issuerName string, err error) {
+ namespace := e.Integration.Namespace
+
+ if t.TLSIssuerName != "" {
+ installed, err := certmanager.IsInstalled(e.Client)
+ if err != nil {
+ return "", "", err
+ }
+ if !installed {
+ return "", "", fmt.Errorf("cert-manager is not
installed but tlsIssuerName %q was set", t.TLSIssuerName)
+ }
+
+ kind := t.TLSIssuerKind
+ if kind == "" {
+ kind = "ClusterIssuer"
+ }
+
+ switch kind {
+ case "Issuer":
+ exists, err := certmanager.GetIssuer(e.Ctx, e.Client,
namespace, t.TLSIssuerName)
+ if err != nil {
+ return "", "", err
+ }
+ if !exists {
+ return "", "", fmt.Errorf("issuer %q not found
in namespace %q", t.TLSIssuerName, namespace)
+ }
+
+ return certmanager.AnnotationIssuer, t.TLSIssuerName,
nil
+ case "ClusterIssuer":
+ exists, err := certmanager.GetClusterIssuer(e.Ctx,
e.Client, t.TLSIssuerName)
+ if err != nil {
+ return "", "", err
+ }
+ if !exists {
+ return "", "", fmt.Errorf("clusterissuer %q not
found", t.TLSIssuerName)
+ }
+
+ return certmanager.AnnotationClusterIssuer,
t.TLSIssuerName, nil
+ default:
+ return "", "", fmt.Errorf("invalid tlsIssuerKind %q:
must be %q or %q", kind, "Issuer", "ClusterIssuer")
+ }
+ }
+
+ if !ptr.Deref(t.TLSCertManagerAuto, false) {
+ return "", "", nil
+ }
+
+ installed, err := certmanager.IsInstalled(e.Client)
Review Comment:
I think we have already this value from a previous call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
