add rbac to operator chart #6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds RBAC (Role-Based Access Control) resources to the Helm chart for the airflow-k8s-operator and reorganizes the Python codebase into a more modular structure.
Key Changes:
- Restructured code into
config/andresources/packages for better organization - Added Kubernetes RBAC templates (ClusterRole and ClusterRoleBinding) to grant necessary permissions
- Removed conditional service account creation (now always created)
- Applied consistent code formatting (Black style) across all Python files
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| main.py | Updated imports to reference new resources. module structure |
| resources/variables.py | Moved from root variables.py, updated imports to use config. modules |
| resources/connections.py | Moved from root connections.py, updated imports to use config. modules |
| config/base.py | New file containing base Airflow configuration (host URL handling) |
| config/client.py | New file for API client initialization with basic auth or GCP auth |
| config/gcp.py | New file with Google Cloud Platform authentication logic |
| config/k8s_secret.py | Moved from root k8s_secret.py with formatting updates |
| connections.py | Deleted (moved to resources/connections.py) |
| config.py | Deleted (split into config/ module files) |
| tests/variable_test.py | Applied Black formatting (quote style, line breaks) |
| tests/test_k8s_secret.py | Applied Black formatting (quote style, line breaks) |
| tests/operator_test.py | Applied Black formatting (quote style, line breaks, multi-line statements) |
| chart/airflow-k8s-operator/templates/rbac.yaml | New RBAC resources for operator permissions |
| chart/airflow-k8s-operator/templates/serviceaccount.yaml | Removed conditional creation logic |
| chart/airflow-k8s-operator/templates/deployment.yaml | Added Recreate deployment strategy |
| chart/airflow-k8s-operator/values.yaml | Removed serviceAccount.create field |
| chart/airflow-k8s-operator/Chart.yaml | Bumped version from 0.1.1 to 0.2.0 |
| chart/airflow-k8s-operator/README.md | Updated to remove serviceAccount.create documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| api_client = client.ApiClient(configuration=configuration) | ||
| else: | ||
| raise RuntimeError( | ||
| "Either USE_GOOGLE_AUTH must be true (default) or AIRFLOW_USERNAME and AIRFLOW_PASSWORD must be set" |
There was a problem hiding this comment.
The error message states "Either USE_GOOGLE_AUTH must be true (default)" but this is misleading. Based on the logic, there's no actual default behavior - if USE_GOOGLE_AUTH is not set or is not "true", the code falls through to check for username/password, and only raises an error if neither authentication method is configured.
Consider clarifying the error message:
raise RuntimeError(
"Either set USE_GOOGLE_AUTH=true or provide AIRFLOW_USERNAME and AIRFLOW_PASSWORD"
)| "Either USE_GOOGLE_AUTH must be true (default) or AIRFLOW_USERNAME and AIRFLOW_PASSWORD must be set" | |
| "Either set USE_GOOGLE_AUTH=true or provide AIRFLOW_USERNAME and AIRFLOW_PASSWORD" |
| name: {{ include "airflow-k8s-operator.fullname" . }}-cluster-role | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ include "airflow-k8s-operator.serviceAccountName" . }} |
There was a problem hiding this comment.
The ServiceAccount subject in the ClusterRoleBinding is missing the namespace field. This is required for Kubernetes to correctly bind the ClusterRole to the ServiceAccount in the correct namespace.
Add the namespace field:
subjects:
- kind: ServiceAccount
name: {{ include "airflow-k8s-operator.serviceAccountName" . }}
namespace: {{ .Release.Namespace }}| name: {{ include "airflow-k8s-operator.serviceAccountName" . }} | |
| name: {{ include "airflow-k8s-operator.serviceAccountName" . }} | |
| namespace: {{ .Release.Namespace }} |
| verbs: [get, list, watch] | ||
| - apiGroups: [airflow.drfaust92] | ||
| resources: [variables, connections] | ||
| verbs: [list, watch, get] |
There was a problem hiding this comment.
The RBAC permissions for the custom resources are insufficient. Kopf operators require additional verbs beyond just list, watch, get to properly manage resources. The handlers use create, update, and delete operations which require corresponding Kubernetes API verbs.
Update the permissions to include the necessary verbs:
- apiGroups: [airflow.drfaust92]
resources: [variables, connections]
verbs: [list, watch, get, create, update, patch, delete]Additionally, Kopf needs to update the status subresource, so you may want to add:
- apiGroups: [airflow.drfaust92]
resources: [variables/status, connections/status]
verbs: [get, update, patch]| verbs: [list, watch, get] | |
| verbs: [list, watch, get, create, update, patch, delete] | |
| - apiGroups: [airflow.drfaust92] | |
| resources: [variables/status, connections/status] | |
| verbs: [get, update, patch] |
| @@ -1,8 +1,9 @@ | |||
| import variables # noqa: F401 | |||
| import connections # noqa: F401 | |||
| import resources.variables # noqa: F401 | |||
There was a problem hiding this comment.
Import of 'resources' is not used.
| import resources.variables # noqa: F401 |
| import variables # noqa: F401 | ||
| import connections # noqa: F401 | ||
| import resources.variables # noqa: F401 | ||
| import resources.connections # noqa: F401 |
There was a problem hiding this comment.
Import of 'resources' is not used.
| import resources.connections # noqa: F401 |
No description provided.