-
Notifications
You must be signed in to change notification settings - Fork 459
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
CM-235: Support for istio-csr through integration with cert-manager-operator #1628
base: master
Are you sure you want to change the base?
Conversation
@bharath-b-rh: This pull request references CM-235 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
/cc @TrilokGeer |
8f3aa72
to
bca36dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we require to install istio-csr
in the same namespace as istiod
, we might always try to find istio-ca
secret and mount ca.crt
in /var/run/secrets/istio-csr/
to avoid "Trust on first use" (read this comment for more context). WDYT?
// Namespace where the istio control-plane is running. | ||
// Same namespace will be used for deploying istio-csr agent. | ||
// +kubebuilder:default:="istio-system" | ||
// +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="namespace is immutable once set" | ||
// +optional | ||
Namespace string `json:"namespace,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not intuitive in my opinion. The namespace of IstioCSR
resource should decide where istio-csr controller will be installed. Additionally, if you assume that istio-csr
must be deployed in the same namespace as istiod
, then we wouldn't need this field at all.
// Namespace where the istio control-plane is running. | |
// Same namespace will be used for deploying istio-csr agent. | |
// +kubebuilder:default:="istio-system" | |
// +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="namespace is immutable once set" | |
// +optional | |
Namespace string `json:"namespace,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But Namespace
is for installing the operand istio-csr
, the controller is managing. And thought not assume the istiod
namespace, made it as a config param. Please let me know WDYT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that I understand it correctly. If I create IstioCSR
resource, will it result in deploying a controller that will manage istio-csr
agent? And the controller will be created in IstioCSR.metadata.namespace
and the agent in IstioCSR.spec.controllerConfig.namespace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the field name ControllerConfig
is misleading, I will update it.
the controller will be created in IstioCSR.metadata.namespace and the agent in IstioCSR.spec.controllerConfig.namespace
istio-csr manager is part of cert-manager-operator
which will watch for istiocsr
custom resource and install istio-csr
agent in IstioCSR.spec.controllerConfig.namespace
. In fact all the configs in controllerConfig
is for setting up the agent. I will update the name and comments appropriately.
And I think it would be better to add a CEL validation for restricting the istiocsr
custom resource in cert-manager-operator namespace. Please let me know WDYT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If istio-csr agent must run in the same namespace as istiod, I would remove controllerConfig.namespace
and rely only on the metadata.namespace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the expectation is to create the new CR in operator namespace and namespace where istiod will be installed need not exist at the time, operator will create it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace where istiod will be installed need not exist at the time
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is based on this doc that, istio-csr can be and must be installed before istio. Please correct me if understanding is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jewertow Is there a maximum tenancy limitation? For cases of mis-managed tenancies, cert-manager operators watching these deployments will be overwhelmed over time leading to performance issues.
kind: ClusterRole | ||
apiVersion: rbac.authorization.k8s.io/v1 | ||
metadata: | ||
name: cert-manager-istio-csr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need a suffix for cluster role names to support multi-tenancy in the future.
Co-authored-by: Jacek Ewertowski <jacek.ewertowski1@gmail.com>
@jewertow Thank you for reviewing the PR!! |
Thank you sharing! I agree, we can mount the secret with CA certificate while starting the istio-csr. |
|
||
### User Stories | ||
|
||
- As an OpenShift user, I want to have an option to dynamically enable `istio-csr`, so that it can be used only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user story is required for installation, un-installation and upgrade usecases.
## Summary | ||
|
||
This enhancement describes the proposal to extend cert-manager-operator to deploy and manage the `istio-csr` | ||
solution. `OpenShift Service Mesh` facilitates the security of both intra-cluster and external service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An elaboration about the istio-csr is required. Would istio-csr solution
mean a controller or set of controllers?
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
/remove-lifecycle stale |
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
@openshift-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/reopen |
/remove-lifecycle rotten |
@bharath-b-rh: This pull request references CM-235 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the spike to target the "4.17.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@bharath-b-rh: Reopened this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@bharath-b-rh: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
||
`istio-csr` controller will make use of static manifest templates for creating below resources to | ||
successfully deploy `istio-csr` agent. | ||
* `certificate.cert-manager.io` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The list has resources created by cert-manager operator as part for cert-manager installation. Are there any differences that needs to be considered for istio-csr?
create the same for enabling the deployment. `istio-csr` should be deployed in the same namespace as that of | ||
the `OpenShift Service Mesh` control plane. | ||
|
||
When `istiocsr` custom resource is created but `issuerref` is not provided operator will create a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you take a relook at this paragraph? cert-manager Certficiate either uses a cluster scoped issuer (ClusterIssuer) or namespace scoped issuer (referred as only Issuer). There is missing context about the Issuer being created using ClusterIssuer.
functionality of `OpenShift Service Mesh` also deployed in the same namespace. | ||
- issuerRef: | ||
IssuerRef is the reference to the certificate authority used for signing the certificates requested by | ||
`OpenShift Service Mesh`. And allowing to change this config will create issues to renewing the existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elaborate the issues related to trust distribution in a separate paragraph.
An OpenShift user could update any configurations in the spec section of the API, except for below | ||
configurations which are immutable and once set cannot be modified. | ||
- namespace: | ||
Namespace where the `istio-csr` agent will be deployed, will also be used for creating other resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are the fields designed to be immutable?
Configurations made available in the spec of `istio-csr` custom resource are passed as process arguments and | ||
updating these configurations would cause new rollout to happen. | ||
|
||
When an OpenShift user deletes istio-csr CR object `istio-csr` agent deployment will be deleted and all the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the remediation in case of deletion failure?
|
||
### Workflow Description | ||
|
||
- An OpenShift user enables `istio-csr` by creating the new custom resource made available for configuring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add a sequence diagram to elaborate on the execution workflow
// Namespace where the istio control-plane is running. | ||
// Same namespace will be used for deploying istio-csr agent. | ||
// +kubebuilder:default:="istio-system" | ||
// +kubebuilder:validation:XValidation:rule="oldSelf == '' || self == oldSelf",message="namespace is immutable once set" | ||
// +optional | ||
Namespace string `json:"namespace,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jewertow Is there a maximum tenancy limitation? For cases of mis-managed tenancies, cert-manager operators watching these deployments will be overwhelmed over time leading to performance issues.
|
||
## Version Skew Strategy | ||
|
||
Istio-csr will be supported for OpenShift Service Mesh Operator 2.4+, Istio v1.10+ and cert-manager v1.3+. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cert-manager operator version needs to be added to the list.
} | ||
|
||
// IstioCSRStatus is the most recently observed status of the IstioCSR. | ||
type IstioCSRStatus struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A re-look at status fields of operator and controller will be needed as per new context.
This enhancement describes the proposal to extend cert-manager-operator to deploy and manage the
istio-csr
solution.OpenShift Service Mesh
facilitates the security of both intra-cluster and external service communications through mTLS. And istio-csr acts as an agent to obtain and renew certificates that facilitate mTLS from thecert-manager
.