Attach dockercfg secrets as pull image secret #284

sleshchenko · 2021-02-25T16:06:52Z

What does this PR do?

This PR add an ability to label secrets with types kubernetes.io/dockercfg and kubernetes.io/dockerconfigjson which controller.devfile.io/devworkspace_pullsecret: true and it will be used as pull secret for DevWorkspace-related deployment.

What issues does this PR fix or reference?

fixes #264

Is it tested? How?

I created two robots and mirror che theia in my private repo, so, they can be used for testing:

# 1. Created patched sample with private image
sed -e 's|image: "quay.io/eclipse/che-theia:next"|image: "quay.io/sleshche/che-theia:latest"|g' ./samples/flattened_theia-next.yaml \
  | kubectl apply -f -

# 2. it won't be started due
#         message: 'rpc error: code = Unknown desc = Error response from daemon: unauthorized:
#         access to the requested resource is not authorized'


# 3. Create tests robots
# Robot without any permissions, just to check that it's ignored since there is no needed label
cat <<EOF | kubectl apply -f -
apiVersion: v1
data:
  .dockerconfigjson: ewoJImF1dGhzIjogewoJCSJxdWF5LmlvIjogewoJCQkiYXV0aCI6ICJjMnhsYzJoamFHVXJhVjlqWVc1ZmJtOTBhR2x1WnpwUE4xcFVSRVExVUZvMFFWVlNWRGxEVlU5VVNEUlZSVXBhUWxBMVRVUXdOVGd5UVRGR1RFWk5NbGxKVkZGQ1R6QTNSMXBEUmtrelZUYzBVRmxTTWpNMCIKCQl9Cgl9Cn0=
kind: Secret
metadata:
  name: blank-robot
  labels: {}
type: kubernetes.io/dockerconfigjson
EOF

# Robot with read access to private quay.io/sleshche/che-theia
cat <<EOF | kubectl apply -f -
apiVersion: v1
data:
  .dockerconfigjson: ewoJImF1dGhzIjogewoJCSJxdWF5LmlvIjogewoJCQkiYXV0aCI6ICJjMnhsYzJoamFHVXJZMmhsWDNSb1pXbGhYM0psWVdSdmJteDVPa1ZOV1ZJeE5sRlRORXBVVVUweFdUZFROVkZHUVZaWlJFazNOME5KVWtaSFVsaGFRMFJVVms0MlUwVXpXRWRSVTFZNVZqTktTVEJXUVZoT09GTTBTVkk9IgoJCX0KCX0KfQ==
kind: Secret
metadata:
  name: che-theia-readonly
  labels:
    controller.devfile.io/devworkspace_pullsecret: "true"
type: kubernetes.io/dockerconfigjson
EOF

# 4. Restart a workspace 

kubectl patch dw theia --type merge --patch '                                     
spec:
  started: false
'
kubectl patch dw theia --type merge --patch '                                     
spec:
  started: true
'

# 5. Workspace now is started.

openshift-ci-robot · 2021-02-25T16:06:54Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sleshchenko · 2021-03-02T09:32:14Z

controllers/workspace/devworkspace_controller.go

@@ -307,6 +307,12 @@ func (r *DevWorkspaceReconciler) Reconcile(req ctrl.Request) (reconcileResult ct
 	serviceAcctName := serviceAcctStatus.ServiceAccountName
 	reconcileStatus.Conditions[devworkspace.WorkspaceServiceAccountReady] = ""

+	pullSecretStatus := provision.PullSecrets(clusterAPI)
+	if !pullSecretStatus.Continue{
+		return reconcile.Result{Requeue: pullSecretStatus.Requeue}, pullSecretStatus.Err


Note that it will be exposed in Workspace status as WAITING FOR DEPLOYMENT, not sure if we should update devfile/api or introduce our own conditions instead

I've added a8cfd71, but note that it will be exposed on workspace status only in case pull secrets failed from first try.

samples/flattened_theia-next.yaml

amisevsk

LGTM with a few comments

controllers/workspace/provision/pull_secret.go

amisevsk · 2021-03-03T15:31:45Z

controllers/workspace/status.go

 	if _, ok := conditions[devworkspace.WorkspaceServiceAccountReady]; ok {
 		return "Waiting for deployment to be ready"
 	}
+	if _, ok := conditions[PullSecretsReadyCondition]; ok {
+		return "Waiting for workspace pull secrets"
+	}
 	if _, ok := conditions[devworkspace.WorkspaceRoutingReady]; ok {
 		return "Waiting for workspace serviceaccount"
 	}


Are we sure this is the right logic? I would assume that if the PullSecretsReady condition is true, then the pull secrets have been prepared and we're not actually waiting on them.

The general approach to this progress message I've taken is once a condition is set, we're waiting on the next condition, i.e.

Suggested change

if _, ok := conditions[devworkspace.WorkspaceServiceAccountReady]; ok {

return "Waiting for deployment to be ready"

}

if _, ok := conditions[PullSecretsReadyCondition]; ok {

return "Waiting for workspace pull secrets"

}

if _, ok := conditions[devworkspace.WorkspaceRoutingReady]; ok {

return "Waiting for workspace serviceaccount"

}

if _, ok := conditions[devworkspace.WorkspaceServiceAccountReady]; ok {

return "Waiting for deployment to be ready"

}

if _, ok := conditions[PullSecretsReadyCondition]; ok {

return "Waiting for workspace service account"

}

if _, ok := conditions[devworkspace.WorkspaceRoutingReady]; ok {

return "Waiting for workspace pull secrets"

}

Each condition gets set in sequence, so the idea is once a condition is set, the workspace is waiting on the next condition in the list -- If conditions[devworkspace.WorkspaceServiceAccountReady] is set, then we know conditions[PullSecretsReadyCondition] is set, etc.

Good catch.
it's downside of multitasking :-D

I'll fix it

I refactored the way how we expose info message d208482

And it seems to work fine but I will need to take a look why
waiting for editor did not appeared, maybe it just passed too fast

> kc get dw theia -w NAME WORKSPACE ID PHASE INFO theia workspace60395b9042b94129 theia workspace60395b9042b94129 theia workspace60395b9042b94129 theia workspace60395b9042b94129 Starting Processing DevWorkspace components theia workspace60395b9042b94129 Starting Processing DevWorkspace components theia workspace60395b9042b94129 Starting Waiting for workspace routing objects theia workspace60395b9042b94129 Starting Waiting for workspace routing objects theia workspace60395b9042b94129 Starting Waiting for workspace routing objects theia workspace60395b9042b94129 Starting Waiting for workspace serviceaccount theia workspace60395b9042b94129 Starting Waiting for workspace serviceaccount theia workspace60395b9042b94129 Starting Waiting for workspace serviceaccount theia workspace60395b9042b94129 Starting Waiting for deployment to be ready theia workspace60395b9042b94129 Starting Waiting for deployment to be ready theia workspace60395b9042b94129 Starting Waiting for deployment to be ready theia workspace60395b9042b94129 Running https://workspace60395b9042b94129.apps.cluster-4237.4237.sandbox422.opentlc.com/theia/ theia workspace60395b9042b94129 Running https://workspace60395b9042b94129.apps.cluster-4237.4237.sandbox422.opentlc.com/theia/ theia workspace60395b9042b94129 Running https://workspace60395b9042b94129.apps.cluster-4237.4237.sandbox422.opentlc.com/theia/ theia workspace60395b9042b94129 Running https://workspace60395b9042b94129.apps.cluster-4237.4237.sandbox422.opentlc.com/theia/

Sometimes it starts quickly :)

pkg/config/constants.go

JPinkney

Other than Angel's comments it looks good to me

amisevsk · 2021-03-06T01:03:28Z

controllers/workspace/status.go

+var PhaseStatusesOrder = []PhaseStatus{
+	{
+		devworkspace.WorkspaceComponentsReady,
+		"Processing DevWorkspace components",
+	},
+	{
+		devworkspace.WorkspaceRoutingReady,
+		"Waiting for workspace routing objects",
+	},
+	{
+		devworkspace.WorkspaceServiceAccountReady,
+		"Waiting for workspace serviceaccount",
+	},
+	{
+		PullSecretsReadyCondition,
+		"Waiting for workspace pull secrets",
+	},
+	{
+		devworkspace.WorkspaceReady,
+		"Waiting for deployment to be ready",
+	},
+}


👍 good idea!

We should consider (in a separate PR) using WorkspaceReady to be the final condition (i.e. after health checks) and add our own DeploymentReady condition to use in its place.

JPinkney

New changes LGTM

openshift-ci-robot · 2021-03-06T03:02:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, sleshchenko

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:

~~OWNERS~~ [JPinkney,amisevsk,sleshchenko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amisevsk · 2021-03-06T03:37:04Z

I also checked that workspaces enter a terminating phase correctly -- we don't have a phase for terminating/deleting, so that is unchanged but the devworkspace shows Cleaning up resources for deletion in the message.

sleshchenko · 2021-03-09T13:47:28Z

/test v5-devworkspaces-operator-e2e

openshift-ci-robot added the do-not-merge/work-in-progress label Feb 25, 2021

openshift-ci-robot requested review from amisevsk and JPinkney February 25, 2021 16:06

openshift-ci-robot added the approved label Feb 25, 2021

sleshchenko force-pushed the dockerCfg branch 2 times, most recently from 3a305e6 to 0d0cab2 Compare March 2, 2021 09:18

sleshchenko commented Mar 2, 2021

View reviewed changes

samples/flattened_theia-next.yaml Show resolved Hide resolved

sleshchenko force-pushed the dockerCfg branch from 0d0cab2 to 08335b1 Compare March 2, 2021 09:37

sleshchenko marked this pull request as ready for review March 2, 2021 09:53

openshift-ci-robot removed the do-not-merge/work-in-progress label Mar 2, 2021

sleshchenko mentioned this pull request Mar 2, 2021

Implement the mechanism to configure dockercfg secret as imagePullSecrets for devworkspace #264

Closed

sleshchenko requested review from amisevsk and JPinkney and removed request for JPinkney and amisevsk March 2, 2021 13:51

sleshchenko self-assigned this Mar 2, 2021

sleshchenko force-pushed the dockerCfg branch from a8cfd71 to e4cf263 Compare March 3, 2021 09:25

amisevsk approved these changes Mar 3, 2021

View reviewed changes

openshift-ci-robot assigned amisevsk Mar 3, 2021

openshift-ci-robot added the lgtm label Mar 3, 2021

sleshchenko added 3 commits March 4, 2021 17:34

Add an ability to mount dockercfg secrets as pull secret

06ff414

Remove vsx installer from Theia sample

f2f8402

Introduce condition to expose pull secrets phase

1fabb49

JPinkney approved these changes Mar 4, 2021

View reviewed changes

openshift-ci-robot assigned JPinkney Mar 4, 2021

sleshchenko force-pushed the dockerCfg branch from e4cf263 to d208482 Compare March 5, 2021 08:03

openshift-ci-robot removed the lgtm label Mar 5, 2021

fixup! Add an ability to mount dockercfg secrets as pull secret

5a87bf6

Simplify the way we evaluate info message

3a159c7

sleshchenko force-pushed the dockerCfg branch from d208482 to 3a159c7 Compare March 5, 2021 09:13

amisevsk approved these changes Mar 6, 2021

View reviewed changes

openshift-ci-robot added the lgtm label Mar 6, 2021

JPinkney approved these changes Mar 6, 2021

View reviewed changes

amisevsk merged commit 32de259 into main Mar 9, 2021

amisevsk deleted the dockerCfg branch March 9, 2021 21:27

sleshchenko mentioned this pull request Sep 24, 2021

[devworkspace-che-operator] Prototype and plan of controller that is reconciling resources in user's namespace eclipse-che/che#20219

Closed

sleshchenko mentioned this pull request Oct 1, 2021

Container registries should create directly dockercfg secret when is run in DevWorkspace mode eclipse-che/che#18990

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach dockercfg secrets as pull image secret #284

Attach dockercfg secrets as pull image secret #284

sleshchenko commented Feb 25, 2021 •

edited

Loading

openshift-ci-robot commented Feb 25, 2021

sleshchenko Mar 2, 2021

sleshchenko Mar 2, 2021

amisevsk left a comment

amisevsk Mar 3, 2021

sleshchenko Mar 3, 2021

sleshchenko Mar 5, 2021 •

edited

Loading

amisevsk Mar 6, 2021

JPinkney left a comment

amisevsk Mar 6, 2021

JPinkney left a comment

openshift-ci-robot commented Mar 6, 2021

amisevsk commented Mar 6, 2021

sleshchenko commented Mar 9, 2021

Attach dockercfg secrets as pull image secret #284

Attach dockercfg secrets as pull image secret #284

Conversation

sleshchenko commented Feb 25, 2021 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

openshift-ci-robot commented Feb 25, 2021

sleshchenko Mar 2, 2021

Choose a reason for hiding this comment

sleshchenko Mar 2, 2021

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Mar 3, 2021

Choose a reason for hiding this comment

sleshchenko Mar 3, 2021

Choose a reason for hiding this comment

sleshchenko Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

amisevsk Mar 6, 2021

Choose a reason for hiding this comment

JPinkney left a comment

Choose a reason for hiding this comment

amisevsk Mar 6, 2021

Choose a reason for hiding this comment

JPinkney left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 6, 2021

amisevsk commented Mar 6, 2021

sleshchenko commented Mar 9, 2021

sleshchenko commented Feb 25, 2021 •

edited

Loading

sleshchenko Mar 5, 2021 •

edited

Loading