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

ipatests: replace ad hoc backup with FileBackup helper #3868

Closed
wants to merge 3 commits into from

Conversation

wladich
Copy link

@wladich wladich commented Nov 7, 2019

Test test_smb_mount_and_access_by_different_users was failing with message
```
kdestroy: Permission denied while initializing krb5
```

This happened because the previous test
`test_smb_access_for_ad_user_at_ipa_client` was calling the fixture
`enable_smb_client_dns_lookup_kdc` which was doing backup of krb5.conf
in a wrong way:
- mktemp (to create a temp file)
- cp /etc/krb5.conf to the temp file
- ...
- mv tempfile /etc/krb5.conf

This flow looses the file permissions, because mktemp creates a file
using the default umask, which results in -rw------- permissions.
The copy does not modify the permissions, and the mv keeps the
permissions from the source => /etc/krb5.conf now has -rw-------.

Fixes: https://pagure.io/freeipa/issue/8115

Copy link
Contributor

@frozencemetery frozencemetery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it's "ad hoc", not "ad-hock". "ad hoc" is from Latin; it means roughly "for this purpose", or in English, is equivalent to "bespoke" or "custom-built". It's unrelated to the word "hock", which has a variety of meanings but is most commonly used around debt.

@wladich wladich added the needs review Pull Request is waiting for a review label Nov 7, 2019
@flo-renaud
Copy link
Contributor

@wladich
Thanks for the fix, LGTM. If you could fix the typo noted by @frozencemetery in the commit msg that would be great.

@flo-renaud flo-renaud self-assigned this Nov 8, 2019
@flo-renaud flo-renaud removed the needs review Pull Request is waiting for a review label Nov 8, 2019
@wladich wladich changed the title ipatests: replace ad-hock backup with FileBackup helper ipatests: replace ad hoc backup with FileBackup helper Nov 8, 2019
@wladich
Copy link
Author

wladich commented Nov 8, 2019

I was sure I fixed it yesterday, but something went wrong. Fixed now.

@freeipa-pr-ci freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Nov 11, 2019
* `cp` now preserves all attributes of original file, there is no reason
  to select only some of them
* backup is now restored with `mv` instead of `cp` to avoid leaving junk

Related to: https://pagure.io/freeipa/issue/8115
Test test_smb_mount_and_access_by_different_users was failing with message
```
kdestroy: Permission denied while initializing krb5
```

This happened because the previous test
`test_smb_access_for_ad_user_at_ipa_client` was calling the fixture
`enable_smb_client_dns_lookup_kdc` which was doing backup of krb5.conf
in a wrong way:
- mktemp (to create a temp file)
- cp /etc/krb5.conf to the temp file
- ...
- mv tempfile /etc/krb5.conf

This flow looses the file permissions, because mktemp creates a file
using the default umask, which results in -rw------- permissions.
The copy does not modify the permissions, and the mv keeps the
permissions from the source => /etc/krb5.conf now has -rw-------.

Fixes: https://pagure.io/freeipa/issue/8115
@wladich wladich added needs review Pull Request is waiting for a review ipa-4-8 Mark for backport to ipa 4.8 and removed needs rebase Pull Request cannot be automatically merged - needs to be rebased labels Nov 11, 2019
@wladich
Copy link
Author

wladich commented Nov 11, 2019

Fix for FileBackup is backported to ipa-4-7 in #3883

@abbra abbra added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Nov 11, 2019
@abbra
Copy link
Contributor

abbra commented Nov 11, 2019

LGTM.

@wladich wladich added the pushed Pull Request has already been pushed label Nov 11, 2019
@wladich
Copy link
Author

wladich commented Nov 11, 2019

master:

  • 72540c4 ipatests: refactor FileBackup helper
  • c2b230c ipatests: replace ad hoc backup with FileBackup helper
  • f58fb57 ipatests: enable test_smb.py in gating.yaml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-8 Mark for backport to ipa 4.8 pushed Pull Request has already been pushed
Projects
None yet
6 participants