fix: block system namespace creation - #2940
Conversation
theakshaypant
left a comment
There was a problem hiding this comment.
Thanks @krishhlogan for picking this up!
request you to remove the jira references and sign the CLA.
|
done @theakshaypant |
theakshaypant
left a comment
There was a problem hiding this comment.
Thanks @krishhlogan ! As a follow up to earlier request
- Please squash your commits into a single one
- Amend the message on the first commit top remove jira reference
cbd211c to
60eea39
Compare
|
done @theakshaypant |
|
/ok-to-test |
theakshaypant
left a comment
There was a problem hiding this comment.
Overall looks fine, only recommend adding a couple more test cases for completeness.
|
@krishhlogan can you please ensure always squashing your commits into one and rebasing with upstream/main |
3f87590 to
1e1f7da
Compare
|
/ok-to-test |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2940 +/- ##
=======================================
Coverage 80.66% 80.66%
=======================================
Files 164 164
Lines 13910 13910
=======================================
Hits 11221 11221
Misses 1967 1967
Partials 722 722
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@zakisk Sure, will do! Thanks for the heads up |
Prevent tkn pac create repo from creating repositories in system namespaces where the default service account is not auto-created and pipelines would fail. - openshift-pipelines, tektoncd-pipelines: return error - openshift-* prefix: return error (system namespaces) - kube-* prefix: show warning, allow with confirmation Signed-off-by: krishhlogan <kloganat@redhat.com> Assisted-by: Claude Sonnet 4.6 (via Claude Code) fix: use correct tekton system namespace name Replace tektoncd-pipelines with tekton-pipelines which is the actual namespace used by Tekton Pipelines installation. Signed-off-by: krishhlogan <kloganat@redhat.com> fix: warn on tekton- prefixed system namespaces Add warning for tekton-* namespaces (other than tekton-pipelines) similar to kube-* handling. tekton-pipelines remains an error. Signed-off-by: krishhlogan <kloganat@redhat.com>
1e1f7da to
97ceb08
Compare
|
/ok-to-test |
|
/paco review |
Paco Review 🔍This PR adds a checkSystemNamespace helper in the tknpac repository create command to prevent (or warn about) creating Pipelines-as-Code Repository CRs in Kubernetes/OpenShift/Tekton system namespaces, wiring it into both the flag-based and interactive-prompt namespace resolution paths in getOrCreateNamespace. It also adds a substantial set of table-driven tests covering the new hard-error and warning behaviors for each namespace prefix. Review difficulty: 2/5 (Easy) — The change is small and localized to one file plus tests, with a straightforward helper function and two call sites. 1 new inline comment(s) found. Reviewed commit: 97ceb08 |
| } | ||
| if strings.HasPrefix(ns, "openshift-") { | ||
| return fmt.Errorf("cannot create a repository in namespace %s as system namespaces are not supported", ns) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] The system-namespace protection is applied inconsistently: any openshift--prefixed namespace is hard-blocked, but kube--prefixed namespaces (e.g. kube-system, kube-public, kube-node-lease — arguably the most sensitive Kubernetes system namespaces) and non-exact tekton--prefixed namespaces only produce a warning and still allow repository creation to proceed. If the intent of this change is to prevent accidental repository creation in system namespaces, kube- should likely be hard-blocked the same way openshift- is, rather than just warned about.
There was a problem hiding this comment.
🟡 Changes recommended
The exact reserved openshift namespace remains allowed and must be rejected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds safeguards for repository creation in system namespaces.
Changes:
- Validates explicit and interactively selected namespaces.
- Rejects pipeline and
openshift-*namespaces; warns forkube-*andtekton-*. - Adds unit tests for both selection paths.
File summaries
| File | Review |
|---|---|
pkg/cmd/tknpac/create/repository.go |
Moderate (1 vote): Exact openshift namespace is not rejected. Nit (2 votes): Update the validation comment to accurately distinguish errors from warnings. |
pkg/cmd/tknpac/create/repository_test.go |
Adds coverage for rejected and warned namespaces. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ns == "openshift-pipelines" || ns == "tekton-pipelines" { | ||
| return fmt.Errorf("namespace %s is not supported as a target for repositories", ns) | ||
| } | ||
| if strings.HasPrefix(ns, "openshift-") { |
| return repoName, repoNamespace, err | ||
| } | ||
|
|
||
| // checkSystemNamespace returns an error for system namespaces and prints a warning for kube-* prefixes. |
📝 Description of the Change
Add validation in tkn pac to restrict repository creation in system namespaces (kube-, openshift-, tekton-*, tekton-pipelines, openshift-pipelines)
The
checkSystemNamespacehelper is called at two points:--namespaceflag is passed explicitlyBehaviour by namespace:
openshift-pipelines,tekton-pipelines— error, not supported as targetsopenshift-*prefix — error, system namespaces are not supportedtekton-*prefix — warning shown, user can still proceedkube-*prefix — warning shown, user can still proceed🔗 Linked GitHub Issue
Fixes #1350
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.