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

kubeadm: Fix omitempty in v1beta2 #77345

Merged
merged 1 commit into from May 4, 2019

Conversation

@rosti
Copy link
Member

commented May 2, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

There are a couple of problems with regards to the omitempty in v1beta1:

  • It is not applied to certain fields. This makes emitting YAML configuration files in v1beta1 config format verbose by both kubeadm and third party Go lang tools. Certain fields, that were never given an explicit value would show up in the marshalled YAML document. This can cause confusion and even misconfiguration.

  • It can be used in inappropriate places. In this case it's used for fields, that need to be always serialized. The only one such field at the moment is NodeRegistrationOptions.Taints. If the Taints field is nil, then it's defaulted to a slice containing a single control plane node taint. If it's an empty slice, no taints are applied, thus, the cluster behaves differently. With that in mind, a Go program, that uses v1beta1 with omitempty on the Taints field has no way to specify an explicit empty slice of taints, as this would get lost after marshalling to YAML.

To fix these issues the following is done in this change:

  • A whole bunch of additional omitemptys are placed at many fields in v1beta2.
  • omitempty is removed from NodeRegistrationOptions.Taints
  • A test, that verifies the ability to specify empty slice value for Taints is included.

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#1358
Refs kubernetes/enhancements#970 kubernetes/kubeadm#1439

Special notes for your reviewer:
/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/priority important-soon
/assign @fabriziopandini
/assign @timothysc
/assign @neolit123
/cc @dlipovetsky

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rosti

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:

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

@neolit123
Copy link
Member

left a comment

thanks @rosti
the diff LGTM.

kubeadm: Fix omitempty in v1beta2
There are a couple of problems with regards to the `omitempty` in v1beta1:

- It is not applied to certain fields. This makes emitting YAML configuration
  files in v1beta1 config format verbose by both kubeadm and third party Go
  lang tools. Certain fields, that were never given an explicit value would
  show up in the marshalled YAML document. This can cause confusion and even
  misconfiguration.

- It can be used in inappropriate places. In this case it's used for fields,
  that need to be always serialized. The only one such field at the moment is
  `NodeRegistrationOptions.Taints`. If the `Taints` field is nil, then it's
  defaulted to a slice containing a single control plane node taint. If it's
  an empty slice, no taints are applied, thus, the cluster behaves differently.
  With that in mind, a Go program, that uses v1beta1 with `omitempty` on the
  `Taints` field has no way to specify an explicit empty slice of taints, as
  this would get lost after marshalling to YAML.

To fix these issues the following is done in this change:

- A whole bunch of additional omitemptys are placed at many fields in v1beta2.
- `omitempty` is removed from `NodeRegistrationOptions.Taints`
- A test, that verifies the ability to specify empty slice value for `Taints`
  is included.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

@rosti rosti force-pushed the rosti:omitempty-v1beta2 branch from 2210219 to 81e3adc May 3, 2019

@dixudx

dixudx approved these changes May 4, 2019

Copy link
Member

left a comment

/lgtm

@rosti

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

/test pull-kubernetes-integration

@fejta-bot

This comment has been minimized.

Copy link

commented May 4, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 0a83ed5 into kubernetes:master May 4, 2019

20 checks passed

cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.