Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

feat: add a proxy url field to kubefed clusters #1377

Merged
merged 4 commits into from
Mar 23, 2021
Merged

feat: add a proxy url field to kubefed clusters #1377

merged 4 commits into from
Mar 23, 2021

Conversation

hectorj2f
Copy link
Contributor

What this PR does / why we need it:

There are certain scenarios where a kubefed cluster can be behind a NAT gateway or DMZ. kubefed needs to be able to talk to these specific clusters using a proxy url. In addition, kubernetes supports (k8s > v1.19) the definition of a proxy-url field in the kubeconfig for the clients. This simplifies the communication against these clusters.

Kubefed clusters could require specifying a Proxy URL in addition to a CaBundle, therefore we should consider adding a ProxyURL field to the kubefed cluster object.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1376

Special notes for your reviewer:

@hectorj2f hectorj2f added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 16, 2021
@hectorj2f hectorj2f self-assigned this Mar 16, 2021
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2021
jimmidyson
jimmidyson approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

LGTM. Wonder if we could have an integration or e2e test for this?

pkg/apis/core/v1beta1/kubefedcluster_types_test.go Outdated Show resolved Hide resolved
pkg/apis/core/v1beta1/kubefedcluster_types_test.go Show resolved Hide resolved
pkg/apis/core/v1beta1/validation/validation.go Outdated Show resolved Hide resolved
pkg/apis/core/v1beta1/validation/validation.go Outdated Show resolved Hide resolved
pkg/controller/util/cluster_util.go Outdated Show resolved Hide resolved
pkg/kubefedctl/join.go Outdated Show resolved Hide resolved
jimmidyson
jimmidyson suggested changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

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

LGTM with some nits. Also, can we get an integration or e2e test for this?

@hectorj2f hectorj2f force-pushed the hectorj2f/kubefed_cluster_proxy_url branch 5 times, most recently from ecf203c to 3aef383 Compare March 16, 2021 14:39
@hectorj2f hectorj2f mentioned this pull request Mar 16, 2021
Merged
6 tasks
@hectorj2f
Copy link
Contributor Author

@jimmidyson Yes, I agree. I'll add some tests to this PR to see how much we can test it without having a realistic infrastructure where to use a proxy.

@hectorj2f hectorj2f force-pushed the hectorj2f/kubefed_cluster_proxy_url branch from 3aef383 to 2f39197 Compare March 22, 2021 19:48
@hectorj2f hectorj2f force-pushed the hectorj2f/kubefed_cluster_proxy_url branch from 2f39197 to f4ac131 Compare March 22, 2021 20:21
irfanurrehman
irfanurrehman approved these changes Mar 22, 2021
View reviewed changes
Copy link
Contributor

@irfanurrehman irfanurrehman left a comment

Choose a reason for hiding this comment

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

Overall LGTM, few questions.
Thanks @hectorj2f

Suggestion: Again about the management of commits.
This commit is of no use, does not do what the title says and pollutes history. Please consider removing/squashing it.

pkg/apis/core/v1beta1/kubefedcluster_types_test.go Outdated Show resolved Hide resolved
pkg/apis/core/v1beta1/suite_test.go Show resolved Hide resolved
pkg/kubefedctl/join_test.go Outdated Show resolved Hide resolved
@hectorj2f
feat: add support for cluster proxy-url
04a40f9 
Signed-off-by: Hector Fernandez <hectorj@gmail.com>

Co-authored-by: Hector Fernandez <hectorj@gmail.com>
Co-authored-by: Martin Hrabovcin <mhrabovcin@gmail.com>"
@hectorj2f
test: add validation and tests for the proxy-url
b83ed76 
Signed-off-by: Hector Fernandez <hectorj@gmail.com>
@hectorj2f
test: add join cluster test
f523042 
Signed-off-by: Hector Fernandez <hectorj@gmail.com>
@hectorj2f hectorj2f force-pushed the hectorj2f/kubefed_cluster_proxy_url branch from f4ac131 to 0699b0a Compare March 23, 2021 06:08
@hectorj2f
test: add another iteration to test the API type
840d377 
Signed-off-by: Hector Fernandez <hectorj@gmail.com>
@hectorj2f hectorj2f force-pushed the hectorj2f/kubefed_cluster_proxy_url branch from 0699b0a to 840d377 Compare March 23, 2021 06:14
@hectorj2f
Copy link
Contributor Author

@jimmidyson I added an integration test to exercise the join cluster operation. I find this test and the others in this PR enough to validate this new property.

makkes
makkes approved these changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@makkes makkes left a comment

Choose a reason for hiding this comment

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

I'm missing a test actually performing tests of the proxy functionality but let's take what we have. ;-)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hectorj2f, irfanurrehman, makkes

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:
  • OWNERS [hectorj2f,irfanurrehman]

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

@makkes
Copy link
Contributor

makkes commented Mar 23, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit 179511a into kubernetes-retired:master Mar 23, 2021
@hectorj2f hectorj2f deleted the hectorj2f/kubefed_cluster_proxy_url branch March 23, 2021 18:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mhrabovcin mhrabovcin mhrabovcin left review comments

@makkes makkes makkes approved these changes

@irfanurrehman irfanurrehman irfanurrehman approved these changes

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@makkes makkes

@hectorj2f hectorj2f

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Kubefed cluster should store the proxy URL to talk to clusters in DMZs
6 participants
@hectorj2f @k8s-ci-robot @makkes @mhrabovcin @jimmidyson @irfanurrehman