Folllow on changes to CONSOLE-2919#612

Closed

TheRealJon wants to merge 3 commits intoopenshift:master-multi-cluster-featurefrom

TheRealJon:CONSOLE-2919-fofllow-on

Member

TheRealJon commented Nov 23, 2021 •

edited

Loading

Addresses my feedback to @florkbr 's PR #602 plus a little bit more polishing.

Create managed cluster oauth cert configmap in openshift-console namespace
Only set managed cluster config file on console config if the managed cluster config map is present
Remove all multicluster related resources when no managed clusters exist or the API is not present.
Simplify some variable and file names
Get rid of a few magic strings and static values (moved to the api package)
Update managed cluster controller to stop syncing if no local oauth client is found.
Update ManagedClusterController.getValidManagedClusters to ManagedClusterController.SyncManagedClusterList for consistency
Add new config map volumes and volume mounts for oauth certs to console operator deployment
Update managed cluster controller to build managed cluster config with oauth data
Add FeatureGate logic to managed cluster controller

openshift-ci Bot requested review from jhadvig and spadgett

November 23, 2021 20:58

TheRealJon force-pushed the CONSOLE-2919-fofllow-on branch from a7ca31e to 82891d3 Compare

November 23, 2021 21:04

Member Author

TheRealJon commented Nov 24, 2021

/retest

1 similar comment

Member Author

TheRealJon commented Nov 29, 2021

/retest

TheRealJon requested review from florkbr and removed request for jhadvig

November 30, 2021 15:40

kdoberst commented Nov 30, 2021

@TheRealJon It looks good to me, but I'm giving @florkbr a chance to take a look before I tag it.

spadgett reviewed

View reviewed changes

Member

spadgett left a comment

Thanks, @TheRealJon

pkg/console/controllers/managedclusters/controller.go

pkg/console/controllers/managedclusters/controller.go Outdated

+              		opt := metav1.CreateOptions{}
+              		mca, err := c.dynamicClient.Resource(api.ManagedClusterActionGroupVersionResource).Namespace(managedClusterName).Create(ctx, required, opt)
+              		if err != nil && apierrors.IsAlreadyExists(err) {
+              			mca, err = c.dynamicClient.Resource(api.ManagedClusterActionGroupVersionResource).Namespace(managedClusterName).Get(ctx, required.GetName(), metav1.GetOptions{})

Member

spadgett Dec 2, 2021

What happens if the console public hostname changes after the managed cluster OAuth client is created? Is the redirect URL updated?

Member Author

TheRealJon Dec 2, 2021 •

edited

Loading

No, I think we will have to add some special logic to patch the spoke cluster OAuth client if the hostname changes.

pkg/console/operator/sync_v400.go Outdated

TheRealJon force-pushed the CONSOLE-2919-fofllow-on branch from 82891d3 to dbcead1 Compare

December 2, 2021 15:58

spadgett approved these changes

View reviewed changes

Member

spadgett left a comment

/lgtm

Thanks @TheRealJon. Let's open a follow up issue for handling console hostname changes.

openshift-ci Bot assigned spadgett

openshift-ci Bot added lgtm approved labels

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

15 similar comments

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 3, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Contributor

openshift-bot commented Dec 4, 2021

/retest-required

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

Member Author

TheRealJon commented Jan 5, 2022

/retest

3 similar comments

Member Author

TheRealJon commented Jan 10, 2022

/retest

Member Author

TheRealJon commented Jan 10, 2022

/retest

Member Author

TheRealJon commented Jan 10, 2022

/retest

jhadvig reviewed

View reviewed changes

Member

jhadvig left a comment

@TheRealJon I see really nice work here! Adding couple of comments.

bindata/configmaps/console-managed-cluster-ingress-cert-configmap.yaml

examples/cvo-unmanage-operator.yaml

manifests/03-rbac-role-cluster.yaml

pkg/console/operator/sync_v400.go Outdated

pkg/console/subresource/configmap/api_server_ca.go Outdated

pkg/console/subresource/configmap/managed_cluster_oauth_server_cert.go

pkg/console/subresource/managedclusteraction/oauth-client.go Outdated

pkg/console/subresource/managedclusterview/oauth-client.go Outdated

Member Author

TheRealJon commented Jan 17, 2022 •

edited

Loading

~~@jhadvig If it's okay with you, I might wait to address these changes in a follow-on PR after we've merged the feature into master since none of the changes seem to be too impactful.~~

Member Author

TheRealJon commented Jan 17, 2022

/retest


          Address various feedback and bugs

241bc05

	- Create managed cluster oauth cert configmap in openshift-console namespace
	- Only set managed cluster config file on console config if the managed cluster config map is present
	- Remove all multicluster related resources when no managed clusters exist or the API is not present.
	- Simplify some variable and file names
	- Get rid of a few magic strings and static values (moved to the api package)
	- Update managed cluster controller to stop syncing if no local oauth client is found.
	- Update ManagedClusterController.getValidManagedClusters to ManagedClusterController.SyncManagedClusterList for consistency
	- Add new config map volumes and volume mounts for oauth certs to console operator deployment
	- Update managed cluster controller to build managed cluster config with oauth data
	- Add FeatureGate logic to managed cluster controller

TheRealJon force-pushed the CONSOLE-2919-fofllow-on branch from 04557e3 to 241bc05 Compare

January 18, 2022 16:58

Contributor

openshift-ci Bot commented Jan 18, 2022

@TheRealJon: TheRealJon unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

Details

In response to this:

/override ci/prow/e2e-gcp

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.


          Address feedback

5c44b47

- Reorder managed cluster controller sync helper func return values so that errors are last
- Use generic console configmap bindata to generate all console configmaps
- Remove update verb from MCA and MCV RBAC rules
- Add error handling for unstructured SetNested... calls

TheRealJon force-pushed the CONSOLE-2919-fofllow-on branch from 7a34ad1 to 5c44b47 Compare

January 18, 2022 22:53

Member

jhadvig commented Jan 19, 2022

/retest

TheRealJon commented

View reviewed changes

pkg/console/controllers/managedclusters/controller.go Outdated

Comment on lines +239 to +246

-              		required := managedclusterviewsub.DefaultOAuthClientView(managedCluster.Name)
-              		mcv, err := c.dynamicClient.Resource(api.ManagedClusterViewGroupVersionResource).Namespace(managedCluster.Name).Create(ctx, required, metav1.CreateOptions{})
-              		if err != nil && apierrors.IsAlreadyExists(err) {
-              			mcvName, _ := managedclusterviewsub.GetName(required)
-              			mcv, err = c.dynamicClient.Resource(api.ManagedClusterViewGroupVersionResource).Namespace(managedCluster.Name).Get(ctx, mcvName, metav1.GetOptions{})
+              		mcv, err := c.dynamicClient.Resource(api.ManagedClusterViewGroupVersionResource).Namespace(managedCluster.Name).Get(ctx, api.OAuthClientManagedClusterViewName, metav1.GetOptions{})
+              		if apierrors.IsNotFound(err) {
+              			required, err := managedclusterviewsub.DefaultOAuthClientView(managedCluster.Name)
+              			if err != nil {
+              				errs = append(errs, fmt.Sprintf("Error initializing oauth client ManagedClusterView for cluster %s: %v", managedCluster.Name, err))
+              				continue
+              			}
+              			mcv, err = c.dynamicClient.Resource(api.ManagedClusterViewGroupVersionResource).Namespace(managedCluster.Name).Create(ctx, required, metav1.CreateOptions{})

Member Author

TheRealJon Jan 19, 2022

Since we have to do error checking on initializing the required MCV, the logic needed to be switched around here. Instead of trying to create the resource, then handling the case where it already exists, we check to see if it already exists, then create it if not.

TheRealJon commented

View reviewed changes

pkg/console/subresource/managedclusteraction/create_oauth_client.go Outdated

Member Author

TheRealJon commented Jan 19, 2022

/retest

jhadvig reviewed

View reviewed changes

Member

jhadvig left a comment

@TheRealJon found a few additional inconsistencies, PTAL

pkg/console/operator/sync_v400.go Outdated

pkg/console/subresource/configmap/managed_cluster_oauth_server_cert.go

pkg/console/subresource/configmap/managed_cluster_oauth_server_cert.go Outdated

pkg/console/subresource/managedclusterview/oauth_client.go Outdated

pkg/console/subresource/managedclusterview/oauth_server_cert.go Outdated

Member Author

TheRealJon commented Jan 19, 2022

@jhadvig I've addressed your feedback, let me know if this looks good and I'll squash


          More feedback

19ced6d

TheRealJon force-pushed the CONSOLE-2919-fofllow-on branch from 52cb5b6 to 19ced6d Compare

January 19, 2022 18:32

Member

spadgett commented Jan 19, 2022

/override ci/prow/e2e-aws-single-node
/override ci/prow/e2e-aws-operator
/override ci/prow/e2e-agnostic-upgrade

Overriding statuses since this is a PR against a feature branch. We will make sure it passes CI before merging to master.

Contributor

openshift-ci Bot commented Jan 19, 2022

@spadgett: Overrode contexts on behalf of spadgett: ci/prow/e2e-agnostic-upgrade, ci/prow/e2e-aws-operator, ci/prow/e2e-aws-single-node

Details

In response to this:

/override ci/prow/e2e-aws-single-node
/override ci/prow/e2e-aws-operator
/override ci/prow/e2e-agnostic-upgrade

Overriding statuses since this is a PR against a feature branch. We will make sure it passes CI before merging to master.

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.

Contributor

openshift-ci Bot commented Jan 19, 2022

@TheRealJon: all tests passed!

Full PR test history. Your PR dashboard.

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 approved these changes

View reviewed changes

Member

jhadvig left a comment

/lgtm

openshift-ci Bot assigned jhadvig

openshift-ci Bot added the lgtm label

Contributor

openshift-ci Bot commented Jan 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, spadgett, TheRealJon

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~~ [jhadvig,spadgett]

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

Member Author

TheRealJon commented Jan 20, 2022

Closing as all of these changes are included in #630

TheRealJon closed this

TheRealJon deleted the CONSOLE-2919-fofllow-on branch

February 4, 2022 19:28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels