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

Install Kibana per tenant #3348

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

asincu
Copy link
Contributor

@asincu asincu commented May 6, 2024

Description

Kibana will be installed per tenant using ECK operator. Both Kibana and ECK will be deployed by external elastic search controller. ECK operator will be configured to watch in all namespaces. In a tenant namespace we will create a Kibana CR that contains an additional container "challenger". Challenger connects to external elastic via mTLS, while Kibana per tenant connects to Elastic using locahost and no TLS. An kibana admin user will be created in external elasticsearch and referenced via SecureSettings field from the Kibana spec. This secret will contain only elasticsearch.password that in mounted by kibana keystore container at startup. This secret will be created by users controller.

Dashboards and ES Proxy will need to access kibana per tenant.

This PR also adds the ability to install or not install kibana per tenant.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@asincu asincu marked this pull request as ready for review May 15, 2024 00:54
@asincu asincu requested a review from a team as a code owner May 15, 2024 00:54
}
})

It("should support an external kibana endpoint", func() {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this should still be a valid test. We'll still need to "support an external kibana endpoint".

@@ -60,7 +60,7 @@ type KibanaPodSpec struct {
type KibanaContainer struct {
// Name is an enum which identifies the Kibana Deployment container by name.
// Supported values are: kibana
// +kubebuilder:validation:Enum=kibana
// +kubebuilder:validation:Enum=kibana;challenger
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have validation that challenger is not allowed in non-multi-tenent clusters.

pkg/controller/manager/manager_controller.go Outdated Show resolved Hide resolved
@@ -351,6 +386,20 @@ func (r *UserController) createUserLogin(ctx context.Context, elasticEndpoint st
return nil
}

func (r *UserController) createUser(ctx context.Context, elasticEndpoint string, user *utils.User, reqLogger logr.Logger) error {
Copy link
Member

Choose a reason for hiding this comment

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

I was looking to see if we needed both createUser and createUserLogin and I think the answer is yes but it looks like we could have createUserLogin call createUser, that should allow removing some duplicate code.

if err = r.client.Update(ctx, &t); err != nil {
logger.Error(err, "Failed to remove user cleanup finalizer from tenant")
}
if deletedUsers > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is maintaining the same behavior as before but it seems like we should only remove the finalizer if all 3 users have been removed. I think if removal of any of them failed then the finalizer should remain. WDYT?

@@ -290,6 +290,10 @@ func (h *NetworkPolicyHelper) ComplianceReporterSourceEntityRule() v3.EntityRule
return CreateSourceEntityRule(h.namespace("tigera-compliance"), "compliance-reporter")
}

func (h *NetworkPolicyHelper) KibanaEntityRule() v3.EntityRule {
Copy link
Member

Choose a reason for hiding this comment

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

I think this function is only used for Multi-tenant kibana and in that case doesn't this rule need to use the tenant namespace?

@asincu asincu added the hold merge Do not merge label Jun 3, 2024
@danudey danudey removed this from the v1.35.0 milestone Aug 2, 2024
@danudey danudey added this to the v1.35.1 milestone Aug 2, 2024
@danudey danudey modified the milestones: v1.35.1, v1.35.2 Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants