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

Provision GW LB service in accordance with OCP platform #2727

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Provision GW LB service in accordance with OCP platform #2727

merged 2 commits into from
Jul 26, 2023

Conversation

yboaron
Copy link
Contributor

@yboaron yboaron commented Jul 13, 2023 •
edited
Loading

A LoadBalancer type Service relies on the cloud provider to create an external load balancer with an IP address in the relevant network space.

Different cloud providers support different Service annotations, this PR updates LB service annotations in accordance with OCP platform.

Fixes: #2603

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr2727/yboaron/roks_lb
🚀 Full E2E won't run until the "ready-to-test" label is applied. I will add it automatically once the PR has 2 approvals, or you can add it manually.

@yboaron yboaron marked this pull request as ready for review July 16, 2023 07:44
@yboaron yboaron force-pushed the roks_lb branch 2 times, most recently from b405e3d to 8f0aa3d Compare July 17, 2023 13:50
@yboaron yboaron requested a review from tpantelis July 18, 2023 12:30
@tpantelis tpantelis force-pushed the roks_lb branch 4 times, most recently from ce29883 to 5d51dea Compare July 19, 2023 01:57
@yboaron
Provision GW LB service in accordance with OCP platform
ba0c294 
A LoadBalancer type Service is a typical way to expose an
application to the internet.
A LoadBalancer type Service relies on the cloud provider to create
an external load balancer with an IP address in the relevant network space.

Different cloud providers support different Service annotations,
this PR updates LB service annotations in accordance with OCP platform.

Fixes: submariner-io#2603

Signed-off-by: Yossi Boaron <[email protected]>
sridhargaddam
sridhargaddam reviewed Jul 24, 2023
View reviewed changes
config/rbac/submariner-operator/ocp_cluster_role.yaml
- apiGroups:
- config.openshift.io
resources:
- infrastructures
Copy link
Member

Choose a reason for hiding this comment

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

In the addon code, I see that we are configuring explicit access to "infrastructures/status"
Please check if similar thing needs to be done even here.

https://github.com/stolostron/submariner-addon/blob/61ece25b8f65c6fa522b4f3216871c85f472b27b/deploy/config/rbac/cluster_role.yaml#L50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I checked it on OCP (ROKS) cluster and it works without explicit access to "infrastructures/status",
also I can see that for example SRIOV operator doesn't specify infrastructure/status and use fields from infra/status

Copy link
Member

Choose a reason for hiding this comment

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

Right, explicit permissions on /status sub-structs is only required for code which updates status information.

@submariner-bot submariner-bot added the ready-to-test When a PR is ready for full E2E testing label Jul 26, 2023
@yboaron yboaron merged commit 715955c into submariner-io:devel Jul 26, 2023
37 checks passed
@tpantelis tpantelis mentioned this pull request Jul 31, 2023
Merged
@dfarrell07 dfarrell07 added release-note-needed Should be mentioned in the release notes needs-triage labels Aug 8, 2023
@dfarrell07
Copy link
Member

@yboaron Do you think this needs to be backported? And reminder about release notes, if they are needed.

@yboaron
Copy link
Contributor Author

yboaron commented Sep 5, 2023

@yboaron Do you think this needs to be backported? And reminder about release notes, if they are needed.

@dfarrell07 , this PR fixed a minor bug in ARO (unneeded annotation relevant for ROSA appears also in ARO) and adds ROKS support.

Since ROKS support should start from 0.16 I don't think backport is needed here.

@skitt skitt removed the needs-triage label Sep 5, 2023
@dfarrell07 dfarrell07 mentioned this pull request Oct 5, 2023
Closed
@skitt
Copy link
Member

skitt commented Oct 17, 2023

@yboaron do you think this is worth mentioning in the release notes? It seems like an implementation detail.

@yboaron
Copy link
Contributor Author

yboaron commented Oct 17, 2023

@yboaron do you think this is worth mentioning in the release notes? It seems like an implementation detail.

Yep, also think we shouldn't mention it in release notes.

@skitt skitt removed the release-note-needed Should be mentioned in the release notes label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpantelis tpantelis tpantelis approved these changes

@skitt skitt skitt approved these changes

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

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

Assignees
No one assigned
Labels
ready-to-test When a PR is ready for full E2E testing
Projects
No open projects
Submariner 0.16
Status: Done
Milestone
No milestone
6 participants
@yboaron @submariner-bot @dfarrell07 @skitt @sridhargaddam @tpantelis