Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ This operator runs the following OpenShift controllers:
* **serving cert signer:**
* Issues a signed serving certificate/key pair to services annotated with 'service.beta.openshift.io/serving-cert-secret-name' via a secret. [See the current OKD documentation for usage.](https://docs.okd.io/latest/security/certificates/service-serving-certificate.html)

* **configmap cabundle injector:**
* Watches for configmaps annotated with 'service.beta.openshift.io/inject-cabundle=true' and adds or updates a data item (key "service-ca.crt") containing the PEM-encoded CA signing bundle. Consumers of the configmap can then trust service-ca.crt in their TLS client configuration, allowing connections to services that utilize service-serving certificates.
* Note: Explicitly referencing the "service-ca.crt" key in a volumeMount will prevent a pod from starting until the configMap has been injected with the CA bundle (https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#restrictions). This behavior helps ensure that pods start with the CA bundle data available.
* **configmap/secret cabundle injector:**
* Watches for configmaps and secrets annotated with 'service.beta.openshift.io/inject-cabundle=true' and adds or updates a data item (key "service-ca.crt") containing the PEM-encoded CA signing bundle. Consumers of the configmap/secret can then trust service-ca.crt in their TLS client configuration, allowing connections to services that utilize service-serving certificates.
* Note: Explicitly referencing the "service-ca.crt" key in a volumeMount will prevent a pod from starting until the configMap/secret has been injected with the CA bundle (https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#restrictions). This behavior helps ensure that pods start with the CA bundle data available.

```
$ oc create configmap foobar --from-literal=key1=foo
Expand Down
81 changes: 81 additions & 0 deletions pkg/controller/cabundleinjector/secret.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
package cabundleinjector

import (
"context"
"fmt"

corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kcoreclient "k8s.io/client-go/kubernetes/typed/core/v1"
listers "k8s.io/client-go/listers/core/v1"
"k8s.io/klog/v2"

apiannotations "github.com/openshift/api/annotations"
"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/service-ca-operator/pkg/controller/api"
)

type secretCABundleInjector struct {
client kcoreclient.SecretsGetter
lister listers.SecretLister
caBundle string

filterFn func(secret *corev1.Secret) bool
}

func newSecretInjectorConfig(config *caBundleInjectorConfig) controllerConfig {
informer := config.kubeInformers.Core().V1().Secrets()

syncer := &secretCABundleInjector{
client: config.kubeClient.CoreV1(),
lister: informer.Lister(),
caBundle: string(config.caBundle),
}

return controllerConfig{
name: "SecretCABundleInjector",
sync: syncer.Sync,
informer: informer.Informer(),
annotationsChecker: annotationsChecker(
api.InjectCABundleAnnotationName,
),
namespaced: true,
}
}

func (bi *secretCABundleInjector) Sync(ctx context.Context, syncCtx factory.SyncContext) error {
namespace, name := namespacedObjectFromQueueKey(syncCtx.QueueKey())

secret, err := bi.lister.Secrets(namespace).Get(name)
if apierrors.IsNotFound(err) {
return nil
} else if err != nil {
return err
}

if bi.filterFn != nil && !bi.filterFn(secret) {
return nil
}

// skip updating when the CA bundle is already there
if data, ok := secret.Data[api.InjectionDataKey]; ok &&
string(data) == bi.caBundle && len(secret.Data) == 1 {

return nil
}

klog.Infof("updating secret %s/%s with the service signing CA bundle", secret.Namespace, secret.Name)

// make a copy to avoid mutating cache state
secretCopy := secret.DeepCopy()
secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
// set the owning-component unless someone else has claimed it.
if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent
secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates")
}

_, err = bi.client.Secrets(secretCopy.Namespace).Update(ctx, secretCopy, metav1.UpdateOptions{})
Comment on lines +61 to +79
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix nil Annotations panic and reconsider overwriting all Secret data

Two things to flag here:

  1. Potential panic on nil annotations (must fix):
    secretCopy.Annotations may be nil for a freshly-created Secret. Writing to a nil map will panic:
if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
    secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent
    secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates")
}

Initialize the map before writing:

-	secretCopy := secret.DeepCopy()
-	secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
-	// set the owning-component unless someone else has claimed it.
-	if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
+	secretCopy := secret.DeepCopy()
+	secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
+	// ensure annotations map is initialized before writing to it
+	if secretCopy.Annotations == nil {
+		secretCopy.Annotations = map[string]string{}
+	}
+	// set the owning-component unless someone else has claimed it.
+	if len(secretCopy.Annotations[apiannotations.OpenShiftComponent]) == 0 {
 		secretCopy.Annotations[apiannotations.OpenShiftComponent] = api.OwningJiraComponent
 		secretCopy.Annotations[apiannotations.OpenShiftDescription] = fmt.Sprintf("Secret is added/updated with a data item containing the CA signing bundle that can be used to verify service-serving certificates")
 	}
  1. Secret Data is fully overwritten (design/semantics):
    secretCopy.Data = map[string][]byte{...} drops all existing keys and, together with the len(secret.Data) == 1 skip condition, forces injected Secrets to contain only service-ca.crt. That’s consistent with the new e2e tests (which assert len(Data)==1 and that extra keys get “stomped”), but it is stricter than the README wording (“adds or updates a data item”) and prevents co-locating other material in the same Secret.

If you want parity with a more typical “only manage InjectionDataKey” pattern (and to better match the README text), consider instead:

-	secretCopy := secret.DeepCopy()
-	secretCopy.Data = map[string][]byte{api.InjectionDataKey: []byte(bi.caBundle)}
+	secretCopy := secret.DeepCopy()
+	if secretCopy.Data == nil {
+		secretCopy.Data = map[string][]byte{}
+	}
+	secretCopy.Data[api.InjectionDataKey] = []byte(bi.caBundle)

and relaxing the len(secret.Data) == 1 checks in Sync/tests to only care that InjectionDataKey matches the operator’s bundle.

🤖 Prompt for AI Agents
In pkg/controller/cabundleinjector/secret.go around lines 61 to 79, initialize
secretCopy.Annotations if nil before writing to it to avoid a nil-map panic, and
stop overwriting the entire Secret.Data: instead of assigning a new map, set or
update only the api.InjectionDataKey entry on secretCopy.Data (ensuring
secretCopy.Data is non-nil first) and adjust the earlier skip condition so it
checks only that the InjectionDataKey value matches bi.caBundle rather than
relying on len(secret.Data)==1.

return err
}
1 change: 1 addition & 0 deletions pkg/controller/cabundleinjector/starter.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ func StartCABundleInjector(ctx context.Context, controllerContext *controllercmd
configConstructors := []configBuilderFunc{
newAPIServiceInjectorConfig,
newConfigMapInjectorConfig,
newSecretInjectorConfig,
newCRDInjectorConfig,
newMutatingWebhookInjectorConfig,
newValidatingWebhookInjectorConfig,
Expand Down
121 changes: 121 additions & 0 deletions test/e2e/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,18 @@ func createAnnotatedCABundleInjectionConfigMap(client *kubernetes.Clientset, con
return err
}

func createAnnotatedCABundleInjectionSecret(client *kubernetes.Clientset, secretName, namespace string) error {
obj := &v1.Secret{
TypeMeta: metav1.TypeMeta{},
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
},
}
setInjectionAnnotation(&obj.ObjectMeta)
_, err := client.CoreV1().Secrets(namespace).Create(context.TODO(), obj, metav1.CreateOptions{})
return err
}

func pollForServiceServingSecret(client *kubernetes.Clientset, secretName, namespace string) error {
return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
_, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
Expand All @@ -236,6 +248,19 @@ func pollForCABundleInjectionConfigMap(client *kubernetes.Clientset, configMapNa
})
}

func pollForCABundleInjectionSecret(client *kubernetes.Clientset, secretName, namespace string) error {
return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
_, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil && errors.IsNotFound(err) {
return false, nil
}
if err != nil {
return false, err
}
return true, nil
})
}

func editServingSecretData(t *testing.T, client *kubernetes.Clientset, secretName, namespace, keyName string) error {
sss, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil {
Expand Down Expand Up @@ -269,6 +294,24 @@ func editConfigMapCABundleInjectionData(t *testing.T, client *kubernetes.Clients
return pollForConfigMapChange(t, client, cmcopy, "foo")
}

func editSecretCABundleInjectionData(t *testing.T, client *kubernetes.Clientset, secretName, namespace string) error {
secret, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil {
return err
}
secretCopy := secret.DeepCopy()
if len(secretCopy.Data) != 1 {
return fmt.Errorf("ca bundle injection secret missing data")
}
secretCopy.Data["foo"] = []byte("blah")
_, err = client.CoreV1().Secrets(namespace).Update(context.TODO(), secretCopy, metav1.UpdateOptions{})
if err != nil {
return err
}

return pollForSecretChange(t, client, secretCopy, "foo")
}

func checkServiceServingCertSecretData(client *kubernetes.Clientset, secretName, namespace string) ([]byte, bool, error) {
sss, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil {
Expand Down Expand Up @@ -312,6 +355,22 @@ func checkConfigMapCABundleInjectionData(client *kubernetes.Clientset, configMap
return nil
}

func checkSecretCABundleInjectionData(client *kubernetes.Clientset, secretName, namespace string) error {
cm, err := client.CoreV1().Secrets(namespace).Get(context.TODO(), secretName, metav1.GetOptions{})
if err != nil {
return err
}
if len(cm.Data) != 1 {
return fmt.Errorf("unexpected ca bundle injection secret data map length: %v", len(cm.Data))
}
ok := true
_, ok = cm.Data[api.InjectionDataKey]
if !ok {
return fmt.Errorf("unexpected ca bundle injection secret data: %v", cm.Data)
}
return nil
}

func pollForConfigMapCAInjection(client *kubernetes.Clientset, configMapName, namespace string) error {
return wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
cm, err := client.CoreV1().ConfigMaps(namespace).Get(context.TODO(), configMapName, metav1.GetOptions{})
Expand Down Expand Up @@ -1436,6 +1495,68 @@ func TestE2E(t *testing.T) {
}
})

// test ca bundle injection secret
t.Run("ca-bundle-injection-secret", func(t *testing.T) {
ns, cleanup, err := createTestNamespace(t, adminClient, "test-"+randSeq(5))
if err != nil {
t.Fatalf("could not create test namespace: %v", err)
}
defer cleanup()

testSecretName := "test-secret-" + randSeq(5)

err = createAnnotatedCABundleInjectionSecret(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error creating annotated secret: %v", err)
}

err = pollForCABundleInjectionSecret(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error fetching ca bundle injection secret: %v", err)
}

err = checkSecretCABundleInjectionData(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error when checking ca bundle injection secret: %v", err)
}
})

// test updated data in ca bundle injection secret will be stomped on
t.Run("ca-bundle-injection-secret-update", func(t *testing.T) {
ns, cleanup, err := createTestNamespace(t, adminClient, "test-"+randSeq(5))
if err != nil {
t.Fatalf("could not create test namespace: %v", err)
}
defer cleanup()

testSecretName := "test-secret-" + randSeq(5)

err = createAnnotatedCABundleInjectionSecret(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error creating annotated secret: %v", err)
}

err = pollForCABundleInjectionSecret(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error fetching ca bundle injection secret: %v", err)
}

err = checkSecretCABundleInjectionData(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error when checking ca bundle injection secret: %v", err)
}

err = editSecretCABundleInjectionData(t, adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error editing ca bundle injection secret: %v", err)
}

err = checkSecretCABundleInjectionData(adminClient, testSecretName, ns.Name)
if err != nil {
t.Fatalf("error when checking ca bundle injection secret: %v", err)
}
})

t.Run("metrics", func(t *testing.T) {
promClient, err := newPrometheusClientForConfig(adminConfig)
if err != nil {
Expand Down