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.

Sign up for GitHub

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

Reconcile all clusters on license change #2145

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Reconcile all clusters on license change #2145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c093bc
Reconcile all clusters on license change
pebrc Nov 21, 2019
a6e761c
Be precise in comments
pebrc Nov 21, 2019
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
13 changes: 3 additions & 10 deletions pkg/controller/license/license_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,16 +115,9 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
if !license.IsEnterpriseLicense(*secret) {
return nil
}
// TODO reconcile license secrets and augment license secret metadata with license UID to minimize parsing
license, err := license.ParseEnterpriseLicense(secret.Data)
if err != nil {
log.Error(err, "ignoring invalid or unparseable license in watch handler")
return nil
}

rs, err := listAffectedLicenses(
k8s.WrapClient(mgr.GetClient()), license.License.UID,
)
// if a license is added/modified we want to update for potentially all clusters managed by this instance
// of ECK which is why we are listing all Elasticsearch clusters here and trigger a reconciliation
rs, err := reconcileRequestsForAllClusters(k8s.WrapClient(mgr.GetClient()))
if err != nil {
// dropping the event(s) at this point
log.Error(err, "failed to list affected clusters in enterprise license watch")
Expand Down
19 changes: 8 additions & 11 deletions pkg/controller/license/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,23 @@
package license

import (
"github.com/elastic/cloud-on-k8s/pkg/controller/common/license"
"github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1"
"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

func listAffectedLicenses(c k8s.Client, licenseName string) ([]reconcile.Request, error) {
var list = corev1.SecretList{}
// list all cluster licenses referencing the given enterprise license
matchLabels := license.NewLicenseByNameSelector(licenseName)
err := c.List(&list, matchLabels)
func reconcileRequestsForAllClusters(c k8s.Client) ([]reconcile.Request, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this name indicates (to me) that something will be reconciled here

Copy link
Collaborator Author

@pebrc pebrc Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's meant to be read as a noun not as a verb :-)

var clusters v1beta1.ElasticsearchList
// list all clusters
err := c.List(&clusters)
if err != nil {
return nil, err
}

// enqueue a reconcile request for each cluster license found leveraging the fact that cluster and license
// share the same name
requests := make([]reconcile.Request, len(list.Items))
for i, cl := range list.Items {
// create a reconcile request for each cluster
requests := make([]reconcile.Request, len(clusters.Items))
for i, cl := range clusters.Items {
requests[i] = reconcile.Request{NamespacedName: types.NamespacedName{
Namespace: cl.Namespace,
Name: cl.Name,
Expand Down
46 changes: 26 additions & 20 deletions pkg/controller/license/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ import (
"reflect"
"testing"

"github.com/elastic/cloud-on-k8s/pkg/controller/common/license"
"github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1"
"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -34,7 +33,6 @@ func Test_listAffectedLicenses(t *testing.T) {
true := true

type args struct {
license string
initialObjects []runtime.Object
}
tests := []struct {
Expand All @@ -47,33 +45,41 @@ func Test_listAffectedLicenses(t *testing.T) {
{
name: "happy path",
args: args{
license: "enterprise-license",
initialObjects: []runtime.Object{
&corev1.Secret{
&v1beta1.Elasticsearch{
ObjectMeta: metav1.ObjectMeta{
Name: "foo-cluster",
Namespace: "default",
SelfLink: "/apis/elasticsearch.k8s.elastic.co/",
Labels: map[string]string{
license.LicenseLabelName: "enterprise-license",
},
},
},
&v1beta1.Elasticsearch{
ObjectMeta: metav1.ObjectMeta{
Name: "bar-cluster",
Namespace: "default",
SelfLink: "/apis/elasticsearch.k8s.elastic.co/",
},
},
},
},
want: []reconcile.Request{{
NamespacedName: types.NamespacedName{
Namespace: "default",
Name: "foo-cluster",
want: []reconcile.Request{
{
NamespacedName: types.NamespacedName{
Namespace: "default",
Name: "foo-cluster",
},
},
}},
{
NamespacedName: types.NamespacedName{
Namespace: "default",
Name: "bar-cluster",
},
}},
wantErr: false,
},
{
name: "list error",
args: args{
license: "bar",
},
name: "list error",
args: args{},
injectedError: errors.New("listing failed"),
wantErr: true,
},
Expand All @@ -85,13 +91,13 @@ func Test_listAffectedLicenses(t *testing.T) {
client = &failingClient{Client: client, Error: tt.injectedError}
}

got, err := listAffectedLicenses(client, tt.args.license)
got, err := reconcileRequestsForAllClusters(client)
if (err != nil) != tt.wantErr {
t.Errorf("listAffectedLicenses() error = %v, wantErr %v", err, tt.wantErr)
t.Errorf("reconcileRequestsForAllClusters() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("listAffectedLicenses() = %v, want %v", got, tt.want)
t.Errorf("reconcileRequestsForAllClusters() = %v, want %v", got, tt.want)
}
})
}
Expand Down
18 changes: 6 additions & 12 deletions test/e2e/es/license_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@ func TestEnterpriseLicenseSingle(t *testing.T) {
esBuilder := elasticsearch.NewBuilder("test-es-license-provisioning").
WithESMasterDataNodes(1, elasticsearch.DefaultResources)

mutatedEsBuilder := esBuilder.
WithNoESTopology().
WithESMasterDataNodes(2, elasticsearch.DefaultResources)

licenseTestContext := elasticsearch.NewLicenseTestContext(k, esBuilder.Elasticsearch)

test.StepList{}.
Expand All @@ -44,14 +40,12 @@ func TestEnterpriseLicenseSingle(t *testing.T) {
WithSteps(test.StepList{
licenseTestContext.CheckElasticsearchLicense(license.ElasticsearchLicenseTypeBasic),
licenseTestContext.CreateEnterpriseLicenseSecret(licenseBytes),
// enterprise license can contain all kinds of cluster licenses so we are a bit lenient here and expect either gold or platinum
licenseTestContext.CheckElasticsearchLicense(
license.ElasticsearchLicenseTypeGold,
license.ElasticsearchLicenseTypePlatinum,
),
}).
// Mutation shortcuts the license provisioning check...
WithSteps(mutatedEsBuilder.MutationTestSteps(k)).
// enterprise license can contain all kinds of cluster licenses so we are a bit lenient here and expect either gold or platinum
WithStep(licenseTestContext.CheckElasticsearchLicense(
license.ElasticsearchLicenseTypeGold,
license.ElasticsearchLicenseTypePlatinum,
)).
WithSteps(mutatedEsBuilder.DeletionTestSteps(k)).
WithSteps(esBuilder.DeletionTestSteps(k)).
RunSequential(t)
}