-
Notifications
You must be signed in to change notification settings - Fork 30
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
refactor: Remove Redundancy and duplication, check for same naming pattern on Kustomize #1721
Merged
kyma-bot
merged 22 commits into
kyma-project:main
from
jeremyharisch:refactorK8sResourceNames
Aug 6, 2024
Merged
refactor: Remove Redundancy and duplication, check for same naming pattern on Kustomize #1721
kyma-bot
merged 22 commits into
kyma-project:main
from
jeremyharisch:refactorK8sResourceNames
Aug 6, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kyma-bot
added
do-not-merge/work-in-progress
Indicates that a PR should not merge because it is a work in progress.
cla: yes
Indicates the PR's author has signed the CLA.
size/L
Denotes a PR that changes 100-499 lines, ignoring generated files.
labels
Jul 25, 2024
jeremyharisch
changed the title
WIP: Remove Redundancy and duplication, check for same naming pattern
refactor: Remove Redundancy and duplication, check for same naming pattern on Kustomize
Jul 26, 2024
kyma-bot
removed
the
do-not-merge/work-in-progress
Indicates that a PR should not merge because it is a work in progress.
label
Jul 26, 2024
7 tasks
c-pius
reviewed
Jul 29, 2024
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.
Questions:
- "controller-manager" term is the default by kubebuilder ("The Manager is an executable that wraps one or more Controllers") and seems to be common within K8s (e.g., kube-controller-manager). Should we keep this as such therefore?
- is there a specific reason why we use "lifecycle-manager" prefix local and "kcp" prefix for control-plane? Why not use the same prefix?
- we still have a inconsistency in that we sometimes include the type, e.g., for bindings, services, certs, ..., but sometimes we also omit it, e.g., for deployment, service account, configmap, ... We also use "role" for both Role and ClusterRole, same with rolebinding for Rolebinding and ClusterRoleBinding. Do we even need to include the type in the name? What is our use case for including it?
How about trying to get to something like the following:
---
kymas.operator.kyma-project.io
CustomResourceDefinition
---
manifests.operator.kyma-project.io
CustomResourceDefinition
---
moduletemplates.operator.kyma-project.io
CustomResourceDefinition
---
watchers.operator.kyma-project.io
CustomResourceDefinition
---
# the service account for the controller-manager
klm-controller-manager
ServiceAccount
---
# permissions for the controller-manager to perform leader election
- klm-leader-election-role
+ klm-controller-manager-leader-election
Role
---
# base permissions for controller-manager to perform its tasks
- klm-manager-role
+ klm-controller-manager
ClusterRole
---
# additional permissions for controller-manager to perform tasks on CRDs
- klm-manager-role-crd
+ klm-controller-manager-crds
ClusterRole
---
# additional permissions for controller-manager to read from non-resource nedpoint /metrics
- klm-metrics-reader
+ klm-controller-manager-metrics
ClusterRole
---
# Permissions for KLM to read watcher's cert-manager-related resources
- klm-manager-role-istio-namespace
+ klm-controller-manager-watcher-certmanager
Role
---
# Permissions for KLM to read resources residing in the SKR
- manager-role-remote-namespace
+ klm-controller-manager-skr
Role
---
# binds the watcher's cert-manager-related resources role
- manager-rolebinding-istio-namespace
+ klm-controller-manager-watcher-certmanager
RoleBinding
---
# binds the SKR-realted resources role
- manager-rolebinding-remote-namespace
+ klm-controller-manager-skr
RoleBinding
---
# binds the leader election role to kcp-sytem namespace
- klm-leader-election-rolebinding
+ klm-controller-manager-leader-election
RoleBinding
---
# binds the base permissions for kcp-system namespace
# since kcp-sysem is the expected namespace, no additional suffix?
- klm-manager-rolebinding-kcp-system
+ klm-controller-manager
RoleBinding
---
# binds the metrics permissions for kcp-system namespace
- klm-metrics-rolebinding
+ klm-controller-manager-metrics
RoleBinding
---
# binds the CRD permissions
- klm-manager-rolebinding-crd
+ klm-controller-manager-crds
ClusterRoleBinding
---
# config defining a plutono dashboard
klm-dashboard-mandatory-modules
ConfigMap
---
# config defining a plutono dashboard
klm-dashboard-overview
ConfigMap
---
# config defining a plutono dashboard
klm-dashboard-status
ConfigMap
---
# config defining a plutono dashboard
klm-dashboard-watcher
ConfigMap
---
# service exposing events from the controller-manager
- klm-event-service
+ klm-controller-manager-events
Service
---
# service exposing metrics from the controller-manager
- klm-metrics-service
+ klm-controller-manager-metrics
Service
---
# service exposing the controller-manager's webhook
- klm-webhook-service
+ ~~klm-controller-manager-webhook~~
+ klm-webhook-service
Service
---
# deployment wrapping the klm controllers
klm-controller-manager
Deployment
---
# serving cert for watcher
- klm-watcher-serving-cert
+ klm-watcher-serving
Certificate
---
# serving cert for controller-manager webhook
- klm-serving-cert
+ klm-controller-manager-webhook-serving
Certificate
---
# self signed cluster issuer for watcher
- klm-watcher-selfsigned-cluster-issuer
+ klm-watcher-selfsigned
ClusterIssuer
---
# self signed issuer for watcher
- klm-watcher-selfsigned-issuer
+ klm-watcher-selfsigned
Issuer
---
# self signed issues for controller-manager
- klm-selfsigned-issuer
+ klm-controller-manager-selfsigned
Issuer
---
# metrics monitor for controller-manager's metrics
- klm-controller-manager-metrics-monitor
+ klm-controller-manager-metrics
ServiceMonitor
---
# gateway for watcher
- klm-watcher-gateway
+ klm-watcher
Gateway
---
# watcher
- klm-kyma-watcher
+ klm-watcher
Watcher
---
klm-controller-manager
AuthorizationPolicy
---
- klm-validating-webhook-configuration
+ ~~klm-controller-manager~~
+ klm-validating-webhook-configuration
ValidatingWebhookConfiguration TODOs
|
c-pius
force-pushed
the
refactorK8sResourceNames
branch
from
July 30, 2024 12:14
9b4de08
to
691dd32
Compare
c-pius
reviewed
Jul 30, 2024
ruanxin
reviewed
Jul 31, 2024
lindnerby
added
the
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Aug 1, 2024
Renaming ready now. We are waiting for feedback from SRE before merging. |
lindnerby
approved these changes
Aug 6, 2024
c-pius
removed
the
do-not-merge/hold
Indicates that a PR should not merge because someone has issued a /hold command.
label
Aug 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Changes proposed in this pull request:
manager
from the kustomize setup, since it is duplicated. I.e.klm-controller-manager-role
which reads askyma-lifecycle-manager-controller-manager-role
, or in default profilelifecycle-manager-manager-role
. Use onlycontroller
insteadklm-
prefix in control-plane setup<prefix>-<resource type>-<details>
klm-leader-election-role
→klm-controller-role-leader-election
klm-manager-controler-role
→klm-controller-role
Old resources
default
control-plane profile
New Resources
default
control plane
Related issue(s)