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

+kubebuilder:default={} does not use nested defaults #622

Open
astoycos opened this issue Sep 21, 2021 · 31 comments
Open

+kubebuilder:default={} does not use nested defaults #622

astoycos opened this issue Sep 21, 2021 · 31 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@astoycos
Copy link
Member

So I believe I exposed a bug in kubebuilder...

when I define a +kubebuilder:default={} as a pointer to another type with +kubebuilder:default values I assume those values should propagate to the parent object.

For example

// policyAuditConfig is the configuration for network policy audit events. If unset,
// reported defaults are used.
// +kubebuilder:default={}
// +optional
PolicyAuditConfig *PolicyAuditConfig `json:"policyAuditConfig,omitempty"`

Where the PolicyAuditConfig is defined in the same package

type PolicyAuditConfig struct {
	// rateLimit is the approximate maximum number of messages to generate per-second per-node. If
	// unset the default of 20 msg/sec is used.
	// +kubebuilder:default=20
	// +kubebuilder:validation:Minimum=1
	// +optional
	RateLimit *uint32 `json:"rateLimit,omitempty"`

	// maxFilesSize is the max size an ACL_audit log file is allowed to reach before rotation occurs
	// Units are in MB and the Default is 50MB
	// +kubebuilder:default=50
	// +kubebuilder:validation:Minimum=1
	// +optional
	MaxFileSize *uint32 `json:"maxFileSize,omitempty"`

	// destination is the location for policy log messages.
	// Regardless of this config, persistent logs will always be dumped to the host
	// at /var/log/ovn/ however
	// Additionally syslog output may be configured as follows.
	// Valid values are:
	// - "libc" -> to use the libc syslog() function of the host node's journdald process
	// - "udp:host:port" -> for sending syslog over UDP
	// - "unix:file" -> for using the UNIX domain socket directly
	// - "null" -> to discard all messages logged to syslog
	// The default is "null"
	// +kubebuilder:default=null
	// +kubebuilder:pattern='^libc$|^null$|^udp:(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]):([0-9]){0,5}$|^unix:(\/[^\/ ]*)+([^\/\s])$'
	// +optional
	Destination string `json:"destination,omitempty"`

	// syslogFacility the RFC5424 facility for generated messages, e.g. "kern". Default is "local0"
	// +kubebuilder:default=local0
	// +kubebuilder:Enum=kern;user;mail;daemon;auth;syslog;lpr;news;uucp;clock;ftp;ntp;audit;alert;clock2;local0;local1;local2;local3;local4;local5;local6;local7
	// +optional
	SyslogFacility string `json:"syslogFacility,omitempty"`
}

However rather than rendering the default correctly I see the following in the generated yaml

policyAuditConfig:
  description: policyAuditConfig is the configuration for network policy audit events. If unset, reported defaults are used.
  type: object
  default: null
  properties:
    destination:
      description: 'destination is the location for policy log messages. Regardless of this config, persistent logs will always be dumped to the host at /var/log/ovn/ however Additionally syslog output may be configured as follows. Valid values are: - "libc" -> to use the libc syslog() function of the host node''s journdald process - "udp:host:port" -> for sending syslog over UDP - "unix:file" -> for using the UNIX domain socket directly - "null" -> to discard all messages logged to syslog The default is "null"'
      type: string
      default: "null"
    maxFileSize:
      description: maxFilesSize is the max size an ACL_audit log file is allowed to reach before rotation occurs Units are in MB and the Default is 50MB
      type: integer
      format: int32
      default: 50
      minimum: 1
    rateLimit:
      description: rateLimit is the approximate maximum number of messages to generate per-second per-node. If unset the default of 20 msg/sec is used.
      type: integer
      format: int32
      default: 20
      minimum: 1
    syslogFacility:
      description: syslogFacility the RFC5424 facility for generated messages, e.g. "kern". Default is "local0"
      type: string
      default: local0

Specifically the default value is null where it should be populated automatically by the defined nested defaults.

I am happy to help propose a fix here, however after spending some time playing with the codebase I'm realizing I will probably need some assistance from someone more experienced with kubebuilder

I already reached out on slack and got no response, which prompted me to open this issue.

Thanks,
Andrew

astoycos added a commit to astoycos/api that referenced this issue Sep 21, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>

TMP
astoycos added a commit to astoycos/api that referenced this issue Sep 21, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>

TMP
astoycos added a commit to astoycos/api that referenced this issue Sep 22, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
astoycos added a commit to astoycos/api that referenced this issue Sep 22, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
astoycos added a commit to astoycos/api that referenced this issue Sep 22, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/api that referenced this issue Sep 22, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/api that referenced this issue Sep 27, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
EmilyM1 pushed a commit to EmilyM1/api that referenced this issue Oct 12, 2021
We need to initialize `ovnKubernetesConfig.policyAuditConfig`
so that the proper defaults are populated when a CNO config
object is created

It would be better to do this using the `+kubebuilder:default`
flag however it is broken for this case as described in this
[upstream issue](kubernetes-sigs/controller-tools#622)

Signed-off-by: astoycos <astoycos@redhat.com>
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 20, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 19, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@astoycos
Copy link
Member Author

/reopen

@k8s-ci-robot
Copy link
Contributor

@astoycos: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Feb 23, 2022
@astoycos
Copy link
Member Author

/bug

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@therealak12
Copy link

/reopen

@k8s-ci-robot
Copy link
Contributor

@therealak12: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@astoycos
Copy link
Member Author

astoycos commented May 1, 2022

/reopen

@k8s-ci-robot
Copy link
Contributor

@astoycos: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this May 1, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liubog2008
Copy link

This issue seems not fixed?

@camilamacedo86 camilamacedo86 reopened this Dec 6, 2022
mtyazici pushed a commit to hazelcast/hazelcast-platform-operator that referenced this issue Dec 12, 2022
- Unify pointer and omitempty JSON tag usage
- Add all default fields for struct with default values.
With the exception of HZ anc MC spec. The change is done due to the
issue in  kubernetes-sigs/controller-tools#622
- WanReplication Queue , Batch and Acknowledgment fields did not have
struct default values but they were not pointers so those fields acted
like they had default values. Added explicit struct default values.
- Add enum kubebuilder tags
- Add optional comment for forgotten fields
- Add explicit required comment for required field
mtyazici pushed a commit to hazelcast/hazelcast-platform-operator that referenced this issue Dec 16, 2022
- Unify pointer and omitempty JSON tag usage
- Add all default fields for struct with default values.
With the exception of HZ anc MC spec. The change is done due to the
issue in  kubernetes-sigs/controller-tools#622
- WanReplication Queue , Batch and Acknowledgment fields did not have
struct default values but they were not pointers so those fields acted
like they had default values. Added explicit struct default values.
- Add enum kubebuilder tags
- Add optional comment for forgotten fields
- Add explicit required comment for required field
mtyazici pushed a commit to hazelcast/hazelcast-platform-operator that referenced this issue Dec 19, 2022
- Unify pointer and omitempty JSON tag usage
- Add all default fields for struct with default values.
With the exception of HZ anc MC spec. The change is done due to the
issue in  kubernetes-sigs/controller-tools#622
- WanReplication Queue , Batch and Acknowledgment fields did not have
struct default values but they were not pointers so those fields acted
like they had default values. Added explicit struct default values.
- Add enum kubebuilder tags
- Add optional comment for forgotten fields
- Add explicit required comment for required field
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue with /reopen
  • Mark this issue as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pmalek
Copy link
Contributor

pmalek commented Aug 23, 2023

This is still an issue. What "works" as a sort of workaround is specifying the default verbatim in every +kubebuilder:default marker, for instance:

type XStrategy struct {
	// +kubebuilder:validation:Optional
	// +kubebuilder:default={"plan":{"deployment":"ScaleDownOnPromotionScaleUpOnRollout"}}
	Resources RolloutResources `json:"resources,omitempty"`
}

type RolloutResources struct {
	// +kubebuilder:validation:Optional
	// +kubebuilder:default={"deployment":"ScaleDownOnPromotionScaleUpOnRollout"}
	Plan RolloutResourcePlans `json:"plan,omitempty"`
}


type RolloutResourcePlans struct {
	// +kubebuilder:validation:Enum=ScaleDownOnPromotionScaleUpOnRollout;DeleteOnPromotionRecreateOnRollout
	// +kubebuilder:default=ScaleDownOnPromotionScaleUpOnRollout
	Deployment RolloutResourcePlanDeployment `json:"deployment,omitempty"`
}

which is error prone and counterproductive. Field based defaults should be enough.

/reopen

@k8s-ci-robot
Copy link
Contributor

@pmalek: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

This is still an issue. What "works" as a sort of workaround is specifying the default verbatim in every +kubebuilder:default marker, for instance:

type XStrategy struct {
  // +kubebuilder:validation:Optional
  // +kubebuilder:default={"plan":{"deployment":"ScaleDownOnPromotionScaleUpOnRollout"}}
  Resources RolloutResources `json:"resources,omitempty"`
}

type RolloutResources struct {
  // +kubebuilder:validation:Optional
  // +kubebuilder:default={"deployment":"ScaleDownOnPromotionScaleUpOnRollout"}
  Plan RolloutResourcePlans `json:"plan,omitempty"`
}


type RolloutResourcePlans struct {
  // +kubebuilder:validation:Enum=ScaleDownOnPromotionScaleUpOnRollout;DeleteOnPromotionRecreateOnRollout
  // +kubebuilder:default=ScaleDownOnPromotionScaleUpOnRollout
  Deployment RolloutResourcePlanDeployment `json:"deployment,omitempty"`
}

which is error prone and counterproductive. Field based defaults should be enough.

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@astoycos
Copy link
Member Author

/reopen

@k8s-ci-robot
Copy link
Contributor

@astoycos: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot reopened this Aug 23, 2023
@astoycos
Copy link
Member Author

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 23, 2023
@franpog859
Copy link

This issue has been here for over two years now 😵 Does anyone plan to work on it?

@morucci
Copy link

morucci commented Dec 18, 2023

I just experienced the same behavior. I've used #622 (comment) as a workaround.

sf-project-io pushed a commit to softwarefactory-project/sf-operator that referenced this issue Jan 2, 2024
This change adds:

- a test to validate that the sf-operator
  can reconsile a SF CR with the minimal set of settings.
- a fix to provide default value for the logserver if the
  main field is not set.
- a fix for the system-config pipeline generator where
  the zuul connection name for the config repo is never
  recomputed.

See: kubernetes-sigs/controller-tools#622

Change-Id: Ibce761d80a9d90eeef29c09105b569890f1b4d8e
@ahmetb
Copy link
Member

ahmetb commented Jan 12, 2024

/lifecycle frozen
/kind bug

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. kind/bug Categorizes issue or PR as related to a bug. labels Jan 12, 2024
@sbueringer
Copy link
Member

Specifically the default value is null where it should be populated automatically by the defined nested defaults.

Not sure if I got this correctly. I think in your example the default of policyAuditConfig should be an empty object.

Here another example:

	// +kubebuilder:default={}
	// +optional
	ControlPlaneCertificateRotation *ControlPlaneCertificateRotation `json:"controlPlaneCertificateRotation,omitempty"`


type ControlPlaneCertificateRotation struct {
	// +kubebuilder:default=true
	// +optional
	Activate bool `json:"activate"`

	// +kubebuilder:default=90
	// +kubebuilder:validation:Minimum=7
	// +optional
	DaysBefore int32 `json:"daysBefore"`
}

This results in:

      default: {}
      properties:
        activate:
          default: true
          type: boolean
        daysBefore:
          default: 90
          format: int32
          minimum: 7
          type: integer
      type: object

The result is at runtime that Kubernetes will first default the controlPlaneCertificateRotation field to an empty object and then default each individual field (defaulting happens top-down and nested defaults are only applied if the parent exists)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests