Bug 1764704: Sync router-ca to the console namespace #328

jhadvig · 2019-10-31T11:10:30Z

Syncing the router-ca from openshift-config-managed namespace to openshift-console namespace and pasing to the console deployment.

Will update the unit tests after the first round of review to avoid unnecessary rewrites.

/assign @benjaminapetersen

openshift-ci-robot · 2019-10-31T11:10:34Z

@jhadvig: This pull request references Bugzilla bug 1764704, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1764704: Sync router-ca to the console namespace

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.

spadgett · 2019-10-31T12:47:42Z

cc @csrwng @sjenning

spadgett · 2019-10-31T12:49:47Z

pkg/console/subresource/deployment/deployment.go

+		{
+			name:        api.RouterCAConfigMapName,
+			readOnly:    true,
+			path:        "/var/router-ca",


We need to set this as the oauthEndpointCAFile in the console config map.

Should we conditionally fall back to the previous CA (not add this volume) if the router-ca does not exist for some reason?

Like, if we hit FailedGet or MissingRouterCABundle on the RouterCAValidation check, can we still roll out a usable console.

pkg/console/controllers/router/controller.go

pkg/console/starter/starter.go

jhadvig · 2019-10-31T16:08:52Z

@benjaminapetersen @spadgett comments addressed

jhadvig · 2019-11-04T09:41:20Z

/retest

jhadvig · 2019-11-04T11:01:59Z

/retest

jhadvig · 2019-11-05T10:24:48Z

53) Modal Annotations
A Jasmine spec timed out. Resetting the WebDriver Control Flow.

/retest

jhadvig · 2019-11-05T14:25:11Z

@benjaminapetersen updated the PR, with couple of nits I've found... PTAL

benjaminapetersen · 2019-11-05T14:33:09Z

pkg/console/controllers/resourcesync/controller.go

+	recorder     events.Recorder
+}
+
+func NewResourceSyncController(


I may be inclined to use the previous name, or come up with something that isn't so similar to the resourcesynccontroller.NewResourceSyncController() in starter.go. This might get confusing later.

resourcesynccontroller.NewResourceSyncController() is responsible for grabbing source files from other namespaces and syncing them to destination files in our namespace.

This ctrl is currently tasked with maintaining these resources (delete mostly) based in management state.

benjaminapetersen · 2019-11-05T14:43:40Z

pkg/console/controllers/resourcesync/controller.go

+
+	switch operatorConfig.Spec.ManagementState {
+	case operatorsv1.Managed:
+		klog.V(4).Infoln("console is in a managed state: syncing secrets and configmaps")


For now, we are only syncing the one, router-ca. Lets update the comment to this to ensure the logs are accurate. Perhaps we can comment on ResourceSyncController struct above indicating the future intent, that this ctrl could maintain all the boring resources like configmaps/secrets that just need to exist/not exist based on ManagementState.

benjaminapetersen · 2019-11-05T14:46:09Z

pkg/console/operator/sync_v400.go

 	return okToMount, reason, err
 }

+func (c *consoleOperator) ValidateRouterCAConfigMap() (routerCA *corev1.ConfigMap, reason string, err error) {


Pondering if this can live in the new controller instead of adding it here.

Ah, probably not now yet, as the actualDeployment, depChanged, depErrReason, depErr := co.SyncDeployment(set.Operator, cm, serviceCAConfigMap, routerCAConfigMap, trustedCAConfigMap, sec, rt, set.Proxy, customLogoCanMount) needs it. Future TODO: break that down into smaller bits.

benjaminapetersen · 2019-11-05T15:00:35Z

pkg/console/starter/starter.go


+	// resourceSyncController contains additional locig for all the secrets and
+	// configmaps that we resourceSyncer is taking care of
+	resourceSyncController := resourcesync.NewResourceSyncController(


NIT: I still think we should pair up client-informer for related resources:

// operatorconfig operatorConfigClient.OperatorV1().Consoles(), operatorConfigInformers.Operator().V1().Consoles(), // configmap kubeClient.CoreV1(), kubeInformersNamespaced.Core().V1().ConfigMaps(),

Dont have any any strong feelings against. Done it this way cause the controllers we started to create have the arguments structured in this way - clients | informers | ...

I updated this recently:
https://github.com/openshift/console-operator/blob/master/pkg/console/starter/starter.go#L149 in #310.

jhadvig · 2019-11-06T16:28:33Z

/hold

jhadvig · 2019-11-06T16:31:15Z

@benjaminapetersen updated the PR with additional commit that is adding the fallback to the /var/run/secrets/kubernetes.io/serviceaccount/ca.crt if the router-ca is not synced in the openshift-console namespace.
Will update also the unit tests after the review... PTAL

benjaminapetersen · 2019-11-06T17:21:39Z

pkg/console/operator/sync_v400.go

+	// If the `router-ca` configmap in `openshift-console` exist we should mount that to the console container,
+	// otherwise default to `/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`
+	_, rcaErr := co.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(api.RouterCAConfigMapName, metav1.GetOptions{})
+	if rcaErr != nil && apierrors.IsNotFound(rcaErr) {


what about other errors? this only covers 404.

is there anything else that would matter?

was thinking about it and wasn't really sure if any other errors should matter in this context... saying that not really sure what should be the reason and type of the created condition, if there will be other error type... also if it makes sense to even create one in this case...

Maybe we should just add a log and call it good:

if rcaErr != nil { klog.V(4).Infof("router-ca GET error: %s",rcaErr) }

Agree with you, probably doesn't matter, unless there is some funny state we aren't thinking about. But a 4+ log level dumping some detail may be helpful.

benjaminapetersen · 2019-11-06T17:23:48Z

pkg/console/subresource/consoleserver/config_builder.go

-	clientSecretFilePath    = "/var/oauth-config/clientSecret"
-	oauthEndpointCAFilePath = "/var/router-ca/ca-bundle.crt"
+	clientSecretFilePath           = "/var/oauth-config/clientSecret"
+	oauthEndpointCAFilePath        = "/var/router-ca/ca-bundle.crt"


Lets fix the name here as well, routerCAFIlePath vs oauthEndpointCAFilePath.
oauthEndpointCAFilePath and defaultOAuthEndpointCAFilePath sound like the same thing.

yeah, probably it will be better to give the true names with an additional comment

benjaminapetersen · 2019-11-06T17:25:12Z

pkg/console/subresource/consoleserver/config_builder.go

 	statusPageID      string
 	customProductName string
 	customLogoFile    string
+	routerCA          string


This property should prob be generically CAFile, as it may be the router-CA, or it may not. The if check below decides which.

pkg/console/starter/starter.go

jhadvig

@benjaminapetersen comments addressed.
Wasn't really sure if it make sense to have report and condition here since if there is not the router-ca than we have a valid fallback...

PTAL

jhadvig · 2019-11-07T14:12:15Z

hack/test-unit.sh

    "github.com/openshift/console-operator/pkg/console/operator"
    "github.com/openshift/console-operator/pkg/console/starter"
    "github.com/openshift/console-operator/pkg/console/subresource/configmap"
+    "github.com/openshift/console-operator/pkg/console/subresource/consoleserver"


It appears we have been missing this pkg in our unit tests

Not ideal to have to manually add these eh? Prob should update this script to loop the directory instead, but looks good for now.

jhadvig · 2019-11-07T14:13:19Z

pkg/console/subresource/consoleserver/config_builder.go


 func (b *ConsoleServerCLIConfigBuilder) authServer() Auth {
+	// we need this fallback due to the way our unit test are structured,
+	// where the ConsoleServerCLIConfigBuilder object is being instantiated empty


not a bad thing I thing... It's good to have a fallback here...

It can't function correctly without one or the other, correct? Is it just because of unit test structure, or is the config broken without an entry?

jhadvig · 2019-11-07T14:17:03Z

/hold cancel

benjaminapetersen · 2019-11-07T16:02:00Z

pkg/console/subresource/configmap/configmap_test.go

  clientID: console
  clientSecretFile: /var/oauth-config/clientSecret
-  oauthEndpointCAFile: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt
+  oauthEndpointCAFile: /var/router-ca/ca-bundle.crt


I think we may want to add 2 new tests here:

name: "Test configmap with router-ca", name: "Test configmap without router-ca",

Probably can duplicate the test name: "Test default configmap, no customization", and just swap the bool & output. This would make it explicit.

benjaminapetersen · 2019-11-07T18:27:50Z

/lgtm

openshift-ci-robot · 2019-11-07T18:28:19Z

[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

jhadvig · 2019-11-07T19:30:20Z

/retest

openshift-ci-robot · 2019-11-07T20:30:25Z

@jhadvig: All pull requests linked via external trackers have merged. Bugzilla bug 1764704 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1764704: Sync router-ca to the console namespace

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.

openshift-ci-robot assigned benjaminapetersen Oct 31, 2019

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 31, 2019

openshift-ci-robot requested review from benjaminapetersen and spadgett October 31, 2019 11:11

spadgett reviewed Oct 31, 2019

View reviewed changes

benjaminapetersen reviewed Oct 31, 2019

View reviewed changes

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

benjaminapetersen reviewed Oct 31, 2019

View reviewed changes

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

benjaminapetersen reviewed Oct 31, 2019

View reviewed changes

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

jhadvig force-pushed the bz1764704 branch 2 times, most recently from f3bf5cf to cb632ca Compare November 5, 2019 14:24

benjaminapetersen suggested changes Nov 5, 2019

View reviewed changes

benjaminapetersen reviewed Nov 5, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2019

benjaminapetersen reviewed Nov 6, 2019

View reviewed changes

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

jhadvig force-pushed the bz1764704 branch from 7863608 to c1963e7 Compare November 7, 2019 14:07

jhadvig force-pushed the bz1764704 branch from c1963e7 to 14f28b5 Compare November 7, 2019 14:11

jhadvig commented Nov 7, 2019

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2019

benjaminapetersen reviewed Nov 7, 2019

View reviewed changes

Bug 1764704: Sync router-ca to the console namespace

9768455

jhadvig force-pushed the bz1764704 branch from 14f28b5 to 9768455 Compare November 7, 2019 18:07

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2019

openshift-merge-robot merged commit a4f6120 into openshift:master Nov 7, 2019

codyhoag mentioned this pull request Nov 7, 2019

Bug 1712525, Ingress Controller requires CA to communicate with web console openshift/openshift-docs#18004

Closed

benjaminapetersen mentioned this pull request Nov 26, 2019

bug 1772759: fix console operator panic when RouterCAValidationDegraded #355

Merged

jhadvig mentioned this pull request Dec 10, 2019

Sync default-ingress-cert to the console namespace #361

Merged

Bug 1764704: Sync router-ca to the console namespace #328

Bug 1764704: Sync router-ca to the console namespace #328

Uh oh!

Conversation

jhadvig commented Oct 31, 2019

Uh oh!

openshift-ci-robot commented Oct 31, 2019

Uh oh!

spadgett commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen Nov 5, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhadvig commented Oct 31, 2019

Uh oh!

jhadvig commented Nov 4, 2019

Uh oh!

jhadvig commented Nov 4, 2019

Uh oh!

jhadvig commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhadvig commented Nov 5, 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!

jhadvig commented Nov 6, 2019

Uh oh!

jhadvig commented Nov 6, 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!

benjaminapetersen Nov 6, 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!

jhadvig left a comment

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!

jhadvig commented Nov 7, 2019

Uh oh!

benjaminapetersen Nov 5, 2019 •

edited

Loading

jhadvig commented Nov 5, 2019 •

edited

Loading

benjaminapetersen Nov 6, 2019 •

edited

Loading