Skip to content

CONSOLE-2892: Allow dynamic plugins to proxy to services on the cluster#1020

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jhadvig:CONSOLE-2892
Nov 4, 2021
Merged

CONSOLE-2892: Allow dynamic plugins to proxy to services on the cluster#1020
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jhadvig:CONSOLE-2892

Conversation

@jhadvig
Copy link
Copy Markdown
Member

@jhadvig jhadvig commented Oct 5, 2021

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2021
@openshift-ci openshift-ci Bot requested review from knobunc and spadgett October 5, 2021 18:27
spadgett
spadgett reviewed Oct 6, 2021
View reviewed changes
Comment thread console/v1alpha1/0000_10_consoleplugin.crd.yaml Outdated
Comment thread console/v1alpha1/types_console_plugin.go Outdated
Comment thread console/v1alpha1/types_console_plugin.go Outdated
Comment thread console/v1alpha1/types_console_plugin.go Outdated
Comment thread console/v1alpha1/0000_10_consoleplugin.crd.yaml Outdated
Comment thread console/v1alpha1/types_console_plugin.go
Comment thread console/v1alpha1/types_console_plugin.go Outdated
CACertificate string `json:"caCertificate"`
// authorize indicates if the proxied request will contain
// user's OpenShift access token. By default the access token
// is not part of the proxied request.
Copy link
Copy Markdown
Member

@spadgett spadgett Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should mention how the token is passed in the Authorization request header.

@jhadvig jhadvig changed the title [WIP] CONSOLE-2892: Allow dynamic plugins to proxy to services on the cluster CONSOLE-2892: Allow dynamic plugins to proxy to services on the cluster Oct 7, 2021
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 7, 2021
@jhadvig
Copy link
Copy Markdown
Member Author

jhadvig commented Oct 7, 2021

@spadgett I've updated the PR. PTAL

@jhadvig jhadvig mentioned this pull request Oct 7, 2021
Merged
@jhadvig jhadvig force-pushed the CONSOLE-2892 branch 2 times, most recently from 5dddb21 to 473eacc Compare November 1, 2021 15:50
@jhadvig
Copy link
Copy Markdown
Member Author

jhadvig commented Nov 1, 2021 •
edited
Loading

PR is ready for final review.

Console Approver:
/assign @spadgett

@RickJWagner
Copy link
Copy Markdown

RickJWagner commented Nov 2, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2021
@opayne1
Copy link
Copy Markdown

opayne1 commented Nov 2, 2021

/label docs-approved

@openshift-ci openshift-ci Bot added the docs-approved Signifies that Docs has signed off on this PR label Nov 2, 2021
spadgett
spadgett reviewed Nov 2, 2021
View reviewed changes
Comment thread console/v1alpha1/types_console_plugin.go Outdated
Comment thread console/v1alpha1/types_console_plugin.go
Comment thread console/v1alpha1/types_console_plugin.go Outdated
// +kubebuilder:validation:Required
// +required
Service ConsolePluginService `json:"service"`
// proxy is a list of services that the plugin needs to connect to.
Copy link
Copy Markdown
Member

@spadgett spadgett Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use consistent case throughout, either Services or services

Suggested change
// proxy is a list of services that the plugin needs to connect to.
// proxy is a list of Services that the plugin needs to connect to.

Comment thread console/v1alpha1/types_console_plugin.go Outdated
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2021
@jhadvig
Copy link
Copy Markdown
Member Author

jhadvig commented Nov 3, 2021

@spadgett I've addressed your comments. lets the service CA with lowercased starting 's' since kubernetes docs are using it this way as well.
PTAL

@yapei
Copy link
Copy Markdown

yapei commented Nov 4, 2021

verified the changes in console-operator PR and it works
/label qe-approved

@openshift-ci openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Nov 4, 2021
spadgett
spadgett approved these changes Nov 4, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Comment thread console/v1alpha1/types_console_plugin.go
// ConsolePluginProxyService holds information on Service to which
// console's backend will proxy the plugin's requests.
type ConsolePluginProxyService struct {
// name of Service that the plugin needs to connect to.
Copy link
Copy Markdown
Member

@spadgett spadgett Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we're using inconsistent punctuation here and below where we don't have a .

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Nov 4, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2021
@openshift-merge-robot openshift-merge-robot merged commit 5088c83 into openshift:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett approved these changes

@knobunc knobunc Awaiting requested review from knobunc

Assignees

@RickJWagner RickJWagner

@spadgett spadgett

@yapei yapei

@christianvogt christianvogt

@opayne1 opayne1

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jhadvig @RickJWagner @opayne1 @yapei @spadgett @christianvogt @openshift-merge-robot