Skip to content

Commit

Permalink
Fix for missing pull secrets in auto created namespaces (#2245)
Browse files Browse the repository at this point in the history 
* fix for missing pull secrets in auto created namespaces

* var name updates

* move env config to controller

* update namespace tests as well as move envconfig to controller

* move copysecret to pkg/utils

* update copyright year

* testing for copysecret functionality

* remove comment

* copyright year

* comment

* remove exit function and update comment

* update controller

* remove secretfound check
  • Loading branch information
@bvennam @knative-prow-robot
bvennam authored and knative-prow-robot committed Dec 13, 2019
1 parent 1d3f987 commit e33dba2
Show file tree
Hide file tree
Showing 9 changed files with 349 additions and 63 deletions.
2 changes: 2 additions & 0 deletions config/500-controller.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ spec:
value: knative.dev/eventing/cmd/broker/filter
- name: BROKER_FILTER_SERVICE_ACCOUNT
value: eventing-broker-filter
- name: BROKER_IMAGE_PULL_SECRET_NAME
value:
ports:
- containerPort: 9090
name: metrics
Expand Down
12 changes: 12 additions & 0 deletions pkg/reconciler/namespace/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package namespace
import (
"context"

"github.com/kelseyhightower/envconfig"
"knative.dev/pkg/configmap"
"knative.dev/pkg/controller"
"knative.dev/pkg/tracker"
Expand All @@ -31,6 +32,10 @@ import (
"knative.dev/pkg/client/injection/kube/informers/rbac/v1/rolebinding"
)

type envConfig struct {
BrokerPullSecretName string `envconfig:"BROKER_IMAGE_PULL_SECRET_NAME" required:"false"`
}

const (
// ReconcilerName is the name of the reconciler
ReconcilerName = "Namespace" // TODO: Namespace is not a very good name for this controller.
Expand All @@ -56,6 +61,13 @@ func NewController(
Base: reconciler.NewBase(ctx, controllerAgentName, cmw),
namespaceLister: namespaceInformer.Lister(),
}

var env envConfig
if err := envconfig.Process("", &env); err != nil {
r.Logger.Info("no broker image pull secret name defined")
}
r.brokerPullSecretName = env.BrokerPullSecretName

impl := controller.NewImpl(r, r.Logger, ReconcilerName)
// TODO: filter label selector: on InjectionEnabledLabels()

Expand Down
21 changes: 21 additions & 0 deletions pkg/reconciler/namespace/namespace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const (
brokerCreated = "BrokerCreated"
serviceAccountCreated = "BrokerServiceAccountCreated"
serviceAccountRBACCreated = "BrokerServiceAccountRBACCreated"
secretCopied = "SecretCopied"
)

var (
Expand All @@ -61,6 +62,8 @@ var (
type Reconciler struct {
*reconciler.Base

brokerPullSecretName string

// listers index properties about resources
namespaceLister corev1listers.NamespaceLister
serviceAccountLister corev1listers.ServiceAccountLister
Expand Down Expand Up @@ -141,6 +144,7 @@ func (r *Reconciler) reconcile(ctx context.Context, ns *corev1.Namespace) error
// reconcileServiceAccountAndRoleBinding reconciles the service account and role binding for
// Namespace 'ns'.
func (r *Reconciler) reconcileServiceAccountAndRoleBindings(ctx context.Context, ns *corev1.Namespace, saName, rbName, clusterRoleName, configClusterRoleName string) error {

sa, err := r.reconcileBrokerServiceAccount(ctx, ns, resources.MakeServiceAccount(ns.Name, saName))
if err != nil {
return fmt.Errorf("service account '%s': %v", saName, err)
Expand Down Expand Up @@ -174,6 +178,23 @@ func (r *Reconciler) reconcileServiceAccountAndRoleBindings(ctx context.Context,
return fmt.Errorf("track role binding '%s': %v", rb.Name, err)
}

if sa.Name == resources.IngressServiceAccountName || sa.Name == resources.FilterServiceAccountName {
// check for existence of brokerPullSecret, and skip copy if it already exists
for _, v := range sa.ImagePullSecrets {
if fmt.Sprintf("%s", v) == ("{" + r.brokerPullSecretName + "}") {
return nil
}
}
_, err := utils.CopySecret(r.KubeClientSet.CoreV1(), system.Namespace(), r.brokerPullSecretName, ns.Name, sa.Name)
if err != nil {
r.Recorder.Event(ns, corev1.EventTypeNormal, secretCopied,
fmt.Sprintf("Error copying secret: %s", err))
} else {
r.Recorder.Event(ns, corev1.EventTypeNormal, secretCopied,
fmt.Sprintf("Secret copied into namespace: %s", sa.Name))
}
}

return nil
}

Expand Down
95 changes: 93 additions & 2 deletions pkg/reconciler/namespace/namespace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/scheme"
clientgotesting "k8s.io/client-go/testing"
"knative.dev/eventing/pkg/apis/eventing/v1alpha1"
eventingv1alpha1 "knative.dev/eventing/pkg/apis/eventing/v1alpha1"
"knative.dev/eventing/pkg/reconciler"
Expand All @@ -43,7 +44,8 @@ import (
)

const (
testNS = "test-namespace"
testNS = "test-namespace"
brokerImagePullSecretName = "broker-image-pull-secret"
)

var (
Expand Down Expand Up @@ -80,8 +82,15 @@ func TestAllCases(t *testing.T) {
rbFilterConfigEvent := Eventf(corev1.EventTypeNormal, "BrokerServiceAccountRBACCreated", "RoleBinding 'knative-testing/eventing-broker-filter-test-namespace' created for the Broker")
brokerEvent := Eventf(corev1.EventTypeNormal, "BrokerCreated", "Default eventing.knative.dev Broker created.")
nsEvent := Eventf(corev1.EventTypeNormal, "NamespaceReconciled", "Namespace reconciled: \"test-namespace\"")
secretEventFilter := Eventf(corev1.EventTypeNormal, "SecretCopied", "Secret copied into namespace: eventing-broker-filter")
secretEventIngress := Eventf(corev1.EventTypeNormal, "SecretCopied", "Secret copied into namespace: eventing-broker-ingress")

// Patches
ingressPatch := createPatch(testNS, "eventing-broker-ingress")
filterPatch := createPatch(testNS, "eventing-broker-filter")

// Object
secret := resources.MakeSecret(brokerImagePullSecretName)
broker := resources.MakeBroker(testNS)
saIngress := resources.MakeServiceAccount(testNS, resources.IngressServiceAccountName)
rbIngress := resources.MakeRoleBinding(resources.IngressRoleBindingName, testNS, resources.MakeServiceAccount(testNS, resources.IngressServiceAccountName), resources.IngressClusterRoleName)
Expand Down Expand Up @@ -137,20 +146,29 @@ func TestAllCases(t *testing.T) {
saIngressEvent,
rbIngressEvent,
rbIngressConfigEvent,
secretEventIngress,
saFilterEvent,
rbFilterEvent,
rbFilterConfigEvent,
secretEventFilter,
brokerEvent,
nsEvent,
},
WantCreates: []runtime.Object{
broker,
secret,
saIngress,
rbIngress,
rbIngressConfig,
secret,
saFilter,
rbFilter,
rbFilterConfig,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
}, {
Name: "Namespace enabled, broker exists",
Expand All @@ -167,18 +185,27 @@ func TestAllCases(t *testing.T) {
saIngressEvent,
rbIngressEvent,
rbIngressConfigEvent,
secretEventIngress,
saFilterEvent,
rbFilterEvent,
rbFilterConfigEvent,
secretEventFilter,
nsEvent,
},
WantCreates: []runtime.Object{
secret,
saIngress,
rbIngress,
rbIngressConfig,
secret,
saFilter,
rbFilter,
rbFilterConfig,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
}, {
Name: "Namespace enabled, broker exists with no label",
Expand Down Expand Up @@ -210,19 +237,28 @@ func TestAllCases(t *testing.T) {
WantEvents: []string{
rbIngressEvent,
rbIngressConfigEvent,
secretEventIngress,
saFilterEvent,
rbFilterEvent,
rbFilterConfigEvent,
secretEventFilter,
brokerEvent,
nsEvent,
},
WantCreates: []runtime.Object{
broker,
secret,
rbIngress,
rbIngressConfig,
secret,
saFilter,
rbFilter,
rbFilterConfig,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
}, {
Name: "Namespace enabled, ingress role binding exists",
Expand All @@ -238,18 +274,27 @@ func TestAllCases(t *testing.T) {
WantErr: false,
WantEvents: []string{
saIngressEvent,
secretEventIngress,
saFilterEvent,
rbFilterEvent,
rbFilterConfigEvent,
secretEventFilter,
brokerEvent,
nsEvent,
},
WantCreates: []runtime.Object{
broker,
secret,
saIngress,
secret,
saFilter,
rbFilter,
rbFilterConfig,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
}, {
Name: "Namespace enabled, filter service account exists",
Expand All @@ -266,18 +311,27 @@ func TestAllCases(t *testing.T) {
saIngressEvent,
rbIngressEvent,
rbIngressConfigEvent,
secretEventIngress,
rbFilterEvent,
rbFilterConfigEvent,
secretEventFilter,
brokerEvent,
nsEvent,
},
WantCreates: []runtime.Object{
broker,
secret,
saIngress,
rbIngress,
rbIngressConfig,
secret,
rbFilter,
rbFilterConfig,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
}, {
Name: "Namespace enabled, filter role binding exists",
Expand All @@ -295,30 +349,67 @@ func TestAllCases(t *testing.T) {
saIngressEvent,
rbIngressEvent,
rbIngressConfigEvent,
secretEventIngress,
saFilterEvent,
secretEventFilter,
brokerEvent,
nsEvent,
},
WantCreates: []runtime.Object{
broker,
secret,
saIngress,
rbIngress,
rbIngressConfig,
secret,
saFilter,
secret,
},
WantPatches: []clientgotesting.PatchActionImpl{
ingressPatch,
filterPatch,
},
},
// TODO: we need a existing default un-owned test.
}

logger := logtesting.TestLogger(t)
// used to determine which test we are on
testNum := 0
table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler {
return &Reconciler{

r := &Reconciler{
Base: reconciler.NewBase(ctx, controllerAgentName, cmw),
namespaceLister: listers.GetNamespaceLister(),
brokerLister: listers.GetBrokerLister(),
serviceAccountLister: listers.GetServiceAccountLister(),
roleBindingLister: listers.GetRoleBindingLister(),
tracker: tracker.New(func(types.NamespacedName) {}, 0),
brokerPullSecretName: brokerImagePullSecretName,
}

// only create secret in required tests
createSecretTests := []string{"Namespace enabled", "Namespace enabled, broker exists",
"Namespace enabled, ingress service account exists", "Namespace enabled, ingress role binding exists",
"Namespace enabled, filter service account exists", "Namespace enabled, filter role binding exists"}

for _, theTest := range createSecretTests {
if theTest == table[testNum].Name {
// create the required secret in knative-eventing to be copied into required namespaces
tgtNSSecrets := r.KubeClientSet.CoreV1().Secrets(system.Namespace())
tgtNSSecrets.Create(resources.MakeSecret(brokerImagePullSecretName))
break
}
}
testNum++
return r
}, false, logger))
}

func createPatch(namespace string, name string) clientgotesting.PatchActionImpl {
patch := clientgotesting.PatchActionImpl{}
patch.Namespace = namespace
patch.Name = name
patch.Patch = []byte(`{"imagePullSecrets":[{"name":"` + brokerImagePullSecretName + `"}]}`)
return patch
}
31 changes: 31 additions & 0 deletions pkg/reconciler/namespace/resources/secret.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
Copyright 2019 The Knative Authors
Licensed 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 resources

import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// MakeSecret creates a Secret object.
func MakeSecret(name string) *corev1.Secret {
return &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
}
}
Loading

0 comments on commit e33dba2

Please sign in to comment.