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][Azure Pipelines] Populate containers with self-AAAA records #5486

Closed
wants to merge 3 commits into from

Conversation

stanislavlevin
Copy link
Contributor

  • IPA server's AAAA records at embedded DNS mode depend on result of get_server_ip_address function(ipaserver.install.installutils), which in turn, relies on NSS. In case of Azure Pipelines, there are neither IPv6 records in '/etc/hosts' nor external DNS, which may provide such. This leads to the missing AAAA records for master and missing AAAA records for ipa-ca pointing to master in embedded DNS. In particular, tests test_ipa_healthcheck_no_errors, test_ipa_dns_systemrecords_check fail with:
    [
      {
        "source": "ipahealthcheck.ipa.idns",
        "check": "IPADNSSystemRecordsCheck",
        "result": "WARNING",
        "uuid": "b979a88a-6373-4990-bc83-ce724e9730b4",
        "when": "20210120055054Z",
        "duration": "0.032740",
        "kw": {
          "msg": "Got {count} ipa-ca AAAA records, expected {expected}",
          "count": 1,
          "expected": 2
        }
      }
    ]

where ipa-ca record exists only for replica.

Note: since the most of the code in setup_containers was touched it has been reformatted.

  • Handle AAAA records in test_ipa_dns_systemrecords_check

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

@rcritten
Copy link
Contributor

No specific comments on the core changes but there is a significant number of single to double quote changes that seem to be formatting-only.

@stanislavlevin
Copy link
Contributor Author

I've especially noted that in PR message and the commit's one:

Note: since the most of the code in setup_containers was touched it has been reformatted.

@abbra
Copy link
Contributor

abbra commented Jan 27, 2021

Yep, in that case it makes sense to do some uniformity. Thanks!

@stanislavlevin
Copy link
Contributor Author

@rcritten, if you strongly object to this I can revert extra changes, even though it was convenient for me to use black here.

@rcritten
Copy link
Contributor

I've never understood the significance of single vs double quotes nor why changing it en-mass is important. I don't really care about it other than polluting git history.

@stanislavlevin
Copy link
Contributor Author

stanislavlevin commented Jan 28, 2021

I'm for enforcing code style one or another. In this case, that was easier for me to do 'hack hack black'.
I like the minimal possible diffs too, but I've already changed this file totally. There can't be a small size diff.
This simplifies my future work here - no need to choose hunks of a patch to commit among style changes because the style changes were done.

@abbra
Copy link
Contributor

abbra commented Feb 12, 2021

I think we can merge this. LGTM.

@abbra abbra added ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 ipa-4-8 Mark for backport to ipa 4.8 labels Feb 12, 2021
IPA server's AAAA records at embedded DNS mode depend on result of
`get_server_ip_address` function(`ipaserver.install.installutils`),
which in turn, relies on NSS.

In case of Azure Pipelines, there are neither IPv6 records in
'/etc/hosts' nor external DNS, which may provide such. This leads to
the missing AAAA records for master and missing AAAA records for `ipa-ca`
pointing to master in embedded DNS.

In particular, tests `test_ipa_healthcheck_no_errors`,
`test_ipa_dns_systemrecords_check` fail with:
```
[
  {
    "source": "ipahealthcheck.ipa.idns",
    "check": "IPADNSSystemRecordsCheck",
    "result": "WARNING",
    "uuid": "b979a88a-6373-4990-bc83-ce724e9730b4",
    "when": "20210120055054Z",
    "duration": "0.032740",
    "kw": {
      "msg": "Got {count} ipa-ca AAAA records, expected {expected}",
      "count": 1,
      "expected": 2
    }
  }
]
```
where `ipa-ca` record exists only for replica.

Note: since the most of the code in setup_containers was touched it has
been reformatted.

Fixes: https://pagure.io/freeipa/issue/8683
Signed-off-by: Stanislav Levin <slev@altlinux.org>
This test assumes that the current environment has only IPv4, but
for example, Azure Pipelines provides both IPv4 and IPv6.

Fixes: https://pagure.io/freeipa/issue/8683
Signed-off-by: Stanislav Levin <slev@altlinux.org>
`update-crypto-policies` tool from RPM package `crypto-policies-scripts`
is required for tests.

Signed-off-by: Stanislav Levin <slev@altlinux.org>
@stanislavlevin
Copy link
Contributor Author

stanislavlevin commented Feb 12, 2021

@abbra, thank you!

  • rebased to master
  • removed temp commit

@abbra abbra added the pushed Pull Request has already been pushed label Feb 15, 2021
@abbra
Copy link
Contributor

abbra commented Feb 15, 2021

master:

  • 3e33e54 Azure: Populate containers with self-AAAA records
  • 596bb32 ipatests: Handle AAAA records in test_ipa_dns_systemrecords_check
  • 778ef95 rpm-spec: Require crypto-policies-scripts

@abbra
Copy link
Contributor

abbra commented Feb 15, 2021

Needs a manual backport to ipa-4-8, @stanislavlevin could you please handle that?

Applying to ipa-4-8: Azure: Populate containers with self-AAAA records
Failure to apply patches: error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch

Failed to apply patches onto origin/ipa-4-8. Manual backport is needed.
Cleaning up

@stanislavlevin
Copy link
Contributor Author

@abbra, yes, sure.

@stanislavlevin
Copy link
Contributor Author

#5555, nice PR number :)

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 ipa-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants