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

Mount cert and add permissions to IDS controller to access managed cluster #1265

Merged
merged 1 commit into from Apr 27, 2021

Conversation

Suraiya-Hameed
Copy link
Contributor

@Suraiya-Hameed Suraiya-Hameed commented Apr 21, 2021

Description

As part of replacing Elastic watchers, IDS controller in management will watch for GlobalAlert in managed cluster and handle them. This PR is to mount tls-manager certificate and adds role needed to access managed cluster from management cluster and also disable GlobalAlert handling in managed cluster's IDS controller.

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.

Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

if managementClusterConnection == nil {
if esLicenseType, err = utils.GetElasticLicenseType(ctx, r.client, reqLogger); err != nil {
r.status.SetDegraded("Failed to get Elasticsearch license", err.Error())
return reconcile.Result{}, err
}

managerInternalTLSSecret, err = utils.ValidateCertPair(r.client,
Copy link
Member

Choose a reason for hiding this comment

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

Depending on which pull is merged first, you may wanna change the key to "", such that it is not checked.
#1260

},
},
}
if !c.managedCluster {
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, do we still need this role at all in managed clusters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, IDS controller in managed cluster need permission to access licensekeys, globalthreatfeeds and globalnetworksets.
I'll remove the permissions for globalalerts, they are not needed in managed cluster anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested by removing globalalerts permission for IDS controller in managed cluster, if we do that then IDS in management doesn't have access to globalalerts in managed cluster. So we need to retain those permissions.

@Suraiya-Hameed Suraiya-Hameed force-pushed the es-watcher-replacement branch 3 times, most recently from acd2d93 to 92b2f5d Compare April 23, 2021 21:48
Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

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

LGTM

@tmjd tmjd modified the milestones: v1.17.0, v1.18.0 Apr 26, 2021
@tmjd tmjd added enterprise Feature applies to enterprise only kind/enhancement New feature or request and removed docs-pr-required labels Apr 26, 2021
in management cluster to access GlobalAlert in managed cluster.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants