Add a render command to facilitate install-time customizations #309

ironcladlou · 2019-10-10T22:00:32Z

Introduce a render command which emits the most essential manifests required
to customize ingress during installation. For now, the command emits the
operator namespace and CRD, which should be enough to allow the creation of
ingresscontroller.operator.openshift.io resources before the operator
manifests are created by the CVO.

This enables the installer to create a new ingresscontroller before the
ingress operator starts, effectively allowing users to bring their own complete
default ingresscontroller resource.

ironcladlou · 2019-10-17T14:47:12Z

Dockerfile

 COPY --from=builder /ingress-operator/ingress-operator /usr/bin/
 COPY manifests /manifests
-RUN useradd ingress-operator
-USER ingress-operator


This new user should be unnecessary, and by using, the installer (as an image consumer) must override the user when doing things like running render with a volume mounted output directory (because ingress-operator user doesn't have permissions to write to the volume mount point).

Although I can work around it in the installer, if we have no reason to run as a new user, removing the useradd makes things more consistent with the other images used for the same purpose.

cmd/ingress-operator/render.go

cmd/ingress-operator/main.go

cmd/ingress-operator/render.go

Miciah · 2019-10-17T16:23:58Z

cmd/ingress-operator/render.go

+func render(dir string, prefix string) error {
+	files := []string{
+		manifests.CustomResourceDefinitionManifest,
+		manifests.NamespaceManifest,


Could we also write out a default ingresscontroller resource so the user does not need to write that from scratch?

Can we have a separate followup discussion about the default handling in that regard? It's not required for this part of the implementation and could have negative consequences to consider.

cmd/ingress-operator/render.go

Miciah · 2019-10-17T16:31:40Z

hack/update-generated-bindata.sh

 # Using "-modtime 1" to make generate target deterministic. It sets all file
 # time stamps to unix timestamp 1
-GO111MODULE=on GOFLAGS=-mod=vendor go run github.com/kevinburke/go-bindata/go-bindata -mode 420 -modtime 1 -pkg manifests -o ${OUTDIR}/pkg/manifests/bindata.go assets/...
+GO111MODULE=on GOFLAGS=-mod=vendor go run github.com/kevinburke/go-bindata/go-bindata -mode 420 -modtime 1 -pkg manifests -o ${OUTDIR}/pkg/manifests/bindata.go assets/... manifests/...


Instead of manifests/..., can we build in only the manifests we need?

go-bindata takes a directory list, not sure there's any real harm here other than binary size inflation?

If specifying files individually is not an option, then another option would be to copy the desired files to a temporary directory and specify that directory instead. But you're right, this isn't a major problem, just something that jumped out at me when I glanced at the bindata.

ironcladlou · 2019-10-17T20:35:22Z

@Miciah latest feedback addressed, PTAL.

Introduce a `render` command which emits the most essential manifests required to customize ingress during installation. For now, the command emits the operator namespace and CRD, which should be enough to allow the creation of `ingresscontroller.operator.openshift.io` resources before the operator manifests are created by the CVO. This enables the installer to create a new `ingresscontroller` before the ingress operator starts, effectively allowing users to bring their own complete default `ingresscontroller` resource.

ironcladlou · 2019-10-18T00:00:21Z

/retest

ironcladlou · 2019-10-18T00:11:26Z

@Miciah tested with openshift/installer#2523, I think this works.

Miciah · 2019-10-18T00:47:33Z

cmd/ingress-operator/render.go

+			fmt.Println("---")
+			fmt.Printf("# file: %s\n", filepath.Base(file))
+			fmt.Println(manifests.MustAssetString(file))
+			fmt.Println("...")


I tried the render command and got the following output (showing the first few lines):

--- # file: 00-custom-resource-definition.yaml --- apiVersion: apiextensions.k8s.io/v1beta1 kind: CustomResourceDefinition metadata: creationTimestamp: null name: ingresscontrollers.operator.openshift.io

On investigation, I realized manifests/00-custom-resource-definition.yaml already has a --- marker. We need to delete either the print statements for the markers or the --- marker in the file.

Of course if this loop does not emit markers, that means we also need to add a --- marker to manifests/00-namespace.yaml so that they will be separated when the render command prints them. I think best would be to remove the marker from manifests/00-custom-resource-definition.yaml and keep the print statements as they are.

We've spent way more time on this particular code branch than is reasonable — it's not actually used for anything but debugging. I went ahead and deleted the entire functionality. Can this now merge?

knobunc · 2019-10-18T12:10:45Z

/lgtm

openshift-ci-robot · 2019-10-18T12:11:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, knobunc

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~~ [ironcladlou,knobunc]

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 requested review from frobware and knobunc October 10, 2019 22:00

ironcladlou force-pushed the render-command branch from 16b0e5b to 4f32269 Compare October 17, 2019 14:44

ironcladlou commented Oct 17, 2019

View reviewed changes

ironcladlou changed the title ~~WIP: Add a render command to facilitate install-time customizations~~ Add a render command to facilitate install-time customizations Oct 17, 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 17, 2019

ironcladlou mentioned this pull request Oct 17, 2019

Enable user defined default ingresscontroller openshift/installer#2523

Merged

2 tasks

Miciah reviewed Oct 17, 2019

View reviewed changes

ironcladlou force-pushed the render-command branch from 4f32269 to ec91087 Compare October 17, 2019 20:26

ironcladlou force-pushed the render-command branch from 557f522 to cfcf833 Compare October 17, 2019 21:08

ironcladlou force-pushed the render-command branch from cfcf833 to c77d6aa Compare October 17, 2019 21:12

render: use more restrictive permissions

28af6c3

Miciah reviewed Oct 18, 2019

View reviewed changes

render: remove unnecessary stdout support

c4387c3

openshift-ci-robot assigned knobunc Oct 18, 2019

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

openshift-merge-robot merged commit 84bfac2 into openshift:master Oct 18, 2019

Miciah mentioned this pull request Apr 27, 2023

NE-1269: Replace bindata using embed #905

Merged

Add a render command to facilitate install-time customizations #309

Add a render command to facilitate install-time customizations #309

Uh oh!

Conversation

ironcladlou commented Oct 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

ironcladlou commented Oct 17, 2019

Uh oh!

ironcladlou commented Oct 18, 2019

Uh oh!

ironcladlou commented Oct 18, 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!

knobunc commented Oct 18, 2019

Uh oh!

openshift-ci-robot commented Oct 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants