(cleanup): Reconfigure the manifests & manifest generation by everettraven · Pull Request #16 · operator-framework/catalogd

everettraven · 2023-03-29T19:24:39Z

Description

Update the config/ directory to be more similar to the standard put in place by the Operator-SDK and utilize kustomize for building the manifests for deploying catalogd on cluster
Update the make kustomize target to install Kustomize v5.0.1 under the bin/ directory
Update RBAC markers in controllers so that the proper permissions are generated under the config/rbac/role.yaml file

Motivation

Follow existing standards for operators/controllers to utilize kustomize for building manifests and the associated config/ directory structure
Enable manifest generation logic to actually populate changes in the correct locations
fixes Refactor manifests and manifest generation #10

to enable use of kustomize Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven · 2023-03-29T19:47:37Z

/cc @anik120

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven · 2023-03-29T19:56:26Z

This PR could very well contain some manifest files that we don't care about (and aren't used by default), like webhook configurations, but I kept them in since I felt there could be an argument for keeping them for the future. If we feel it is better to remove them completely I am happy to do so.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

joelanford · 2023-03-30T20:41:19Z

like webhook configurations

controller-gen generates those if we need them. So they're probably safe to delete if they aren't already being generated.

joelanford · 2023-03-30T20:42:15Z

+    name: catalogd
+    namespace: system
+  versionPriority: 10
+  caBundle: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUR6RENDQXJTZ0F3SUJBZ0lVT0t0RXJnS3pGMTl4ZXh6cUVXbExxcGlmN2lnd0RRWUpLb1pJaHZjTkFRRUwKQlFBd2RERUxNQWtHQTFVRUJoTUNkVzR4Q3pBSkJnTlZCQWdNQW5OME1Rb3dDQVlEVlFRSERBRnNNUW93Q0FZRApWUVFLREFGdk1Rc3dDUVlEVlFRTERBSnZkVEV6TURFR0ExVUVBd3dxY25WcmNHRnJMWEJoWTJ0aFoyVnpaWEoyClpYSXRZMlZ5ZEdsbWFXTmhkR1V0WVhWMGFHOXlhWFI1TUI0WERUSXlNVEF3TnpFNU1Ua3dNMW9YRFRJek1UQXcKTnpFNU1Ua3dNMW93ZERFTE1Ba0dBMVVFQmhNQ2RXNHhDekFKQmdOVkJBZ01Bbk4wTVFvd0NBWURWUVFIREFGcwpNUW93Q0FZRFZRUUtEQUZ2TVFzd0NRWURWUVFMREFKdmRURXpNREVHQTFVRUF3d3FjblZyY0dGckxYQmhZMnRoCloyVnpaWEoyWlhJdFkyVnlkR2xtYVdOaGRHVXRZWFYwYUc5eWFYUjVNSUlCSWpBTkJna3Foa2lHOXcwQkFRRUYKQUFPQ0FROEFNSUlCQ2dLQ0FRRUFxZlRZSkpURmwwZ2F0RVQvUDlFTlhsWjRqYUdLSklMSFZVb2VjankwRmxVRgpDVnYvdTYrMzNwcnFBNjBiWmpzenFzN1FmTmZRTmd3azllTE03TDRiNVkyZnc1SitPTEhtSlc3S0FHVTcxa2JsCnBWaXZ6MFBjbUdNZlFFQU1kYTB0T2xZRjQyNEIvZTE2TEwyNDBQNzdxOFprQzEvU3N1NVlpMkVvRzlOQ3c2ZUgKalhRT2dXcFd0OE1VT1E1ZmZYRUdBcTlwcWJNeGN0UUxDS3JHVlVHVGdmaHQvb2xKWlcxbTJ6QVhEVDRQa0ZYRQo2MnNNVFBmamFFNWtmeUhxKy9nRXlUU0FPbW1LVzdJOXBNOHdZcVJ1ZHB4alZrWm5UQWlMVkNDckk2SGhaVVlECmpURThzTlhld2J2WU5rdnlVUnN4QjdEWHdOcE1kN2d3SzhiQ1BqTHBiUUlEQVFBQm8xWXdWREFkQmdOVkhRNEUKRmdRVVZjeXQ1L2F2RVc2SXJiOC9CKzkvYjhDZnFEb3dId1lEVlIwakJCZ3dGb0FVVmN5dDUvYXZFVzZJcmI4LwpCKzkvYjhDZnFEb3dFZ1lEVlIwVEFRSC9CQWd3QmdFQi93SUJBVEFOQmdrcWhraUc5dzBCQVFzRkFBT0NBUUVBClhFbU15L0tnMUZnVFdnL244ajcrak5aSFhHcWs2ZnZQMzNVbTZMaTdVL0JSOWZyQ05rQk9GSU9ZV21BWUlBQksKbEhSMStDQkJ5NncwSjFVMjZ4VEVaTnVPQVhOSHdPQVlNaURia2xRamlIMEdtcWpNUGxDQlhoUGpHTEhBK2h0dwpXd3JTL2h1bkJlU2RmbERtYThUbm9OMDUzTHZBanFNN3cwU3JOaWw3ZzhtM2k2djFmdGxNNkNJU1N0M05wRTZNCllnU2pMRTRLNmxCVFI0VU81K0hsanBVK2JWUCtHcDRMWHBGaEZJMU9jZjAxTC9iRGF3elhmYXNoNm8yVS9lOE8KOUZTa1Q2TzFlVW5COFRyRVMrZDlaKzBQS2dqZXAzU0NmV1N5WDFSdzZKZGM3SmNYY0I0ZmdLQ3FMNWtIbHNXUgpNcXErQ3UycVlZbnNiclo4ZVZaUGtRPT0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo="


Where does this CA bundle come from? Should we use cert-manager to generate and inject this?

Should be resolved in 7a347fb

joelanford · 2023-03-30T20:43:40Z

+    spec:
+      serviceAccountName: apiserver
+      containers:
+      - name: apiserver


For follow-up: we should setup the security context to make sure this can run under restricted PSA.

Yea I just remembered, there's already some pending work to make this compatible with restricted PSA : ref
I'll create an issue so that we can track this.

joelanford · 2023-03-30T20:44:24Z

+kind: Kustomization
+images:
+- name: apiserver
+  newName: docker.io/anik120/rukpak-packageserver


I assume this needs to change in some way?

I'm guessing this PR was opened before the image hosting was changed.

should be fixed in 7a347fb

joelanford · 2023-03-30T20:45:51Z

+#- patches/webhook_in_bundlemetadata.yaml
+#- patches/webhook_in_packages.yaml


Since these are served by an apiserver, I assume these comments are fully obsolete, and they'll never make sense in this context?

removed in 7a347fb

joelanford · 2023-03-30T20:47:43Z

+  - kind: CustomResourceDefinition
+    version: v1
+    group: apiextensions.k8s.io
+    path: spec/conversion/webhook/clientConfig/service/name


Is this present by default in kubebuilder/sdk scaffolding? I assume we're not actually using conversion webhooks, so my instinct would be to remove. But if this is there, but inert, by default I guess no harm in keeping it.

It was included by default, but seemed to be inert. I removed it anyways in 7a347fb

grokspawn · 2023-03-30T20:47:52Z

+  - list
+  - watch
+- apiGroups:
+  - core.catalogd.io


Is this domain related to this discussion?
Did anyone register that domain? Do we want to start using it until that has been done?

My vote is that for now we go with catalogd.operatorframework.io. And we continue the debate. This could be done in a follow-up as far as I'm concerned though.

I don't know if we reached any concrete conclusion from those discussions. But does feel like follow up PR concern though.

Since I have to make a few changes anyways I can update the domain as well

Changed the domain in 7a347fb

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

anik120

/lgtm

joelanford · 2023-03-31T17:35:59Z

+images:
+- name: apiserver
+  newName: quay.io/operator-framework/catalogd-server
+  newTag: latest


Based on discussion in the other PR?

Suggested change

newTag: latest

newTag: devel

I was going to wait and make the necessary changes based on whichever PR gets in first

joelanford · 2023-03-31T17:37:11Z

@@ -0,0 +1,98 @@
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition


Wait, I'm confused. If the Package and BundleMetadata APIs are CRDs what are we getting out of the apiservice and our separate etcd? Seems like I need to go read code.

My understanding is we are offloading the storage of those resources to the custom apiservice to handle and it was storing them in the separate etcd instance. That being said, I have extremely limited knowledge on custom apiservices and have no idea if this one is working as expected.

cc @anik120

If that were true, there would be no CRDs for Package and BundleMetadata.

…framework#16) * refactor manifests to enable use of kustomize Signed-off-by: Bryce Palmer <bpalmer@redhat.com> * fix incorrect tag Signed-off-by: Bryce Palmer <bpalmer@redhat.com> * revert storage-class Signed-off-by: Bryce Palmer <bpalmer@redhat.com> * address review comments Signed-off-by: Bryce Palmer <bpalmer@redhat.com> --------- Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

anik120

Some post merge thoughts.

anik120 · 2023-04-03T17:49:04Z


-KUSTOMIZE = $(shell pwd)/bin/kustomize
+## Tool Binaries
+KUSTOMIZE ?= $(LOCALBIN)/kustomize


The deploy target is broken
(KUSTOMIZE) edit set image controller=${IMG}

That should be CONTROLLER_IMG

anik120 · 2023-04-03T17:49:58Z

+        # seccompProfile:
+        #   type: RuntimeDefault


This needs to be uncommented

anik120 · 2023-04-03T17:50:31Z

+      # TODO(user): Uncomment the following code to configure the nodeAffinity expression
+      # according to the platforms which are supported by your solution. 
+      # It is considered best practice to support multiple architectures. You can
+      # build your manager image using the makefile target docker-buildx.
+      # affinity:
+      #   nodeAffinity:
+      #     requiredDuringSchedulingIgnoredDuringExecution:
+      #       nodeSelectorTerms:
+      #         - matchExpressions:
+      #           - key: kubernetes.io/arch
+      #             operator: In
+      #             values:
+      #               - amd64
+      #               - arm64
+      #               - ppc64le
+      #               - s390x
+      #           - key: kubernetes.io/os
+      #             operator: In
+      #             values:
+      #               - linux


We can get rid of these lines instead of keeping them commented.

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

refactor manifests

bc08fcb

to enable use of kustomize Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven force-pushed the cleanup/manifests branch from 4c68d78 to bc08fcb Compare March 29, 2023 19:30

openshift-ci Bot requested a review from anik120 March 29, 2023 19:47

fix incorrect tag

879e673

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven commented Mar 30, 2023

View reviewed changes

Comment thread config/etcd/etcd.yaml Outdated

revert storage-class

630f2f7

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

joelanford reviewed Mar 30, 2023

View reviewed changes

grokspawn reviewed Mar 30, 2023

View reviewed changes

address review comments

7a347fb

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

anik120 approved these changes Mar 31, 2023

View reviewed changes

openshift-ci Bot assigned anik120 Mar 31, 2023

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2023

joelanford reviewed Mar 31, 2023

View reviewed changes

grokspawn merged commit 72bf6c0 into operator-framework:main Mar 31, 2023

anik120 reviewed Apr 3, 2023

View reviewed changes

everettraven mentioned this pull request Apr 5, 2023

(chore): manifests cleanup #24

Merged

everettraven added a commit to everettraven/catalogd that referenced this pull request Apr 7, 2023

fix manifests as follow up to operator-framework#16

09a572a

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven added a commit to everettraven/catalogd that referenced this pull request Apr 10, 2023

fix manifests as follow up to operator-framework#16

5c3dd08

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

		#- patches/webhook_in_bundlemetadata.yaml
		#- patches/webhook_in_packages.yaml

Conversation

everettraven commented Mar 29, 2023

Uh oh!

everettraven commented Mar 29, 2023

Uh oh!

everettraven commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

joelanford commented Mar 30, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anik120 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!

Choose a reason for hiding this comment

Uh oh!

anik120 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

everettraven commented Mar 29, 2023 •

edited

Loading