CRI: An empty DNSConfig != unspecified

[roman-kiselenko]

if DnsConfig empty, it should copy the host's resolv.conf

What type of PR is this?

/kind other

What this PR does / why we need it:

This PR coordinates the behavior with the containerd. containerd/containerd#9730

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Reference kubernetes/kubernetes#120748

Does this PR introduce a user-facing change?

I guess some users will find this behavior a breaking change.

In case of an empty struct of DSNConfig /etc/resolv.conf be copied.

[openshift-ci]

Hi @roman-kiselenko. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[roman-kiselenko]

/cc @saschagrunert @haircommander @mrunalp

[codecov]

Codecov Report

Merging #7764 (abf3885) into main (59ea87d) will increase coverage by 0.00%.
Report is 6 commits behind head on main.
The diff coverage is 0.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7764   +/-   ##
=======================================
  Coverage   48.04%   48.05%           
=======================================
  Files         148      148           
  Lines       16339    16374   +35     
=======================================
+ Hits         7850     7868   +18     
- Misses       7533     7546   +13     
- Partials      956      960    +4     

[haircommander]

/ok-to-test

this is interesting. It's listed explicitly in https://github.com/containerd/containerd/pull/9730/files#diff-960bde24e7e0ac9df6b2556d747f483f0f3e6938d092e2b69bdb6750b7374be9R282-R285 that copying the host resolvConf on nil is potentially not the correct way, but containerd needs to do it for backwards compatibility. It almost feels like we shouldn't introduce this behavior if we don't have that reason? the alternative would be treating a nil object like an empty one and creating an empty s.config.DnsServer if it isn't defined.

WDYT @cri-o/cri-o-maintainers

[roman-kiselenko]

I also have doubts about this since copying the host resolve.conf potentially may lead to problems.

The alternative would be treating a nil object like an empty one and creating an empty s.config.DnsServer if it isn't defined.

Something like this 🤔 :

if s.config.DnsConfig == nil {
   s.config.DnsConfig = &types.DnsConfig{}
}

[haircommander]

yeah exactly!

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: roman-kiselenko, saschagrunert

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 [saschagrunert]

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

[saschagrunert]

/retest

[roman-kiselenko]

Not sure why tests are failed, will take a look closer 👀
/retest

[sohankunkerkar]

Something like this 🤔 :

if s.config.DnsConfig == nil {
s.config.DnsConfig = &types.DnsConfig{}
}

@roman-kiselenko are you planning to add this code?

[roman-kiselenko]

Something like this 🤔 :

if s.config.DnsConfig == nil {
s.config.DnsConfig = &types.DnsConfig{}
}

@roman-kiselenko are you planning to add this code?

yep 👍

[sohankunkerkar]

/hold

[sohankunkerkar]

/retest
/lgtm
/unhold