(CAT-1483) - Enhancement of handling of apt::source's repos and release parameters

[Ramesh7]

Summary

Generating the correct source list based on the $release and $repos.

Related Issues (if any)

https://github.com/puppetlabs/puppetlabs-apt/issues/1132

Checklist

• [x] 🟢 Spec tests.
• [ ] 🟢 Acceptance tests.
• [ ] Manually verified. (For example puppet apply)

[Ramesh7]

I haven't looked at the problem in detail enough to suggest how to solve this, but I think a different approach needs to be taken.

Thanks @kenyon for your review, the problem statement here is user can put any random values like / for $release and $repo results source list gets generated invalid. So I am trying to enhance with valid set of values so that it will be valid generated source list.

[XSpielinbox]

See #1132 - this is the core problem.

[Ramesh7]

I think you completely misunderstood the problem. I hope my comments help.

In my opinion it would be way easier if puppet would just follow the the sources.list spec and require a release to be given (and perhaps as a convenience, if none is given default to stable or $facts['os']['distro']['codename']), whereas repos is optional and if none are given it just terminates the apt source after the release.

Thanks for your inputs @XSpielinbox. Wanted to reconfirm my understanding :

On $release parameter the current implementation is giving priority to user input and defaulting to $facts['os']['distro']['codename'] if not given and if the $fact is not available then it will fail so not sure if you want here to be stable over failing? Coming to $repos it default to main if user is not passing but its not optional param, so I think you want it optional and if none is given then the source list should terminates to release only.

Please confirm my understanding?

[XSpielinbox]

Thanks for your inputs @XSpielinbox. Wanted to reconfirm my understanding :

On $release parameter the current implementation is giving priority to user input and defaulting to $facts['os']['distro']['codename'] if not given and if the $fact is not available then it will fail so not sure if you want here to be stable over failing?

Ah, ok. Then the documentation is very confusing in my opinion. It states in the REFERENCE.md, that release is optional and shall default to undef, which I would interpret that it defaults to omitting it completely. When it already defaults to $facts['os']['distro']['codename'], if not given, this is absolutely fine. A second fallback is not needed in my opinion. The documentation should make this more clear, however. NOTE: That the user does not has to specify release is just a convenience function of Puppet - in the resulting soures.list there should always be a distribution/suite (so the parameter release is configuring).

Coming to $repos it default to main if user is not passing but its not optional param, so I think you want it optional and if none is given then the source list should terminates to release only.

Please confirm my understanding?

Yes, I definitely want repos to be optional, because as outlined above there are many valid use cases where one does not have a component and even is not allowed to specify one. There must be a way to tell puppet that the sources.list ends after the release (the distribution/suite in apt) and does not have a repos (one or more components in apt).

[Ramesh7]

I just tried by patching local code by making both $repos optional with below manifest :

apt::source { 'puppetlabs':
  location => 'http://apt.puppetlabs.com',
  key      => {
    'id'     => '6F6B15509CF8E59E6E469F327F438280EF8D349F',
    'server' => 'pgp.mit.edu',
  },
}

Run agent which got completed with error :

Error: /Stage[main]/Apt::Update/Exec[apt_update]: Failed to call refresh: '/usr/bin/apt-get update' returned 100 instead of one of [0]
Error: /Stage[main]/Apt::Update/Exec[apt_update]: '/usr/bin/apt-get update' returned 100 instead of one of [0]

The generated source list file which terminated on release itself and no repos:

root@44fe6a22604b:~# cat /etc/apt/sources.list.d/puppetlabs.list
# This file is managed by Puppet. DO NOT EDIT.
# puppetlabs
deb http://apt.puppetlabs.com jammy

On other side with current code without passing with same manifest it generated following source list

deb http://apt.puppetlabs.com jammy main

which works perfect.

[XSpielinbox]

I just tried by patching local code by making both $repos optional with below manifest :

apt::source { 'puppetlabs':
  location => 'http://apt.puppetlabs.com',
  key      => {
    'id'     => '6F6B15509CF8E59E6E469F327F438280EF8D349F',
    'server' => 'pgp.mit.edu',
  },
}

Run agent which got completed with error :

Error: /Stage[main]/Apt::Update/Exec[apt_update]: Failed to call refresh: '/usr/bin/apt-get update' returned 100 instead of one of [0]
Error: /Stage[main]/Apt::Update/Exec[apt_update]: '/usr/bin/apt-get update' returned 100 instead of one of [0]

The generated source list file which terminated on release itself and no repos:

root@44fe6a22604b:~# cat /etc/apt/sources.list.d/puppetlabs.list
# This file is managed by Puppet. DO NOT EDIT.
# puppetlabs
deb http://apt.puppetlabs.com jammy

On other side with current code without passing with same manifest it generated following source list

deb http://apt.puppetlabs.com jammy main

which works perfect.

Yes, this is to be expected, because the components may only be omitted, if the suite is an absolute suite, meaning it ends with /, but if it it ends with /, the components must be omitted, so: deb http://apt.puppetlabs.com jammy main -> valid deb http://apt.puppetlabs.com jammy -> invalid, no component deb http://apt.puppetlabs.com jammy/ main -> invalid, absolute suite component deb http://apt.puppetlabs.com jammy/ -> valid

[Ramesh7]

I just tried by patching local code by making both $repos optional with below manifest :

apt::source { 'puppetlabs':
  location => 'http://apt.puppetlabs.com',
  key      => {
    'id'     => '6F6B15509CF8E59E6E469F327F438280EF8D349F',
    'server' => 'pgp.mit.edu',
  },
}

Run agent which got completed with error :

Error: /Stage[main]/Apt::Update/Exec[apt_update]: Failed to call refresh: '/usr/bin/apt-get update' returned 100 instead of one of [0]
Error: /Stage[main]/Apt::Update/Exec[apt_update]: '/usr/bin/apt-get update' returned 100 instead of one of [0]

The generated source list file which terminated on release itself and no repos:

root@44fe6a22604b:~# cat /etc/apt/sources.list.d/puppetlabs.list
# This file is managed by Puppet. DO NOT EDIT.
# puppetlabs
deb http://apt.puppetlabs.com jammy

On other side with current code without passing with same manifest it generated following source list

deb http://apt.puppetlabs.com jammy main

which works perfect.

Yes, this is to be expected, because the components may only be omitted, if the suite is an absolute suite, meaning it ends with /, but if it it ends with /, the components must be omitted, so: deb http://apt.puppetlabs.com jammy main -> valid deb http://apt.puppetlabs.com jammy -> invalid, no component deb http://apt.puppetlabs.com jammy/ main -> invalid, absolute suite component deb http://apt.puppetlabs.com jammy/ -> valid

Then I think the module should handle if the release contain / then it should omit the component, is that correct understanding? And I believe that's behaviour you are expecting, right? Also wanted to confirm the behaviour what if the release contain / char in middle of release name, then I think the component name will be required. The same behaviour I have tried locally and seering behaviour what I have described.

deb http://apt.puppetlabs.com jammy/test => Invalid deb http://apt.puppetlabs.com jammy/test main => Valid

[Ramesh7]

@XSpielinbox Have created temp commit based on my understanding https://github.com/puppetlabs/puppetlabs-apt/commit/003c904b40e3c20cfb1607aaf61db327315555cd Please review and suggest. Thanks

[XSpielinbox]

Then I think the module should handle if the release contain / then it should omit the component, is that correct understanding?

No, only if release ENDS with a / there shall not be a component, it is also stated in the sources.list man page from Ubuntu.

And I believe that's behaviour you are expecting, right?

This would also be an option, though I was actually expecting that the user has to take care of that himself, that this makes sense and puppet would just lay the groundwork to enable him to do so easily. That's why it's crucial to treat release and repos as arbitrary strings what input validation is concerned in my opinion. Even though there are perhaps strings that will never be able to produce a valid sources.list (e.g. perhaps specifying a string that starts with # and therefore is treated as a comment), I would consider it out of the scope and maybe even outside of the doable of this module to ensure, that this will always be valid. Rather it should ensure, that whatever the user wishes to input there, always will get applied, with the focus of course being on use-cases that adhere to the sources.list specification. As having a component after a distribution that ends with / is an example of something that will never be valid, it would make sense to me to have puppet drop the component in this case automatically.

But to be clear: I don't care, whether it then drops the component automatically or I explicitly have to state that I don't want a component at all. What matters to me is, that I don't have to do some strange workaround like passing an empty component or specifying a component that is actually a comment to be able to produce the absolutely valid sources.list that I need.

Also wanted to confirm the behaviour what if the release contain / char in middle of release name, then I think the component name will be required. The same behaviour I have tried locally and seering behaviour what I have described.

deb http://apt.puppetlabs.com jammy/test => Invalid deb http://apt.puppetlabs.com jammy/test main => Valid

Yes, as stated above, you can have as many / as you like, it only matters, whether there is one at the end. If there is one at the end, it must only be followed by comments or a line-terminator. If it does not end with / it must be followed by at least one component.

[XSpielinbox]

@XSpielinbox Have created temp commit based on my understanding 003c904 Please review and suggest. Thanks

As it is not part of a PR, I cannot review it, but I commented beneath the commit.

[Ramesh7]

@XSpielinbox Have pulled commit changes here, ready for review. Also handled additional spaces concern on that commit, now this change will not pull additional spaces in source list file. Looking forward to here from you.

[XSpielinbox]

Summary

Generating the correct source list based on the $release and $repos.

Related Issues (if any)

1133

Checklist

* [x]  🟢 Spec tests.

* [ ]  🟢 Acceptance tests.

* [ ]  Manually verified. (For example `puppet apply`)

Another thing, though: This PR would then now be a solution for #1132 and NOT #1133

[praj1001]

LGTM