CM-706: Revisits istiocsr API for GA release #1837
Conversation
|
@bharath-b-rh: This pull request references CM-706 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 story to target the "4.21.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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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 |
| // +kubebuilder:validation:MinLength:=0 | ||
| // +kubebuilder:validation:MaxLength:=4096 |
There was a problem hiding this comment.
Is there any restriction in terms of the length of the label selector? https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/
There was a problem hiding this comment.
There was a problem hiding this comment.
I referred this linked doc, however still it's not clear to me how we are getting 4096 as max length here.
The validation says:
- Key:
Prefix (253) / Name (63)= 317 - Equality-based operator : 2
- Value: 63
So a key/value pair would be of max length: 382
Per the following section
An upper limit of
4096is fixed allowing
to configure up to 10 namespaces considering the limitations on the label selectors and when equality operators are used.
We are limiting 10 namespaces. So it's coming out as : 382*10+9 (commas) = 3829
There was a problem hiding this comment.
Yeah, the calculations are correct. And we are setting a max limitation as 4096. The calculations are for max limits of each part in the selector, so it could be 10 or more than that, but this field value can only have 4096 chars.
There was a problem hiding this comment.
The calculations are for the maximum limits of each part in the selector
Yeah, in the above calculation I took the maximum limit for all the sections, and it turns out that 10 namespaces can be accommodated within a 3829 length of string. Wondering why we need some more extra space (4096-3829)=267?
Let me know if my understanding is wrong here.
There was a problem hiding this comment.
The upper limit for the field is 4096 characters not the number of selectors. And as suggested up to 10 can be added with the suggested the limit when selectors part are all of allowed max length. The delta shown in your calculation can be more if not all selectors are of max length.
There was a problem hiding this comment.
And as suggested up to 10 can be added with the suggested the limit when selectors part are all of allowed max length.
Maybe I am missing something here, but to my understanding, it seems like even if a user uses maxlength for all 10 selectors, they will still be left with the delta shown above, which feels like extra space that can accommodate 10(MaxLength)+x selectors.
bharath-b-rh
force-pushed
the
cm-706
branch
2 times, most recently
from
6cc8bd6 to
32329dc
Compare
|
|
||
| #### Operator uninstallation or `istiocsrs.operator.openshift.io` instance deletion. | ||
|
|
||
| Operator will remove all the resources created for installing `istio-csr` agent when `spec.cleanupOnDeletion` is enabled. |
There was a problem hiding this comment.
Which CR status provides information about the deletion action?
There was a problem hiding this comment.
Could you please elaborate on what is the expectation. The field is used for determining the action when the CR is marked for deletion, we can't make use of status subresource, but we can emit event if the resources are deleted or not.
| is active and should be able to update the certificate endpoint to `istio-csr` agent endpoint. | ||
| - As an OpenShift user, I want to have an option to dynamically enable monitoring for the `istio-csr` project and | ||
| to use the OpenShift monitoring solution when required. | ||
| - As an OpenShift user, I want to limit istio-csr functionality to specific namespaces for better security and control. |
There was a problem hiding this comment.
Does the following usecase list help to elaborate better?
- As an OpenShift administrator, I want to have an option to deploy
istio-csragent, so that it can be enabled as a day2 operation. - As an OpenShift administrator, I want to be able to configure
istio-csragent, so that only required
features can be enabled. - As an OpenShift administrator, I should be able to uninstall
istio-csragent when not required as a day2 operation without disrupting the cert-manager installation. - As an OpenShift administrator, I should be able to choose to keep secrets and related controller should clean up all resources created for the
istio-csragent deployment. - As an OpenShift security engineer, I want to be able to identify all artefacts created by
istio-csragent, for better auditability. - As an OpensShift SRE, I should be able to get details information as part of different status conditions and messages to identify the reasons of failures and carry out corrective actions successfully.
- As an OpenShift service mesh administrator, I should be able to use
istio-csrendpoint to automate certificate requests via istiod on the pre-installed service mesh clusters. - As an OpenShift SRE, I should be able to collect metrics for
istio-csrfor monitoring. - As an OpenShift administrator, I should be able to configure rbac permissions for configmap resources to be applicable to only select namespaces.
- As an OpenShift security engineer, I want to restrict istio-csr operation to only member namespaces of the Service Mesh, so that tenants outside the mesh cannot request certs.
- As an OpenShift administrator, I want to configure istio cluster id that can be verified against the csr to avoid misconfiguration or infer any default value
Non goals
- As an OpenShift security engineer, I want an automatic deletion of
istio-ca-root-certconfigmap from a selected namespace. - As an OpenShift administrator, I want the namespaces chosen for
istio-ca-root-certconfigmap injection to validated against service mesh configuration to avoid drift in the namespace selection.
There was a problem hiding this comment.
@TrilokGeer Could you please help me understand below user stories
As an OpenShift administrator, I should be able to configure rbac permissions for configmap resources to be applicable to only select namespaces.
IIUC, is this proposal for RBAC hardening, to dynamically update the namespaces in istio-csr specific ClusterRole with those matching the selector configured by user for IstioDataPlaneNamespaceSelector? Does it mean, the operator will need watch on Namespace resource and check whether the labels exist on that particular namespace and update the ClusterRole. And when the label is removed or namespace is deleted, update the ClusterRole accordingly. So this would mean having new controller to support this functionality correct?
As an OpenShift security engineer, I want to restrict istio-csr operation to only member namespaces of the Service Mesh, so that tenants outside the mesh cannot request certs.
I think this is the functionality of the istio because the proxies will send the CertificateRequests to istiod, and istiod will act as a proxy here for certificate requests and further forward it to istio-csr as a gRPC request.
Ref: https://github.com/istio/istio/blob/master/architecture/security/istio-agent.md
| // +kubebuilder:validation:MinLength:=0 | ||
| // +kubebuilder:validation:MaxLength:=4096 |
There was a problem hiding this comment.
And as suggested up to 10 can be added with the suggested the limit when selectors part are all of allowed max length.
Maybe I am missing something here, but to my understanding, it seems like even if a user uses maxlength for all 10 selectors, they will still be left with the delta shown above, which feels like extra space that can accommodate 10(MaxLength)+x selectors.
bharath-b-rh
force-pushed
the
cm-706
branch
from
76e5f64 to
06c1358
Compare
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
bharath-b-rh
force-pushed
the
cm-706
branch
from
e954080 to
73fe110
Compare
Signed-off-by: Bharath B <[email protected]>
bharath-b-rh
force-pushed
the
cm-706
branch
from
73fe110 to
30d9c47
Compare
|
Few of the user stories may not be relevant to be handled as part of the GA, it's sugggested to add them as part of risks/constraints or non-goals and possibly consider for future iterations. |
bharath-b-rh
force-pushed
the
cm-706
branch
from
066ab21 to
a7a7c39
Compare
|
@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. |
|
/label tide/merge-method-squash |
|
/lgtm cc: @TrilokGeer Thanks a lot for the in-call review yesterday. Summarizing the changes for your feedback. Following use cases were moved to Non-Goals:
Risks & Mitigation chapter was updated:
@bharath-b-rh kindly add anything that i missed |
5 participants
PR has following changes:
istiocsrs.operator.openshift.ioAPIImplementation Details/Notes/ConstraintsRisks and MitigationsOperational Aspects of API ExtensionsAlternatives (Not Implemented)