-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add support for Kubernetes and OpenShift-type DevWorkspace components #961
Conversation
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.
Looks good to me so far, did some testing on OpenShift 4.11 and seems to work as expected 👍
429a5ed
to
c62e0a2
Compare
pkg/library/kubernetes/testdata/k8s_objects/non-k8s-object.yaml
Outdated
Show resolved
Hide resolved
pkg/library/kubernetes/testdata/provision_tests/error-if-object-exists-no-devworkspaceID.yaml
Outdated
Show resolved
Hide resolved
pkg/library/kubernetes/testdata/provision_tests/no-k8s-components-devworkspace.yaml
Outdated
Show resolved
Hide resolved
pkg/library/kubernetes/testdata/provision_tests/error-if-object-exists.yaml
Outdated
Show resolved
Hide resolved
if k8sLikeComponent.Inlined == "" { | ||
continue | ||
} | ||
if k8sLikeComponent.GetDeployByDefault() { |
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.
design question, my undestanding that for k8s / openshift components user explicitly need to set deployByDefault=true
?
As a developer, I'm wondering about the use-case when I craft a devfile, add the k8s component, and do not want it to be applied. For me it is not really clear the necessity of the DeployByDefault
field in general
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.
IIRC the idea behind deployByDefault = false
is that an apply
command could be used to create the resource on demand -- e.g. I run a VSCode task that creates a service/deployment on the cluster. I believe it's more relevant to the odo
use case than it is for DWO, since in our case you could just store the yaml to apply in the repo itself. The original devfile/api issue is devfile/api#693.
One potential use case could be for creating resources without worrying about DWO's serviceAccount having the correct permissions.
specCopy.Spec.InitContainers = nil | ||
specCopy.Spec.Volumes = nil | ||
if !equality.Semantic.DeepDerivative(specCopy.Spec, clusterCopy.Spec) { | ||
return true, false |
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.
Does it return true, false
, because everything else is immutable/should not be changed?
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.
Yes, most pod fields are technically immutable:
Forbidden: pod updates may not change fields other than `spec.containers[*].image`,
`spec.initContainers[*].image`, `spec.activeDeadlineSeconds`, `spec.tolerations` (only
additions to existing tolerations) or `spec.terminationGracePeriodSeconds` (allow it
to be set to 1 if it was previously negative)
Using deployments obscures this fact by deleting + recreating pods when their .spec.template
is updated, so those same fields are mutable there.
|
||
|
||
output: | ||
errRegexp: "object .* exists and is not owned by this workspace" |
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.
no n&N
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.
👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, AObuchow, ibuziuk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bcadf9d
to
b3ec956
Compare
New changes are detected. LGTM label has been removed. |
Codecov ReportBase: 54.11% // Head: 47.60% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #961 +/- ##
==========================================
- Coverage 54.11% 47.60% -6.52%
==========================================
Files 44 60 +16
Lines 2770 3504 +734
==========================================
+ Hits 1499 1668 +169
- Misses 1135 1693 +558
- Partials 136 143 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Add check on create/update DevWorkspaces to verify that user performing the action has RBAC permissions to create/update/delete objects in that Kubernetes type. If not, the operator is rejected. For now, only inlined Kubernetes components are processed to avoid having to always fetch from URI. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Update the sync package to support it attempting to create potentially arbitrary Kubernetes objects, as there are no restrictions (outside of RBAC) on what can be included in Kubernetes/OpenShift components. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Add support for syncing Kubernetes/OpenShift DevWorkspace components to the cluster. If the object is one "recognized" by DWO, it is synced using the same procedure as regular DevWorkspace Objects; otherwise the workspace gets a warning and only the metadata is kept in sync. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
As DevWorkspaces can define Kubernetes objects in components, it's necessary to improve the checks we do to determine if an object on the cluster needs to be updated. This is especially an issue in the case of defaulted fields in objects, as comparing in a straightforward way will mean the object has to be updated every single time. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Implement Unwrap() functions on sync package errors. This allows e.g. retreiving the underlying error behind a sync.UnrecoverableSyncError Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Add functions for comparing Pods to sync package to allow pods to be handled with updates/syncing. This is necessary to allow using pods in workspace kubernetes-type components without having a warning condition set on the DevWorkspace Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Reject DevWorkspaces that use Kubernetes/OpenShift components that define objects the DevWorkspace Operator's service account does not have permissions to manage. This is done to give the user a clearer error message when DWO needs additional permissions. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Duplicate code for checking RBAC for Kubernetes/OpenShift components to accomodate v1alpha1 DevWorkspaces as well. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Add webhook checks to prevent creation/mutation of DevWorkspaceTemplates that define Kubernetes components if the requesting user does not have those permissions. This prevents working around RBAC checks in DevWorkspaces by just creating a DevWorkspaceTemplate instead. Note however that currently DWO does not check that a user creating a workspace has permissions to reference this DevWorkspaceTemplate -- if a user can reference a DWT, they will be able to create the objects defined in that DWT via a DevWorkspace without further RBAC verification. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Since deployByDefault=false components are not intended to be deployed when the DevWorkspace is started, remove restrictions/checks on these components -- i.e. URI is allowed, and we don't check RBAC permissions. These components are meant to be deployed later, e.g. via an apply command, and so verifying RBAC permissions on them is up to the tool used to apply them (e.g. by using the user's token to apply the resources). Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
b3ec956
to
d864400
Compare
What does this PR do?
Add support for components of type Kubernetes and OpenShift to the DevWorkspace Operator, with the caveats
deployByDefault=false
components are ignoredWhat issues does this PR fix or reference?
Closes #798
Is it tested? How?
Test by creating DevWorkspaces that use kubernetes components, e.g.:
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che