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

ClusterNetwork should be optional #1009

Closed
chuckha opened this issue Jun 11, 2019 · 5 comments
Closed

ClusterNetwork should be optional #1009

chuckha opened this issue Jun 11, 2019 · 5 comments
Labels
area/api Issues or PRs related to the APIs kind/bug Categorizes issue or PR as related to a bug.

Comments

@chuckha
Copy link
Contributor

chuckha commented Jun 11, 2019

/kind bug

What steps did you take and what happened:
ClusterNetwork on v1a1.Cluster object is required but nothing else is. Is there a reason this is required and not defaulted? kubeadm can probably provide some sane defaults for these values.

What did you expect to happen:
defaulting the cluster network fields.

Anything else you would like to add:

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 11, 2019
@detiber
Copy link
Member

detiber commented Jun 11, 2019

I see no issues with providing defaulting for this, the question would be how to do it. There are potentially 2 different ways:

  • webhook
    • this would require no changes to the types themselves
    • adding the default would not trigger an additional reconciliation
  • updating the types to allow for omitempty
    • could add the default in the cluster controller, this would trigger an additional reconciliation
    • could allow to remain empty, but would require any places that would consume this value to know handle applying a default.

@vincepri
Copy link
Member

/area api

@k8s-ci-robot k8s-ci-robot added the area/api Issues or PRs related to the APIs label Jun 11, 2019
@davidewatson
Copy link
Contributor

I think kubeadm does default these values. I put up a PR a while back to make this field optional: #919

@detiber
Copy link
Member

detiber commented Jun 11, 2019

kubeadm does indeed default these values. That said, this goes a bit beyond just bootstrapping, as it would also impact the CNI configuration as well.

@vincepri
Copy link
Member

Closing now that #919 has been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Issues or PRs related to the APIs kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

5 participants