Signed-off-by: Yashvardhan Kukreja [email protected]

Why not directly configure the existing metrics server to be TLS-terminated? Because the existing metrics server isn't setup by us but by controller-runtime, behind the scenes, while the manager gets bootstrapped and it is currently hardcoded to be http-exposed. Hence, this PR adds a relay server (a sidecar to our metrics-server) which acts as the TLS-terminated front-end to our clients (Prometheus scraper, ServiceMonitors) masking the insecure http-exposed server.

The addon-operator-metrics.addon-operator.svc (service) exposes the metrics at /metrics at the following hosts: https://addon-operator-metrics.addon-operator.svc:8443/metrics and http://addon-operator-metrics.addon-operator.svc:8080/metrics.

Description

With https://github.com/openshift/addon-operator/pull/32 AllNamespaces and OwnNamespace install types have gained OLM specific parameters. This PR prefixes those options and config stanzas with OLM to reflect that.

Signed-off-by: Mayank Shah [email protected]

Description

To get started with this operator, I have reviewed and did some minor refactors. I have not reviewed everything, as I feel this is already large enough for a PR and to get us started.

Out-of-scope

• Test cases
• Stopped at Phase 4 of the Reconcile loop

Work done

• make test-unit + make lint + pre-commit-hooks work (counting on CI to run integration tests)
• I left a few // TODO ... comments directly inline. This is not ideal so I tried to avoid it as much as possible.
• renamed k8sApiErrors to apierrors to mimic kubebuilder/controller-runtime
• added probe endpoints to the manager (healthz + readyz)
• added minimal error logging inside of Reconcile
• condensed few error handling statements like this one:

if err := myFunc(...); err != nil { 
    return err 
}

Comments / Questions

• Migrate testing to Ginkgo/Gomega? controller-runtime + kubebuilder have settled on that. Great example here microsoft/azure-databricks-operator
• We are missing some info/debug logging inside the controller
• How do we want to instrument the controller to play nice with OSD? Prometheus? Do we have an example?
• We should avoid passing the logger around, it is internal to Reconciler (I saw a comment acknowledging this). We can just instantiate a new one:

log := r.Log.WithName("mrbean")

• I feel we should try to stick to kubebuilder framework as much as we can. Under heavy development. Lot's of experience. If they think something should be done in a certain way, I would tend to agree and blindly follow. Aren't we all sheeps? :sheep:
• In another PR, bump Makefile dependencies to latest + retest? Can we enable this via bot PR's?
• parseAddonInstallConfig is what I would call 'defensive' programming, but the comment acknowledges this and suggests migrating to Defaulting/Validating webhooks. Also, returning more than two values is always a red flag from my perspective, maybe we can refactor this?

Signed-off-by: Ankit Kurmi [email protected]

Description

Updating preferredDuringSchedulingIgnoredDuringExecution to requiredDuringSchedulingIgnoredDuringExecution.

This PR introduces the testify/suite library for integration tests. This mechanism allows us to group theses tests into a common integrationTestSuite and add proper setup and teardown for it.

• Use SetupSuite and TeardownSuite methods for the test suite
• De-duplicate test code and combine catalog_source_test.go, namespaces_test.go, and subscription_test.go into a single test with each of them as a subtest in addon_test.go
• Minor refactors within the integration/ package to adapt to the suite library
• Move Addon object declarations into re-usable fixtures in fixtures_test.go, thereby removing redundant Addon object initializations

Removal of parallel test execution

This PR also removes parallel execution of tests becausetestify/suite has poor support for parallel test execution - https://github.com/stretchr/testify/issues/187 . Running tests within a test suite in parallel causes the following problems:

• Raciness - test suite is torn down while some tests are still running
• Pod logs printing happens out-of-order

Disable for cache for CSV resource

Add OnlyMetadata option to ClusterServiceVersion watch setup

Replace the use of operatorsv1alpha1.ClusterServiceVersion type with unstructured.Unstructured in the get CSV operation. This ensures that Get operations on CSV resource are not cached as unstructured.Unstructured based client operations are not cached by default in the manager default client.

Ref: https://github.com/kubernetes-sigs/controller-runtime/blob/cd0058ad295c268da1e7233e609a9a18dd60b5f6/pkg/client/split.go#L115

Upgrade go version, controller-runtime and k8s libraries

Signed-off-by: Ankit Kurmi [email protected]

Description

1. Added Prerequisites, Unit test, Releasing and Deployment into the Index section.
2. Added Unit test section into the README file.

Signed-off-by: Ankit Kurmi [email protected]

Description

Changing variable name for better code readability for webhook_test.go under internal/webhooks and to make it consistent.

Signed-off-by: Ankit Kurmi [email protected]

Description

Adding unit test for HasAdoptAllStrategy function under internal/controllers/addon/utils.go which checks if an addon has ResourceAdoptionAdoptAll strategy.

This is the final refactor left for https://issues.redhat.com/browse/MTSRE-298

This PR refactors the Addon Reconciler code to have consistent exit handling and a single place where the Addon Status is updated via the kube-api.

Signed-off-by: Mayank Shah [email protected]

Adding a way to track all addons being reconciled by the addon-operator and expose metrics with prometheus.

What it looks like at the end of running tests make test-integration-short (addons go from Pending to Ready to Terminating):

Update what metrics output looks like: https://pastebin.com/2N5gHNRc

TODO

• [x] The Terminating state is final, addon never get updated afterwards. Meaning the addon_operator_addons_phase_total{phase="Terminating"} 6 will effectively be a counter never going down. Is that OK?
• [x] Write integration test and validate localhost:8080/metrics exposes this metric
• [x] I noticed that we have a helper method to update all phases except PhasePending. Is it worth refactoring this?

Make addon reconciler watch operatorv1/Operator instead of operatorv1alpha1/ClusterServiceVersion

Replace CSV based addon install succes validation with operator resource based install success validation

Signed-off-by: Nikhil Thomas [email protected]

Note: the current code might be deleting the csvKey to AddonKey mappings of addons other than the one being reconciled at a given moment.

This patch ensures that only the mappings of the current Addon, which are obsolete are removed

Signed-off-by: Nikhil Thomas [email protected]