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.

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

🌱 Add support for 'ipam' and 'other' providers to tilt #6647

Closed
wants to merge 1 commit into from

Conversation

schrej
Copy link
Member

@schrej schrej commented Jun 14, 2022

What this PR does / why we need it:
This adds support for 'ipam-' and 'other-' providers to tilt-prepare to allow using ipam or any other new providers with our tilt setup.

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 14, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign neolit123 after the PR has been reviewed.
You can assign the PR to them by writing /assign @neolit123 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 14, 2022
@schrej schrej force-pushed the tilt/other-providers branch from d12d374 to 537bb68 Compare June 14, 2022 10:05
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 14, 2022
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2022
@sbueringer
Copy link
Member

sbueringer commented Jul 7, 2022

Q: Is this the PR where we introduce new provider types by introducing ipam- and other- prefixes in addition to the ones we already have for bootstrap/infra/control-plane?

Mostly asking because I'm not aware of that clusterctl or any other part of the code base supports it. (e.g.

func ManifestLabel(name string, providerType ProviderType) string {
switch providerType {
case BootstrapProviderType:
return fmt.Sprintf("bootstrap-%s", name)
case ControlPlaneProviderType:
return fmt.Sprintf("control-plane-%s", name)
case InfrastructureProviderType:
return fmt.Sprintf("infrastructure-%s", name)
default:
return name
}
}
)

@schrej
Copy link
Member Author

schrej commented Jul 7, 2022

They are not supported by clusterctl. There is an issue regarding clusterctl support here: #6154
The idea here is to just enable those two kinds of providers to work with tilt for now. We can adapt the tilt config once we've decided how to handle it in clusterctl.

@fabriziopandini
Copy link
Member

/hold
Provider and ProviderType are clusterctl constructs, and until there is a proper analysis on what are the impacts of having additional provider types this can potentially prevent us from running clusterctl commands r or from debugging E2E tests (which are using clusterctl internally) on a cluster created with tilt.

As an alternative, we can merge this PR to simplify IPAM provider development, but we should clearly document in the comment/book that this is a temporary solution and it could lead to errors in the scenario described above

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2022
@schrej
Copy link
Member Author

schrej commented Aug 3, 2022

I have no idea what changed, and I didn't even notice because I was working with this branch checked out locally for a while, but it seems like this change is no longer necessary. It used to cause errors to have a provider that's not recognised by tilt-prepare, but that error vanished. We probably don't need this anymore.
The following config works fine with this tilt-provider.yaml config: https://github.com/telekom/cluster-api-ipam-provider-in-cluster/blob/main/tilt-provider.yaml

default_registry: localhost:5000
allowed_contexts: ["kind-capi"]
kind_cluster_name: "capi"
provider_repos:
- ../cluster-api-ipam-provider-in-cluster
enable_providers:
# - docker
- ipam-in-cluster
extra_args: 
  core: ["--feature-gates=ClusterTopology=true"]
  kubeadm-bootstrap: ["--feature-gates=ClusterTopology=true"]
  kubeadm-control-plane: ["--feature-gates=ClusterTopology=true"]
  docker: ["--feature-gates=ClusterTopology=true"]
debug:
  ipam-in-cluster:
    port: 30001

I'll reopen in case I run into issues again.

@schrej schrej closed this Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants