-
Notifications
You must be signed in to change notification settings - Fork 1.8k
OSDOCS-15110 incorporated edits #95399
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
Conversation
454aa8f to
7b227f8
Compare
snarayan-redhat
left a comment
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.
Left one suggestion. Rest look good.
| .Procedure | ||
|
|
||
| . Uninstall the operand objects by running each of the following commands: | ||
| . Delete the `ZeroTrustWorkloadIdentityManager`` cluster by running the following command: |
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.
| . Delete the `ZeroTrustWorkloadIdentityManager`` cluster by running the following command: | |
| . Delete the `ZeroTrustWorkloadIdentityManager` cluster by running the following command: |
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.
Done
| .Procedure | ||
|
|
||
| . Uninstall the operand objects by running each of the following commands: | ||
| . Delete the `ZeroTrustWorkloadIdentityManager`` cluster by running the following command: |
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 can have a parent step Uninstall the operand objects... and then have each of these as sub steps.
same applies to all instances below
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.
Done
| $ oc delete ZeroTrustWorkloadIdentityManager cluster | ||
| ---- | ||
|
|
||
| . Delete the `SpireOIDCDiscoveryProvider`` cluster by running the following command: |
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.
| . Delete the `SpireOIDCDiscoveryProvider`` cluster by running the following command: | |
| . Delete the `SpireOIDCDiscoveryProvider` cluster by running the following command: |
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.
Done
| ---- | ||
|
|
||
| . Delete the Custom Resource Definitions (CRDs) by running each of the following commands: | ||
| . Delete the SPIRE server Custom Resource Definitions (CRD) by running the following command: |
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.
| . Delete the SPIRE server Custom Resource Definitions (CRD) by running the following command: | |
| . Delete the SPIRE server custom resource definitions (CRD) by running the following command: |
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.
Done
f5372fe to
89c5933
Compare
|
/merge-review-needed |
| = Configuring metrics collection for SPIRE server by using a Service Monitor | ||
|
|
||
| The SPIRE Server operand exposes metrics by default on port `9402` at the `/metrics` endpoint. You can configure metrics collection for the SPIRE Server by creating a `ServiceMonitor` custom resource (CR) that enables Prometheus Operator to collect custom metrics. | ||
| The SPIRE server operand exposes metrics by default on port `9402` at the `/metrics` endpoint. You can configure metrics collection for the SPIRE server by creating a `ServiceMonitor` custom resource (CR) that enables Prometheus Operator to collect custom metrics. |
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.
@snarayan-redhat Any specific reason to use server instead of Server? I think we should keep the consistency - use "server, agent" or "Server, Agent", both capitalized or both not.
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.
@wgabor0427 I believe you concluded based on some discussion with an engineer, right?
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.
@lunarwhite When I looked in the file, it was already lowercase, unless Shuba fixed 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.
@wgabor0427 I think @lunarwhite is suggesting that both agent and server be kept similar. Currently it's Agent and server. Let's keep both lowercase if it wasn't a guideline-driven decision.
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.
Sorry for the frequent changes in requirements. I think it's more appropriate to keep it capitalized Agent Server. This is because "SPIRE Server/Agent" often appears as a proper noun, just like it's used in the upstream documentation https://spiffe.io/docs/latest/spire-about/spire-concepts/
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.
Done
| $ oc delete clusterrolebinding -l=app.kubernetes.io/managed-by=zero-trust-workload-identity-manager | ||
| ---- | ||
|
|
||
| .. Delete the cluster-wide cluster role by running the following command: |
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.
| .. Delete the cluster-wide cluster role by running the following command: | |
| .. Delete the cluster role by running the following command: |
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.
Fixed
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.
Done
| ---- | ||
|
|
||
| . Delete the cluster-wide role-based access control (RBAC) by running each of the following commands: | ||
| .. Delete the cluster-wide role binding by running the following command: |
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.
| .. Delete the cluster-wide role binding by running the following command: | |
| .. Delete the cluster role binding by running the following command: |
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.
Fixed
bf16470 to
046e2ee
Compare
|
@lunarwhite @snarayan-redhat I've changed all instances of server and agent to lowercase. It can be peer-reviewed again |
|
@wgabor0427 Could you please address my legacy comment first before moving forward
That's what https://issues.redhat.com/browse/OCPBUGS-57776 is reporting, the fix has been patched to 4.19, 4,20, main branches, but not 4.18 yet. Without fixing it first, this PR's auto cherrypick would still fail on 4.18 branch. |
|
@lunarwhite Since https://issues.redhat.com/browse/OCPBUGS-57776 has been closed, can we move forward with this. |
lunarwhite
left a comment
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.
Thanks for your work. Left some suggestions
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 think it may not be very user-friendly to have each command in separated blocks. Because for this operator, the uninstallation commands would be quite lengthy. If we keep them separated, users need to do a lot more copy-paste.
We already grouped these commands by resources type, so I think current middle ground is acceptable, for both UX and documentation conventions. What do you think?
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.
@lunarwhite I'm not sure what to do about this. Let me check around and see if there is something similar somewhere
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 can only have one command per block so we cannot group the commands.
| = Configuring metrics collection for SPIRE server by using a Service Monitor | ||
|
|
||
| The SPIRE Server operand exposes metrics by default on port `9402` at the `/metrics` endpoint. You can configure metrics collection for the SPIRE Server by creating a `ServiceMonitor` custom resource (CR) that enables Prometheus Operator to collect custom metrics. | ||
| The SPIRE server operand exposes metrics by default on port `9402` at the `/metrics` endpoint. You can configure metrics collection for the SPIRE server by creating a `ServiceMonitor` custom resource (CR) that enables Prometheus Operator to collect custom metrics. |
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.
Sorry for the frequent changes in requirements. I think it's more appropriate to keep it capitalized Agent Server. This is because "SPIRE Server/Agent" often appears as a proper noun, just like it's used in the upstream documentation https://spiffe.io/docs/latest/spire-about/spire-concepts/
| == Zero Trust Workload Identity Manager components and features | ||
|
|
||
| // SPIFFE SPIRE components | ||
| include::modules/zero-trust-manager-about-components.adoc[leveloffset=+1] | ||
|
|
||
| //SPIRE features | ||
| include::modules/zero-trust-manager-about-features.adoc[leveloffset=+1] | ||
|
|
||
| == Zero Trust Workload Identity Manager Workflow |
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.
| == Zero Trust Workload Identity Manager components and features | |
| // SPIFFE SPIRE components | |
| include::modules/zero-trust-manager-about-components.adoc[leveloffset=+1] | |
| //SPIRE features | |
| include::modules/zero-trust-manager-about-features.adoc[leveloffset=+1] | |
| == Zero Trust Workload Identity Manager Workflow | |
| // SPIFFE SPIRE components | |
| include::modules/zero-trust-manager-about-components.adoc[leveloffset=+1] | |
| //SPIRE features | |
| include::modules/zero-trust-manager-about-features.adoc[leveloffset=+1] |
It seems that we don't need "==" headline in this file
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 need to have "==" in the title or I get an error that I cannot have level 0 sections
2ebd88e to
35db145
Compare
lunarwhite
left a comment
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.
Thanks for your patience. Left minor suggestions otherwise I think it's in good shape now!
|
|
||
|
|
||
| A SPIRE server is responsible for managing and issuing SPIFFE identities within a trust domain. It stores registration entries (selectors that determine under what conditions a SPIFFE ID should be issued) and signing keys. The SPIRE server works in conjunction with the SPIRE agent to perform node attestion via node plugins. For more information, see link:https://spiffe.io/docs/latest/spire-about/spire-concepts/#all-about-the-server[About the SPIRE server]. | ||
| A SPIRE Server is responsible for managing and issuing SPIFFE identities within a trust domain. It stores registration entries (selectors that determine under what conditions a SPIFFE ID should be issued) and signing keys. The SPIRE Server works in conjunction with the SPIRE Agent to perform node attestion via node plugins. For more information, see link:https://spiffe.io/docs/latest/spire-about/spire-concepts/#all-about-the-server[About the SPIRE server]. |
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 SPIRE Server is responsible for managing and issuing SPIFFE identities within a trust domain. It stores registration entries (selectors that determine under what conditions a SPIFFE ID should be issued) and signing keys. The SPIRE Server works in conjunction with the SPIRE Agent to perform node attestion via node plugins. For more information, see link:https://spiffe.io/docs/latest/spire-about/spire-concepts/#all-about-the-server[About the SPIRE server]. | |
| A SPIRE Server is responsible for managing and issuing SPIFFE identities within a trust domain. It stores registration entries (selectors that determine under what conditions a SPIFFE ID should be issued) and signing keys. The SPIRE Server works in conjunction with the SPIRE Agent to perform node attestion via node plugins. For more information, see link:https://spiffe.io/docs/latest/spire-about/spire-concepts/#all-about-the-server[About the SPIRE Server]. |
| [id="spire-controller-manager_{context}"] | ||
| == SPIRE Controller Manager | ||
|
|
||
| The SPIRE Controller Manager uses custom resource definitions (CRDs) to facilitate the registration of workloads. To facilitate workload registration, the SPIRE Controller Manager registers controllers against pods and CRDs. When changes are detected on these resources, a workload reconciliation process is triggered. This process determines which SPIRE entries should exist based on the existing pods and CRDs. The reconciliation process creates, updates, and deletes entries on the SPIRE server as appropriate. |
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 SPIRE Controller Manager uses custom resource definitions (CRDs) to facilitate the registration of workloads. To facilitate workload registration, the SPIRE Controller Manager registers controllers against pods and CRDs. When changes are detected on these resources, a workload reconciliation process is triggered. This process determines which SPIRE entries should exist based on the existing pods and CRDs. The reconciliation process creates, updates, and deletes entries on the SPIRE server as appropriate. | |
| The SPIRE Controller Manager uses custom resource definitions (CRDs) to facilitate the registration of workloads. To facilitate workload registration, the SPIRE Controller Manager registers controllers against pods and CRDs. When changes are detected on these resources, a workload reconciliation process is triggered. This process determines which SPIRE entries should exist based on the existing pods and CRDs. The reconciliation process creates, updates, and deletes entries on the SPIRE Server as appropriate. |
| //Attestation | ||
| include::modules/zero-trust-manager-about-attestation.adoc[leveloffset=+1] | ||
|
|
||
| == Zero Trust Workload Identity Manager components and features |
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.
| == Zero Trust Workload Identity Manager components and features |
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 need to have "==" in the title or I get an error that I cannot have level 0 sections
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.
@wgabor0427 these two comments are to remove the headings; do you want to update before merge?
| //SPIRE features | ||
| include::modules/zero-trust-manager-about-features.adoc[leveloffset=+1] | ||
|
|
||
| == Zero Trust Workload Identity Manager workflow |
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.
| == Zero Trust Workload Identity Manager workflow |
| .Procedure | ||
|
|
||
| . Uninstall the operand objects by running each of the following commands: | ||
| . Uninstall the operands by running each of the following commands: |
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 line of change is still helpful. Apart from that, could we keep them in this file as-is for now? As I said https://github.com/openshift/openshift-docs/pull/95399/files#r2214848484, maybe separate each single command would not be a user-friendly idea.
cc @anirudhAgniRedhat @ldhananj in case you have any suggestions
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 can only have one command per step. We cannot combine commands.
|
Hi @wgabor0427, any updates on this PR? I think we could proceed with merge once my last group of comments are addressed. This can go as part of 0.2.0 release |
09b6582 to
bb1d2da
Compare
|
/lgtm |
|
/label peer-review-needed |
|
@lunarwhite: The label(s) 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. |
|
/label merge-review-needed |
|
/label merge-review-needed |
1 similar comment
|
/label merge-review-needed |
bb1d2da to
c43ef63
Compare
|
New changes are detected. LGTM label has been removed. |
|
@wgabor0427: 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. |
|
/cherrypick enterprise-4.20 |
|
/cherrypick enterprise-4.19 |
|
/cherrypick enterprise-4.18 |
|
@ShaunaDiaz: new pull request created: #98501 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. |
|
@ShaunaDiaz: new pull request created: #98502 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. |
|
@ShaunaDiaz: #95399 failed to apply on top of branch "enterprise-4.18": 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. |
Version(s):
4.18+
Issue:
https://issues.redhat.com/browse/OSDOCS-15110
Link to docs preview:
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-configuration.html
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-features.html
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-install.html
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-monitoring.html
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-overview.html
https://95399--ocpdocs-pr.netlify.app/openshift-enterprise/latest/security/zero_trust_workload_identity_manager/zero-trust-manager-uninstall.html
QE review:
Additional information: