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

refactor(bundle): kustomize changes #1375

Merged
merged 7 commits into from
Jun 1, 2021
Merged

refactor(bundle): kustomize changes #1375

merged 7 commits into from
Jun 1, 2021

Conversation

SteveMattar
Copy link

@SteveMattar SteveMattar commented May 23, 2021 •
edited by mangelajo
Loading

This is relevant only for deploying submariner using the bundle.

When working with subctl the manifests definition and objects creation are handled automatically, but when it comes to the operator bundle we have to make sure those resources are aligned.

  • Add the missing broker manifests.
  • Update the CSV description and samples.

Note: The submariner-k8s-broker namespace cannot be created during the operator bundle installation.
The user has to make sure the namespace exists before installing the bundle.
Same for the SCC on Openshift.

Backported in #1398

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1375/SteveMattar/kustomize

@SteveMattar SteveMattar added the backport This change requires a backport to eligible release branches label May 23, 2021
@SteveMattar SteveMattar mentioned this pull request May 24, 2021
Open
@SteveMattar SteveMattar requested a review from nyechiel May 24, 2021 08:46
mangelajo
mangelajo reviewed May 24, 2021
View reviewed changes
Copy link
Contributor

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

See my comments, let's setup a meeting tomorrow with @skitt

config/samples/submariner_v1alpha1_servicediscovery.yaml Show resolved Hide resolved
config/samples/submariner_v1alpha1_servicediscovery.yaml Outdated Show resolved Hide resolved
config/samples/submariner_v1alpha1_submariner.yaml Outdated Show resolved Hide resolved
config/samples/submariner_v1alpha1_submariner.yaml Outdated Show resolved Hide resolved
config/rbac/lighthouse-agent/cluster_role_binding.yaml Show resolved Hide resolved
config/broker/kustomization.yaml Outdated Show resolved Hide resolved
config/manifests/bases/submariner.clusterserviceversion.yaml Outdated Show resolved Hide resolved
@SteveMattar SteveMattar requested review from mangelajo and nyechiel May 26, 2021 06:53
@nyechiel
Copy link
Member

@skitt @mangelajo can you please review?

@nyechiel nyechiel added this to the 0.10-m2 milestone May 26, 2021
mangelajo
mangelajo suggested changes May 27, 2021
View reviewed changes
Copy link
Contributor

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

I think I would like to keep a pointer still to subctl, before anybody ventures into standalone operator management they should know that subctl is available and that we prefer that solution instead.

bundle/manifests/submariner.clusterserviceversion.yaml
Comment on lines +106 to +114
If running on an OpenShift cluster, perform the following steps to bind the Submariner service accounts to the **privileged** SCC.

```shell
oc adm policy add-scc-to-user privileged system:serviceaccount:submariner-operator:submariner-gateway
oc adm policy add-scc-to-user privileged system:serviceaccount:submariner-operator:submariner-routeagent
oc adm policy add-scc-to-user privileged system:serviceaccount:submariner-operator:submariner-globalnet
oc adm policy add-scc-to-user privileged system:serviceaccount:submariner-operator:submariner-lighthouse-coredns
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move these under the suggested "#### Deployment with the operator (without subctl)" ? as those wouldn't apply to subctl?

bundle/manifests/submariner.clusterserviceversion.yaml
echo export PATH=\$PATH:~/.local/bin >> ~/.profile
```
Deploy the broker:
On the broker cluster:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
On the broker cluster:
#### Deployment with subctl
For deploying the submariner operator and submariner we recommend the use of subctl. Please have a look at our [quickstarts](https://submariner.io/getting-started/quickstart/) for more details.
#### Deployment with the operator (without subctl)
On the broker cluster:

SteveMattar
SteveMattar commented May 27, 2021
View reviewed changes
bundle/manifests/submariner.clusterserviceversion.yaml
@@ -103,61 +121,71 @@ spec:
For complete information about subctl, please refer to [this page](https://submariner.io/operations/deployment/subctl).
In addition to Operator and subctl, Submariner also provides [Helm Charts](https://submariner.io/operations/deployment/helm).

Copy link
Author

@SteveMattar SteveMattar May 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
### Deployment
Submariner provides an [Operator](https://github.com/submariner-io/submariner-operator) for easy API-based
installation and management.
A command line utility, [subctl](https://github.com/submariner-io/submariner-operator/releases), wraps the
Operator to aid users with manual deployments and easy experimentation.
subctl greatly simplifies the deployment of Submariner, and is therefore the recommended deployment method.
For complete information about subctl, please refer to [this page](https://submariner.io/operations/deployment/subctl).
In addition to Operator and subctl, Submariner also provides [Helm Charts](https://submariner.io/operations/deployment/helm).

@mangelajo subctl is mentioned here. There is a reference with a link for all sort of deployments we provide: subctl, helm,...
I think this specific description should focus on the bundle installation only and if the user wants to use a different method he should follow our documentation.
Does it make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to highlight that they should use subctl as first option, unless they have a good reason to use the operator standalone. As it will save many general issues.

Do you think there is a better way to highlight it?

Copy link
Author

Choose a reason for hiding this comment

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

In our current description we recommend for subctl:

subctl greatly simplifies the deployment of Submariner, and is therefore the recommended deployment method

We can maybe rephrase the sentence to make the message even more clear.

Copy link
Author

Choose a reason for hiding this comment

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

@mangelajo

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label May 27, 2021
@dfarrell07 dfarrell07 requested a review from mangelajo June 1, 2021 14:27
@mangelajo mangelajo enabled auto-merge (rebase) June 1, 2021 14:52
Steve Mattar added 2 commits June 1, 2021 20:38
chore(bundle): update the samples
693a00f 
Signed-off-by: Steve Mattar <smattar@redhat.com>
refactor(bundle): change the way we override the image and repo
46af388 
Signed-off-by: Steve Mattar <smattar@redhat.com>
Steve Mattar added 5 commits June 1, 2021 20:38
refactor(bundle): change the way we set the subject sa namespace
d213a62 
Signed-off-by: Steve Mattar <smattar@redhat.com>
feat(bundle): add the broker manifests for external use
f157cfd 
Signed-off-by: Steve Mattar <smattar@redhat.com>
docs(bundle): update the csv description
161b483 
Signed-off-by: Steve Mattar <smattar@redhat.com>
fix(bundle): disable non-supported install-modes
a9c7b1e 
Signed-off-by: Steve Mattar <smattar@redhat.com>
chore: re-generate bundle 0.9.0
28232a8 
Signed-off-by: Steve Mattar <smattar@redhat.com>
@mangelajo mangelajo merged commit c8d81a0 into submariner-io:devel Jun 1, 2021
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1375/SteveMattar/kustomize]

@SteveMattar SteveMattar deleted the kustomize branch June 2, 2021 07:04
@mangelajo mangelajo mentioned this pull request Jun 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dfarrell07 dfarrell07 dfarrell07 approved these changes

@mangelajo mangelajo mangelajo approved these changes

@skitt skitt skitt approved these changes

@mkolesnik mkolesnik Awaiting requested review from mkolesnik mkolesnik is a code owner

@Oats87 Oats87 Awaiting requested review from Oats87 Oats87 is a code owner

@tpantelis tpantelis Awaiting requested review from tpantelis tpantelis is a code owner

@nyechiel nyechiel Awaiting requested review from nyechiel

Assignees

@SteveMattar SteveMattar

Labels
backport This change requires a backport to eligible release branches backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Milestone
0.10-m2
Development

Successfully merging this pull request may close these issues.
6 participants
@SteveMattar @submariner-bot @nyechiel @dfarrell07 @mangelajo @skitt