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

optionally configure cloud routes based on overlay #561

Merged
merged 1 commit into from
Feb 28, 2023
Merged

optionally configure cloud routes based on overlay #561

merged 1 commit into from
Feb 28, 2023

Conversation

kon-angelo
Copy link
Contributor

@kon-angelo kon-angelo commented Feb 27, 2023 •
edited
Loading

How to categorize this PR?

/area control-plane
/kind bug
/platform gcp

What this PR does / why we need it:
Do not configure cloud routes if you are not using overlay network

Which issue(s) this PR fixes:
Fixes ##558

Special notes for your reviewer:

Release note:

Disable cloud-controller-manager's route controller only if the shoot is using an overlay network.

@kon-angelo kon-angelo requested review from a team as code owners February 27, 2023 13:47
@gardener-robot gardener-robot added area/control-plane Control plane related kind/bug Bug platform/gcp Google cloud platform/infrastructure needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Feb 27, 2023
@kon-angelo kon-angelo added this to the v1.28 milestone Feb 27, 2023
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 27, 2023
@kon-angelo
Copy link
Contributor Author

/invite @ScheererJ

@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 27, 2023
ScheererJ
ScheererJ reviewed Feb 27, 2023
View reviewed changes
Copy link
Member

@ScheererJ ScheererJ left a comment

Choose a reason for hiding this comment

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

Please invert the release note.

pkg/controller/controlplane/valuesprovider.go
return o == nil || o.Enabled, nil
}

return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

Would it not be a good idea to return an error here? This is certainly not an expected case in that we do not know the networking type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. That would strictly limit the networking extensions to the very limited set of {calico,cilium} and hardcode that dependency on the infra provider.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, your call.

ScheererJ
ScheererJ approved these changes Feb 27, 2023
View reviewed changes
Copy link
Member

@ScheererJ ScheererJ left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot
Copy link

@ScheererJ Command /lgtm is not available to you but only to a Maintainer, Member.

MartinWeindel
MartinWeindel approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@MartinWeindel MartinWeindel left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Feb 28, 2023
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Feb 28, 2023
@kon-angelo kon-angelo merged commit 6169c8f into gardener:master Feb 28, 2023
@kon-angelo kon-angelo deleted the configure-routes branch February 28, 2023 15:47
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Feb 28, 2023
@kon-angelo kon-angelo mentioned this pull request Mar 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinWeindel MartinWeindel MartinWeindel approved these changes

@ScheererJ ScheererJ ScheererJ approved these changes

Assignees
No one assigned
Labels
area/control-plane Control plane related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/gcp Google cloud platform/infrastructure reviewed/lgtm Has approval for merging size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.
6 participants
@kon-angelo @gardener-robot @MartinWeindel @ScheererJ @gardener-robot-ci-2 @gardener-robot-ci-3