Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

controller-manager: Configurable image repos + imagePullSecrets #170

Merged
merged 3 commits into from
Mar 21, 2022
Merged

controller-manager: Configurable image repos + imagePullSecrets #170

merged 3 commits into from
Mar 21, 2022

Conversation

harisokanovic
Copy link
Contributor

@harisokanovic harisokanovic commented Mar 10, 2022 •
edited
Loading

Add config option for kube-rbac-proxy image instead of a hard
coded name, so that the controller-manager can be depoloyed
from a 3rd party registry (e.g. air gapped cluster, caching proxy).

Add image.repo config options to allow specifying a different
repository without changing name or version, for convenience.

Add imagePullSecrets so that images can be pulled from private repos.

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

spilchen
spilchen reviewed Mar 11, 2022
View reviewed changes
Copy link
Collaborator

@spilchen spilchen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I had one suggestion and a question.

helm-charts/verticadb-operator/values.yaml Outdated Show resolved Hide resolved
helm-charts/verticadb-operator/values.yaml Show resolved Hide resolved
@spilchen
Copy link
Collaborator

Can you run this from the repo root dir?

helm unittest --helm3 --output-type JUnit helm-charts/verticadb-operator

I just noticed we no longer run this in our CI. I'll fix this separately, but it should be clean for you.

@harisokanovic
Copy link
Contributor Author 

$ helm unittest --helm3 --output-type JUnit helm-charts/verticadb-operator 

### Chart [ verticadb-operator ] helm-charts/verticadb-operator

 PASS  CA bundle tests  helm-charts/verticadb-operator/tests/caBundle_test.yaml
 PASS  test image:tag creation  helm-charts/verticadb-operator/tests/image-name-and-tag_test.yaml
 PASS  test namespace selector in the webhook configuration     helm-charts/verticadb-operator/tests/namespace-selector_test.yaml
 PASS  test that resources can be specified for the operator    helm-charts/verticadb-operator/tests/resources_test.yaml
 PASS  skip-cert-manager-CRs    helm-charts/verticadb-operator/tests/skip-cert-manager_test.yaml

Charts:      1 passed, 1 total
Test Suites: 5 passed, 5 total
Tests:       6 passed, 6 total
Snapshot:    0 passed, 0 total
Time:        10.113183ms

@harisokanovic harisokanovic force-pushed the dev/haris/rbac-proxy-config-options branch from a130aac to 2ba0008 Compare March 11, 2022 23:22
spilchen
spilchen reviewed Mar 14, 2022
View reviewed changes
config/default/manager_auth_proxy_patch.yaml Outdated
@@ -10,7 +10,7 @@ spec:
spec:
containers:
- name: kube-rbac-proxy
image: gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0
image: kube-rbac-proxy
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is messing up the olm install of the operator. Can we revert this and update the sed in create-helm-charts.sh so that it is able to match this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can. Is there any documentation for testing "olm install" on my desktop? I don't know what that is. I'm also curious why this install flow doesn't simply use helm and take the default from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can deploy with OLM via this:

DEPLOY_WITH=olm make setup-olm deploy

This is taken from DEVELOPER.md.

OLM is the deployment method for operatorhub.io. It's deployment is baked into the operator-sdk that we use, so we don't have as much control over it when compared to helm.

Copy link
Contributor Author

@harisokanovic harisokanovic Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. I pushed a new change. Please note I also added imagePullSecrets to values.yaml to access private repos; I forgot it in my initial submission.

The ... setup-olm deploy command successfully built an image, but failed push to docker.io/library/verticadb-operator-bundle due to permissions. I'm not sure how to how to change this location for a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. I will take a closer look on Monday. You can set BUNDLE_IMG environment variable to control the image name. But lets use the CI to test this out.

@harisokanovic harisokanovic force-pushed the dev/haris/rbac-proxy-config-options branch from 2ba0008 to bd54812 Compare March 18, 2022 21:14
@harisokanovic harisokanovic changed the title controller-manager: Configurable image repos controller-manager: Configurable image repos + imagePullSecrets Mar 18, 2022
spilchen
spilchen reviewed Mar 21, 2022
View reviewed changes
Copy link
Collaborator

@spilchen spilchen left a comment

Choose a reason for hiding this comment

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

Looks good. I added a unit test for the new helm parameters. We should merge with latest main to pickup the most recent changes. Did you have trouble signing the CLA?

scripts/create-helm-charts.sh Show resolved Hide resolved
harisokanovic and others added 2 commits March 21, 2022 12:04
@harisokanovic
controller-manager: Configurable image repos + imagePullSecrets
5a41c13 
Add config option for kube-rbac-proxy image instead of a hard
coded name, so that the controller-manager can be depoloyed
from a 3rd party registry (e.g. air gapped cluster, caching proxy).

Add image.repo config options to allow specifying a different
repository without changing name or version, for convenience.

Add imagePullSecrets so that images can be pulled from private repos.
@harisokanovic
Add UT for new helm parameters
a2b74f1
@harisokanovic harisokanovic force-pushed the dev/haris/rbac-proxy-config-options branch from 27b2ca3 to a2b74f1 Compare March 21, 2022 17:06
spilchen
spilchen approved these changes Mar 21, 2022
View reviewed changes
Copy link
Collaborator

@spilchen spilchen left a comment

Choose a reason for hiding this comment

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

Thanks for contributing

@spilchen spilchen merged commit f082f9b into vertica:main Mar 21, 2022
spilchen pushed a commit to spilchen/vertica-kubernetes that referenced this pull request Mar 21, 2022
@harisokanovic
controller-manager: Configurable image repos + imagePullSecrets (vert…
1ae268c 
…ica#170)

Add config option for kube-rbac-proxy image instead of a hard
coded name, so that the controller-manager can be depoloyed
from a 3rd party registry (e.g. air gapped cluster, caching proxy).

Add image.repo config options to allow specifying a different
repository without changing name or version, for convenience.

Add imagePullSecrets so that images can be pulled from private repos.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spilchen spilchen spilchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harisokanovic @CLAassistant @spilchen