CNTRLPLANE-3731: Add Agentic SDLC context files to cluster-policy-controller#191
CNTRLPLANE-3731: Add Agentic SDLC context files to cluster-policy-controller#191atiratree wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@atiratree: This pull request references CNTRLPLANE-3731 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 "5.0.0" version, but no target version was set. DetailsIn 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. |
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds repository documentation in ChangesRepository documentation set
Estimated code review effort: 2 (Simple) | ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 185-201: The fenced diagram/example blocks in AGENTS.md are
unlabeled, which triggers markdownlint and removes highlighting. Update both
affected fenced blocks near the ClusterPolicyController flow and the later
example section to use an explicit language tag such as text, keeping the
content unchanged. Make sure the fix applies to both identified blocks,
including the one referenced by the ControllerInitializers /
RunClusterPolicyController diagram.
- Around line 245-246: AGENTS.md has stale Go and Kubernetes client version
references that no longer match the module metadata. Update both version
mentions in the guide so they align with the values declared in go.mod and
vendor/modules.txt, changing the Go version reference from 1.25 to 1.23.0 and
the client-go reference from v1.35.2 to v0.32.2. Make the same correction in
both AGENTS.md sections that describe these toolchain/dependency versions.
In `@ARCHITECTURE.md`:
- Around line 297-300: Fix the spelling of the controller name in the Components
list so it matches the Reconciliation naming used elsewhere. Update the
referenced `ClusterQuotaReconcilationController` entry in `ARCHITECTURE.md` to
the correct `ClusterQuotaReconciliationController` spelling, keeping the rest of
the section unchanged.
- Around line 39-441: The fenced examples in ARCHITECTURE.md are missing
language identifiers, which is triggering MD040. Update each fenced block in the
document to use an appropriate tag such as text for diagrams and plain examples,
and any other fitting language where relevant, so the snippets are properly
labeled and the lint warning is cleared. Use the existing fenced sections around
the architecture diagrams and code samples as the targets.
In `@CONTRIBUTING.md`:
- Around line 62-69: Clarify the filename convention in CONTRIBUTING.md by
scoping “All filenames should be lowercase” to source/package filenames, or
explicitly exempting top-level documentation files like the repo-root docs
referenced elsewhere. Update the Directory and File Conventions section so the
rule no longer conflicts with uppercase root docs, and keep the guidance
consistent with the existing notes about Go source files, package directories,
and nested subdirectories.
- Around line 170-179: The verification command text is inconsistent with the
repository guidance and should be aligned in CONTRIBUTING.md. Update the section
that describes PR verification to use the actual supported commands referenced
by AGENTS.md, specifically /verified or /verified by ci, and remove the
unsupported /verified by @{your_github_handle} wording. Keep the surrounding
Cluster Bot guidance intact while making the verification step match the real
merge workflow.
- Around line 128-137: The markdown examples in CONTRIBUTING.md use unlabeled
code fences that trigger MD040. Update the PR title example block in the example
section and the directory tree block referenced in the comment to include a
fence language such as text so the fences are explicitly labeled. Use the
existing example sections around the PR title examples and directory tree
snippet to locate and adjust the fences consistently.
- Around line 223-245: The Go version requirement in CONTRIBUTING.md is
inconsistent with the repository’s actual toolchain; update the Required Tools
and verification text to match the Go version used by go.mod and
Dockerfile.rhel. Keep the wording aligned with the repo’s build/runtime
requirement, and ensure the version note under the prerequisite check reflects
the same Go version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fdbc0af8-6748-42ca-b60d-53e69262e277
📒 Files selected for processing (4)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.md
| ``` | ||
| cmd/cluster-policy-controller/main.go | ||
| │ | ||
| ▼ | ||
| NewClusterPolicyControllerCommand("start") | ||
| │ | ||
| ▼ | ||
| RunClusterPolicyController() | ||
| │ | ||
| ├──▶ Parse OpenShiftControllerManagerConfig | ||
| ├──▶ Wait for API server health (/healthz, up to 5 min) | ||
| ├──▶ Create EnhancedControllerContext | ||
| ├──▶ Iterate ControllerInitializers map | ||
| │ └──▶ For each enabled controller: call InitFunc | ||
| ├──▶ StartInformers() | ||
| └──▶ Block on ctx.Done() | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the bare fenced examples.
These two diagrams/examples are missing a language tag, which trips markdownlint and disables syntax highlighting. Use text (or another explicit label) for both blocks.
Also applies to: 205-247
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 185-185: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 185 - 201, The fenced diagram/example blocks in
AGENTS.md are unlabeled, which triggers markdownlint and removes highlighting.
Update both affected fenced blocks near the ClusterPolicyController flow and the
later example section to use an explicit language tag such as text, keeping the
content unchanged. Make sure the fix applies to both identified blocks,
including the one referenced by the ControllerInitializers /
RunClusterPolicyController diagram.
Source: Linters/SAST tools
| ├── go.mod # Go 1.25, k8s v1.35.2 | ||
| └── OWNERS # control-plane-approvers, ibihim, liouk |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "go.mod toolchain:"
grep -nE '^(go |toolchain )' go.mod || true
echo
echo "client-go references:"
grep -nE 'client-go|k8s.io/client-go' go.mod vendor/modules.txt || true
echo
echo "AGENTS.md version claims:"
rg -n 'Go 1\.25|client-go v1\.35\.2|k8s v1\.35\.2' AGENTS.mdRepository: openshift/cluster-policy-controller
Length of output: 29111
Update the version references in AGENTS.md
AGENTS.md still says Go 1.25 and client-go v1.35.2, but go.mod/vendor/modules.txt declare go 1.23.0 and k8s.io/client-go v0.32.2. Please align the guide in both places (AGENTS.md:245-246 and AGENTS.md:566-567).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 245 - 246, AGENTS.md has stale Go and Kubernetes
client version references that no longer match the module metadata. Update both
version mentions in the guide so they align with the values declared in go.mod
and vendor/modules.txt, changing the Go version reference from 1.25 to 1.23.0
and the client-go reference from v1.35.2 to v0.32.2. Make the same correction in
both AGENTS.md sections that describe these toolchain/dependency versions.
| ### Directory and File Conventions | ||
|
|
||
| - Avoid package sprawl. Find an appropriate subdirectory for new packages. | ||
| - Libraries with no appropriate home belong in new package subdirectories of `pkg/`. | ||
| - Avoid general utility packages. Packages called "util" are suspect. Instead, derive a name that describes your desired function. | ||
| - All filenames should be lowercase. | ||
| - Go source files and directories use underscores, not dashes. | ||
| - Package directories should generally avoid using separators as much as possible. When package names are multiple words, they usually should be in nested subdirectories. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the filename rule.
“All filenames should be lowercase” conflicts with the uppercase repo-root docs you reference later. Narrow this to source/package filenames or explicitly exempt the top-level docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` around lines 62 - 69, Clarify the filename convention in
CONTRIBUTING.md by scoping “All filenames should be lowercase” to source/package
filenames, or explicitly exempting top-level documentation files like the
repo-root docs referenced elsewhere. Update the Directory and File Conventions
section so the rule no longer conflicts with uppercase root docs, and keep the
guidance consistent with the existing notes about Go source files, package
directories, and nested subdirectories.
| **Example PR Title**: | ||
| ``` | ||
| OCPBUGS-12345: fix UID range allocation for namespaces with existing annotations | ||
| ``` | ||
|
|
||
| or | ||
|
|
||
| ``` | ||
| NO-JIRA: fix typo in CONTRIBUTING.md | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the unlabeled fences.
These blocks will keep tripping markdownlint MD040; add a fence language (text is fine here) to the PR title examples and the directory tree block. As per static analysis, the warnings are at lines 129, 135, and 272.
Also applies to: 272-280
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 129-129: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 135-135: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` around lines 128 - 137, The markdown examples in
CONTRIBUTING.md use unlabeled code fences that trigger MD040. Update the PR
title example block in the example section and the directory tree block
referenced in the comment to include a fence language such as text so the fences
are explicitly labeled. Use the existing example sections around the PR title
examples and directory tree snippet to locate and adjust the fences
consistently.
Source: Linters/SAST tools
| As part of merging a PR, there is a requirement to verify that the changes you've made are working as expected using the `/verified` comment command. | ||
|
|
||
| While there are a lot of scenarios where the existing CI/CD checks may be sufficient to verify your changes are working (and can be denoted by commenting `/verified by ci`), | ||
| there may be scenarios where manual verification is required. | ||
|
|
||
| You can use the `Cluster Bot` Slack App to create a cluster from a PR by sending it a message in the format of `launch ${OCP_VERSION},${PR_LINK} ${PLATFORM},${VARIANT}`. | ||
| As an example, `launch 4.22,https://github.com/openshift/cluster-policy-controller/pull/123 aws` would launch an OpenShift 4.22 cluster with the changes made in the PR running on AWS. | ||
| For more information on what `Cluster Bot` can do, you can send it a message saying `help` and it will respond with additional documentation on how it can be used. | ||
|
|
||
| Once you've verified your changes work as expected, you can mark the PR as verified by commenting `/verified by @{your_github_handle}` on the PR. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the documented verification syntax.
/verified by @{your_github_handle} doesn’t match the repo guidance in AGENTS.md, which only mentions /verified or /verified by ci. Please align this section with the actual PR command syntax. Based on learnings, use /verified or /verified by ci when marking a PR verified before merge.
🧰 Tools
🪛 LanguageTool
[style] ~172-~172: Consider using a synonym to be more concise.
Context: ...fied` comment command. While there are a lot of scenarios where the existing CI/CD chec...
(A_LOT_OF)
[uncategorized] ~179-~179: The official name of this software platform is spelled with a capital “H”.
Context: ...n mark the PR as verified by commenting /verified by @{your_github_handle} on the PR. ### Additional Res...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` around lines 170 - 179, The verification command text is
inconsistent with the repository guidance and should be aligned in
CONTRIBUTING.md. Update the section that describes PR verification to use the
actual supported commands referenced by AGENTS.md, specifically /verified or
/verified by ci, and remove the unsupported /verified by @{your_github_handle}
wording. Keep the surrounding Cluster Bot guidance intact while making the
verification step match the real merge workflow.
Source: Learnings
|
I put together this summary and overall review. Feel free to check it out or feed it to an agent: Desired Document StructureThe files follow a clear design: AGENTS.md is the agent entry point, ARCHITECTURE.md holds architecture details, CONTRIBUTING.md is shared across repos, and README.md is kept slim. Files are meant to link to each other rather than duplicate content. CLAUDE.md symlinks to AGENTS.md. This review evaluates each file against that intent and these principles:
File-by-file AssessmentAGENTS.md (653 lines)Good parts:
Problems:
ARCHITECTURE.md (590 lines)Good parts:
Problems:
CONTRIBUTING.md (419 lines)This file is shared across repos, so most findings here are not specific to this PR. Noting them for awareness. Good parts:
Problems (noting these are constrained by the shared-across-repos nature):
Summary of Main Findings
Recommendations
|
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
cf1ec18 to
0f52c56
Compare
|
@tchap thanks for the feedback. I hope it is better now. |
|
Two things:
Overall it's always hard to review this, because it's more of a art. If it works for the agents, it's ok. I also take it as bit of a WIP, especially AGENTS.md, which can be updated regularly based on what works and what doesn't. Personally I find the structure good and pretty complete. I haven't verified all the details about the architecture, though, I am assuming it's correct and I am mostly reviewing the structure. |
|
BTW for some reason your branch is 28 commits behind |
|
I tried some verification with Claude: Factual Accuracy VerificationClaims in all four files were verified against the source code. Most are accurate. The following discrepancies were found: Errors1. Privileged Namespaces Controller targets — ARCHITECTURE.md line 383 Claim: "Target Namespaces: Namespaces matching OpenShift infrastructure patterns ( Actual: The controller uses 2. PSA annotation casing — ARCHITECTURE.md line 364 Claim: Actual: 3. Claim: Lists Actual: The public interface is Misleading4. Service account constant claim — AGENTS.md line 300 Claim: "Every controller uses a dedicated service account name constant in Actual: Five of six do, but |
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
0f52c56 to
e30089e
Compare
openshift-ci
Bot
added
the
needs-rebase
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
e30089e to
8dcb8f2
Compare
openshift-ci
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 300: The service-account guidance in AGENTS.md is too absolute given the
current controller set, since one controller still uses an inline literal
instead of a config.go constant. Reword the rule to make the dedicated service
account constant the preferred pattern, or explicitly mention the exception, and
use the controller/client-builder terminology already present so it’s easy to
align with ClientBuilder.Client() and the config.go constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 761a8945-9654-43fc-88d0-c48a72df7d58
📒 Files selected for processing (5)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mdREADME.md
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- README.md
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
8dcb8f2 to
ef42976
Compare
|
Thanks for the second review. I tried to incorporate the suggestions.
Doh! I mixed up the master x main again. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 9-13: The last reference in the README resource list is pointing
back to README.md instead of the contribution guide, creating a self-loop.
Update the final bullet in the README.md links section so the CONTRIBUTING.md
entry points to the actual contribution guide, matching the surrounding
AGENTS.md and ARCHITECTURE.md links.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e4f1bde4-0d24-43dd-b229-3caad6c10cb6
📒 Files selected for processing (5)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mdREADME.md
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
ef42976 to
8489990
Compare
|
|
||
| ## Common Workflows | ||
|
|
||
| ### Adding a New Controller |
There was a problem hiding this comment.
I feel like much of the following can be guessed from existing code since there are existing controllers. I would be careful with particular code snippets or checklists if it's just copying actual existing code. This AGENTS.md is still pretty long. The following section of what to do and what not to do is nice, but perhaps the code snippets are not that helpful. Just something to consider...
There was a problem hiding this comment.
Sounds good, I removed most of the code snippets.
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
2 times, most recently
from
0cc4bd0 to
ab2ea96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 67-69: The filename policy in AGENTS.md is inconsistent with
existing uppercase repo-root documentation like AGENTS.md itself. Update the
guidance in the “What to Always Do” section to scope the lowercase rule to
source/package filenames or explicitly exempt top-level documentation files, and
make sure the wording is internally consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9da5ccd-8cf6-46d4-b2b5-08b2bbbdd461
📒 Files selected for processing (5)
AGENTS.mdARCHITECTURE.mdCLAUDE.mdCONTRIBUTING.mdREADME.md
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- README.md
|
|
||
| ### What to Always Do | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Scope the filename rule.
This guidance is self-contradictory as written: the repo-root docs in this PR are uppercase, including this file. Narrow it to source/package filenames or explicitly exempt top-level documentation files.
♻️ Proposed fix
- - All filenames should be lowercase.
+ - Source/package filenames should be lowercase.
+ - Top-level documentation files are exempt.As per coding guidelines: All filenames should be lowercase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AGENTS.md` around lines 67 - 69, The filename policy in AGENTS.md is
inconsistent with existing uppercase repo-root documentation like AGENTS.md
itself. Update the guidance in the “What to Always Do” section to scope the
lowercase rule to source/package filenames or explicitly exempt top-level
documentation files, and make sure the wording is internally consistent.
Source: Coding guidelines
|
/lgtm |
| - Synchronize Pod Security Admission labels based on SCC assignments | ||
| - Label privileged system namespaces with PSA privileged level |
There was a problem hiding this comment.
| - Synchronize Pod Security Admission labels based on SCC assignments | |
| - Label privileged system namespaces with PSA privileged level | |
| - If PSA enforcement is turned on, synchronize Pod Security Admission (PSA) labels based on SCC assignments | |
| - If PSA enforcement is turned off, synchronize only warn and audit PSA labels based on SCC assignments |
There was a problem hiding this comment.
Thanks, mentioning the enforcement now.
|
|
||
| - **Don't forget the service account constant** - Every controller should use a dedicated service account name constant in `config.go` for `ClientBuilder.Client()`. Missing this means the controller can't authenticate. | ||
|
|
||
| - **Don't confuse the two PSA label sync controllers** - There are two distinct controllers: `podsecurity-admission-label-syncer` (syncs labels based on SCC/RBAC analysis) and `privileged-namespaces-psa-label-syncer` (just labels system namespaces as privileged). They serve different purposes. |
There was a problem hiding this comment.
Non sense, they are being used based on the feature flag for PSA enforcement.
There was a problem hiding this comment.
I removed some of these design decissions/nuances from this section. It should not be in a Mistakes section.
There was a problem hiding this comment.
Yes, this was my mistake. We enforce privileged for "default", "kube-system" and "kube-public" with a dedicated controller. I mixed it up with the feature flag variant.
|
|
||
| - **Don't confuse the two PSA label sync controllers** - There are two distinct controllers: `podsecurity-admission-label-syncer` (syncs labels based on SCC/RBAC analysis) and `privileged-namespaces-psa-label-syncer` (just labels system namespaces as privileged). They serve different purposes. | ||
|
|
||
| - **Don't modify `nsexemptions/` without understanding the implications** - The namespace exemption list determines which namespaces are never synced by the PSA label syncer. Changes here affect cluster security posture. |
There was a problem hiding this comment.
I would feel most comfortable if an AI wouldn't change this at all. Only reason for an AI to touch this is to cheat on tests.
There was a problem hiding this comment.
+1, changed the wording
|
|
||
| - **Don't use table-driven tests without the `t.Run()` pattern** - All tests should use subtests for better failure isolation: | ||
|
|
||
| - **Don't assume the PSA label syncer controls all namespaces** - It only syncs namespaces where `security.openshift.io/scc.podSecurityLabelSync` is not `"false"` and the namespace is not exempted. System namespaces starting with `openshift-` require explicit opt-in. |
There was a problem hiding this comment.
I would not know why an agent would assume that and why it would need this for development.
| │ │ │ │ ┌──────────────┐ ┌──────────────────────────┐ │ │ │ │ | ||
| │ │ │ │ │ Namespace │ │ Pod Security Admission │ │ │ │ │ | ||
| │ │ │ │ │ SCC │ │ Label Syncer │ │ │ │ │ | ||
| │ │ │ │ │ Allocation │ │ (+ Privileged NS) │ │ │ │ │ |
There was a problem hiding this comment.
changed the diagram to mention all the 6 controllers
| │ | ||
| ▼ | ||
| Apply PSA labels (server-side apply): | ||
| - pod-security.kubernetes.io/enforce: <level> |
There was a problem hiding this comment.
Only if feature flag is enabled as mentioned in "two modes".
|
|
||
| **Field Ownership**: Uses server-side apply with field manager `pod-security-admission-label-synchronization-controller`. Includes logic to migrate ownership from the legacy `cluster-policy-controller` field manager. | ||
|
|
||
| ### 6. Privileged Namespaces PSA Label Syncer |
There was a problem hiding this comment.
I would remove this and settle solely on 5.
There was a problem hiding this comment.
Probably doesn't hurt to have it here since we an overview of every controller, but I can remove it if you think it doesn't give much useful/extra information.
There was a problem hiding this comment.
Yes, this was my mistake. We enforce privileged for "default", "kube-system" and "kube-public".
| ```yaml | ||
| pod-security.kubernetes.io/enforce: privileged | ||
| pod-security.kubernetes.io/audit: privileged | ||
| pod-security.kubernetes.io/warn: privileged |
There was a problem hiding this comment.
I tried on a live cluster, and we do have these labels set.
There was a problem hiding this comment.
Yes, this was my mistake. We enforce privileged for "default", "kube-system" and "kube-public".
| **Rationale**: | ||
| - **Different logic**: System namespaces are always privileged (simple). User namespaces need SCC/RBAC analysis (complex). | ||
| - **Different failure modes**: The privileged controller is critical infrastructure. The syncer is more nuanced and can be wrong without catastrophic impact. | ||
| - **Different opt-in models**: System namespaces are always labeled. User namespaces can opt out. |
There was a problem hiding this comment.
The only reason is the feature flag. One sets all 3 PSA labels, the other doesn't set the enforce label, which therefore falls back to the global variable, which is privileged.
There was a problem hiding this comment.
It was not entirerly correct so I removed this section. I am keeping only the more obvious Design Decisions for the AI.
| **Symptom**: Multiple field managers trying to set the same PSA labels. | ||
|
|
||
| **Recovery**: | ||
| - The controller detects when another manager owns a label and backs off |
There was a problem hiding this comment.
Assuming human intervention <- customer sets a different label by hand
There was a problem hiding this comment.
Ack - changed the wording
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
ab2ea96 to
a2dee27
Compare
|
New changes are detected. LGTM label has been removed. |
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
a2dee27 to
0b8048e
Compare
| Take the most permissive PSA level across all SAs | ||
| │ | ||
| ▼ | ||
| Apply the following PSA labels if OpenShiftPodSecurityAdmission feature is enabled (server-side apply): |
There was a problem hiding this comment.
Sorry, if I wasn't specific enough:
- If OpenShiftPodSecurityAdmission=true we set pod-security.kubernetes.io/enforce, pod-security.kubernetes.io/warn and pod-security.kubernetes.io/audit
- If OpenShiftPodSecurityAdmission=false pod-security.kubernetes.io/warn and pod-security.kubernetes.io/audit. we do not set we set pod-security.kubernetes.io/enforce,
There was a problem hiding this comment.
Thanks, using your wording now.
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
0b8048e to
7971391
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
atiratree
force-pushed
the
CNTRLPLANE-3731
branch
from
7971391 to
ed4cba0
Compare
|
@atiratree: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
Summary by CodeRabbit