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

Deploy operands in the operator's namespace #134

Merged
merged 1 commit into from
May 6, 2022
Merged

Deploy operands in the operator's namespace #134

merged 1 commit into from
May 6, 2022

Conversation

alebedev87
Copy link
Contributor

@alebedev87 alebedev87 commented Apr 11, 2022 •
edited

Operand can be deployed in the operator namespace:

$ oc -n external-dns-operator get pods
NAME                                     READY   STATUS    RESTARTS   AGE
external-dns-operator-6644944b64-2txst   2/2     Running   0          2m12s

$ oc get ns external-dns
Error from server (NotFound): namespaces "external-dns" not found

$ oc apply -f infoblox-cr.yaml 
externaldns.externaldns.olm.openshift.io/sample-infoblox created

$ oc -n external-dns-operator get pods
NAME                                            READY   STATUS    RESTARTS   AGE
external-dns-operator-6644944b64-2txst          2/2     Running   0          3m40s
external-dns-sample-infoblox-57f97c7849-q2rp9   1/1     Running   0          59s

$ oc get externaldns 
NAME              AGE
sample-infoblox   68s

$ oc -n external-dns-operator logs external-dns-sample-infoblox-57f97c7849-q2rp9
...
time="2022-04-11T20:55:57Z" level=info msg="Created OpenShift client https://10.217.4.1:443"
time="2022-04-11T20:56:08Z" level=debug msg="fetched 0 records from infoblox"
...
time="2022-04-11T20:56:08Z" level=info msg="All records are already up to date"

Removal of the CR and the operator namespace works with no problem:

$ oc delete externaldns sample-infoblox
externaldns.externaldns.olm.openshift.io "sample-infoblox" deleted

$ oc -n external-dns-operator get pods
NAME                                     READY   STATUS    RESTARTS   AGE
external-dns-operator-6644944b64-2txst   2/2     Running   0          8m24s

$ oc delete ns external-dns-operator
namespace "external-dns-operator" deleted

$ oc get ns external-dns-operator
Error from server (NotFound): namespaces "external-dns-operator" not found

Even if there is a CR and an operand the namespace can be removed - no problem:

$ oc -n external-dns-operator get pods
NAME                                            READY   STATUS    RESTARTS   AGE
external-dns-operator-6cf8d8576d-r9drk          2/2     Running   0          34s
external-dns-sample-infoblox-57f97c7849-rztbw   1/1     Running   0          11s

$ oc get externaldns
NAME              AGE
sample-infoblox   18s

$ oc delete ns external-dns-operator
namespace "external-dns-operator" deleted

$ oc get externaldns
NAME              AGE
sample-infoblox   41s

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2022
@openshift-ci openshift-ci bot requested review from arjunrn and candita April 11, 2022 21:12
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 11, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2022
@alebedev87 alebedev87 changed the title [wip] operator and operand in the single namespace Deploy operands in the operator's namespace May 2, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 2, 2022
@alebedev87 alebedev87 force-pushed the single-namespace branch 6 times, most recently from 1746eb1 to 23e0f7f Compare May 2, 2022 21:14
@alebedev87
Copy link
Contributor Author

/assign @quarterpin
I tested this change manually (see the description), the e2e tests are passed too.
However this is a major design change and needs to be tested thoroughly.

@alebedev87
Copy link
Contributor Author

/hold
migration doc is to be added, no need to spawn test clusters

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2022
@alebedev87
Copy link
Contributor Author

/unhold
as it doesn't stop the tests :(

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2022
@alebedev87
Copy link
Contributor Author

/assign @sherine-k

@alebedev87
Copy link
Contributor Author

/retest

sherine-k
sherine-k reviewed May 4, 2022
View reviewed changes
Copy link
Contributor

@sherine-k sherine-k left a comment

Choose a reason for hiding this comment

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

I like this implementation as it preserves the possibility to go back to separate NSs in the future if needed. Kudos!
Couple of nitpicks below

bundle/manifests/external-dns_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml
@@ -11,3 +11,6 @@ subjects:
- kind: Group
name: system:serviceaccounts:external-dns
namespace: external-dns
Copy link
Contributor

Choose a reason for hiding this comment

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

serviceAccount external-dns still exists (used by operands) but namespace would become external-dns-operator, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The service accounts for the operand will be created in external-dns-operator namespace now. If the operator is installed on a new cluster external-dns namespace won't be there, same about service accounts in it. If the operator is upgraded on a cluster which already had it, then yes the service accounts will still be there until the migration procedure is followed.
This binding is not strictly necessary anymore, I kept it just in case, the operator can be used in both modes with it.

bundle/manifests/external-dns_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml
@@ -11,3 +11,6 @@ subjects:
- kind: Group
name: system:serviceaccounts:external-dns
namespace: external-dns
- kind: Group
name: system:serviceaccounts:external-dns-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a doubt as to why the operator serviceAccount also needs this clusterrolebinding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the operator doesn't need it. It's just that there is no way to exclude 1 service account from the namespace group.
Unfortunately there is not much choice to work this around without giving the operator the update permission on clusterrolebindings resource.

config/rbac/operand_rolebinding.yaml
@@ -11,3 +11,6 @@ subjects:
- kind: Group
name: system:serviceaccounts:external-dns
namespace: external-dns
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to comment on bundle/manifests/external-dns_rbac.authorization.k8s.io_v1_clusterrolebinding.yaml

@quarterpin
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 4, 2022
@sherine-k
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2022
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2022
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 6, 2022
@sherine-k
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 6, 2022

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@alebedev87
Copy link
Contributor Author

/label docs-approved
/label px-approved

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels May 6, 2022
@openshift-merge-robot openshift-merge-robot merged commit 8e86b1e into openshift:main May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherine-k sherine-k sherine-k left review comments

@arjunrn arjunrn Awaiting requested review from arjunrn

@candita candita Awaiting requested review from candita

Assignees

@sherine-k sherine-k

@quarterpin quarterpin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alebedev87 @quarterpin @sherine-k @openshift-bot @openshift-merge-robot