Add ConsoleCliDownloads controller #306

jhadvig · 2019-10-03T14:59:55Z

https://jira.coreos.com/browse/CONSOLE-1423

Adding ConsoleCliDownloads controller that will take care of ConsoleCliDownloads oc binaries for different platforms.

/assign @benjaminapetersen

pkg/console/controllers/clidownloads/controller.go

benjaminapetersen · 2019-10-04T02:04:39Z

pkg/console/controllers/clidownloads/controller.go

+	}
+}
+
+func GetOrCreate(consoleClient consolev1client.ConsoleCLIDownloadInterface, required *v1.ConsoleCLIDownload) (*v1.ConsoleCLIDownload, bool, error) {


Contemplating, do we want a full Apply for this? If it exists, but someone tampers with it, we probably want to stomp it back to the correct values.

pkg/console/controllers/clidownloads/controller.go

pkg/console/starter/starter.go

jhadvig · 2019-10-08T13:46:11Z

@benjaminapetersen updated the PR. The controller will make sure the ConsoleCliDownload CR will be created for oc and odo (@spadgett FYI)
Already started to work on the unit tests but need to align on the format of the controller.

Also I've seen #309 ... after it will land will update the PR with HandleDegraded as we spoke.

PTAL

manifests/03-rbac-role-cluster-extensions.yaml

pkg/console/controllers/clidownloads/controller.go

pkg/console/starter/starter.go

benjaminapetersen · 2019-10-09T14:17:16Z

pkg/console/starter/starter.go

 		resourceSyncer,
 	)

+	cliDownloadsController := clidownloads.NewCLIDownloadsSyncController(


Lets organize these with client/informer next to each other, client first like https://github.com/openshift/console-operator/pull/310/files#diff-69ecbb024dab972c8729678abdf9c0faR79.

I think it will help, granted . don't think there will be much added to this.

cliDownloadsController := clidownloads.NewCLIDownloadsSyncController( // operator is to set status, correct? operatorConfigClient.OperatorV1(), operatorClient, // client, informer pair for ConsoleCLIDownloads consoleClient.ConsoleV1().ConsoleCLIDownloads(), consoleInformers.Console().V1().ConsoleCLIDownloads(), // route just to get route... but should we be informed of a route change? routesClient.RouteV1(), recorder, )

I think we prob want the routeInformer. If the Host changes on the route, then we need to update the CLIDownloads.

I don't think that we need the top-level console informer for this. I considered suggesting it, but nothing user configurable matters.

Quick thoughts on this one:

// top level config (only needs informer, we don't manipulate, so no need to use client) operatorClient, // operator (just client, because we DO manipulate status, but we don't need to be informed of changes for this, however, it wouldn't hurt us to kick if the operatorconfig did change) operatorConfigClient.OperatorV1(), // cli download client (need both client and informer because we both need to know about changes and make changes) consoleClient.ConsoleV1().ConsoleCLIDownloads(), consoleInformers.Console().V1().ConsoleCLIDownloads(), // routes (I believe we only need the informer, we aren't changing the route, we just need to know if something else changes) routesInformersNamespaced.Route().V1().Routes(), // recorder recorder,

Check that, you'll need both for route:

routeClient // so we can GET the downloads route & pluck out the canonical host. routeInformer // notify us of any route changes

We have to be informed of changes AND get the route to use the host, my bad.

pkg/console/controllers/clidownloads/controller.go

jhadvig · 2019-10-10T11:42:36Z

@benjaminapetersen Ive updated the PR based on comments also added comments.

Added a removeCLIDownloads() method for the Removed operator state.

Only thing that is missing are the e2e tests that I've started to work on, but I think it would be better to address them as a separate PR, since this is already quite beefy 😄

Other thing is that we need to figure out #306 (comment)

PTAL

benjaminapetersen · 2019-10-15T14:38:02Z

/lgtm

benjaminapetersen · 2019-10-15T14:57:48Z

/retest

Terraform failed.

openshift-bot · 2019-10-15T15:58:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-15T18:08:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

benjaminapetersen · 2019-10-15T20:21:22Z

And here is our test failure:

fail [github.com/openshift/origin/test/extended/authorization/rbac/groups_default_rules.go:221]: Oct 15 19:10:38.376: system:authenticated has extra permissions in namespace "":
{APIGroups:["console.openshift.io"], Resources:["consoleclidownloads"], Verbs:["create" "update" "delete"]}

@spadgett

openshift-bot · 2019-10-15T22:02:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-15T23:33:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

benjaminapetersen · 2019-10-16T00:59:05Z

/retest

openshift/origin#23975 has merged so test should be unblocked.

openshift-bot · 2019-10-16T02:48:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-10-16T05:37:12Z

/retest

Please review the full test history for this PR and help us cut down flakes.

jhadvig · 2019-10-16T07:46:35Z

/retest

jhadvig · 2019-10-16T09:11:06Z

/retest

jhadvig · 2019-10-16T11:05:04Z

/retest

jhadvig · 2019-10-16T12:45:04Z

/retest

openshift-bot · 2019-10-16T12:45:37Z

/retest

Please review the full test history for this PR and help us cut down flakes.

spadgett · 2019-10-16T13:30:33Z

manifests/03-rbac-role-cluster-extensions.yaml

+  - get
+  - list
+  - watch
+  - create


This should only be get/list/watch

I think he needs full CRUD for this one, since the operator is creating 2 of this resource, or updating if tampered with.

I updated the test incorrectly, added this resource to the same list as the rest of our resources. My mistake, I forgot this resource is more actively managed than our others.

So you probably need to pull the consoleclidownloads out of ClusterRole console-extensions-reader file and put it in its own file. Name the role something like console-clidownloads-extension-manager and then create a ClusterRoleBinding for console-operator.

Then we can update the origin test to allow this.

No, this is wrong. This is the role that gets assigned to system:authenticated. We cannot let all users edit these. The operator role is a different one.

I updated the test incorrectly, added this resource to the same list as the rest of our resources. My mistake, I forgot this resource is more actively managed than our others.

Your origin test updates are correct.

Right, intended to mean "console operator only" definitely not, "everyone".

spadgett · 2019-10-16T14:05:00Z

/lgtm cancel
RBAC is incorrect

openshift-ci-robot · 2019-10-17T23:18:02Z

@jhadvig: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`598c5ad`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jhadvig · 2019-10-18T11:29:14Z

@benjaminapetersen PR updated. PTAL

benjaminapetersen · 2019-10-18T14:03:25Z

/lgtm

openshift-ci-robot · 2019-10-18T14:03:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jhadvig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen]

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

openshift-ci-robot assigned benjaminapetersen Oct 3, 2019

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 3, 2019

jhadvig mentioned this pull request Oct 3, 2019

Add URL for downloading CLI to the console-config configmap #301

Closed

openshift-ci-robot requested review from benjaminapetersen and spadgett October 3, 2019 15:00

benjaminapetersen reviewed Oct 4, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Oct 4, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed Oct 4, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed Oct 4, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Outdated Show resolved Hide resolved

benjaminapetersen reviewed Oct 7, 2019

View reviewed changes

pkg/console/starter/starter.go Outdated Show resolved Hide resolved

benjaminapetersen mentioned this pull request Oct 7, 2019

Make status handlers standalone helper funcs #309

Merged

jhadvig force-pushed the cli_url_crd branch 2 times, most recently from 227b26e to 5690834 Compare October 8, 2019 13:41

benjaminapetersen mentioned this pull request Oct 8, 2019

simplify download server #307

Closed

jhadvig force-pushed the cli_url_crd branch from 5690834 to fc0daf4 Compare October 9, 2019 13:55

jhadvig changed the title ~~[WIP] Add ConsoleCliDownloads controller~~ Add ConsoleCliDownloads controller Oct 9, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2019

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

manifests/03-rbac-role-cluster-extensions.yaml Outdated Show resolved Hide resolved

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

pkg/console/starter/starter.go Show resolved Hide resolved

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

jhadvig force-pushed the cli_url_crd branch from fc0daf4 to ce6c655 Compare October 9, 2019 14:50

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Show resolved Hide resolved

benjaminapetersen reviewed Oct 9, 2019

View reviewed changes

pkg/console/controllers/clidownloads/controller.go Show resolved Hide resolved

jhadvig force-pushed the cli_url_crd branch 2 times, most recently from f18dbdd to 1502751 Compare October 10, 2019 11:24

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 15, 2019

benjaminapetersen mentioned this pull request Oct 15, 2019

Add consoleyamlsamples to list of console resource exceptions openshift/origin#23975

Merged

spadgett reviewed Oct 16, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Oct 16, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2019

Add ConsoleCliDownloads controller

91024de

jhadvig force-pushed the cli_url_crd branch from 598c5ad to 91024de Compare October 18, 2019 09:32

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2019

openshift-merge-robot merged commit 8ffe751 into openshift:master Oct 18, 2019

Add ConsoleCliDownloads controller #306

Add ConsoleCliDownloads controller #306

Uh oh!

Conversation

jhadvig commented Oct 3, 2019 • edited by spadgett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhadvig commented Oct 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhadvig commented Oct 10, 2019

Uh oh!

benjaminapetersen commented Oct 15, 2019

Uh oh!

benjaminapetersen commented Oct 15, 2019

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

benjaminapetersen commented Oct 15, 2019

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

openshift-bot commented Oct 15, 2019

Uh oh!

benjaminapetersen commented Oct 16, 2019

Uh oh!

openshift-bot commented Oct 16, 2019

Uh oh!

openshift-bot commented Oct 16, 2019

Uh oh!

jhadvig commented Oct 16, 2019

Uh oh!

jhadvig commented Oct 16, 2019

Uh oh!

jhadvig commented Oct 16, 2019

Uh oh!

jhadvig commented Oct 16, 2019

Uh oh!

openshift-bot commented Oct 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Oct 16, 2019

jhadvig commented Oct 3, 2019 •

edited by spadgett

Loading

benjaminapetersen Oct 10, 2019 •

edited

Loading

openshift-ci-robot commented Oct 17, 2019 •

edited

Loading