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

Add test for invalid credentials in dns provider #520

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

averevki
Copy link
Contributor

Closes #519

@averevki averevki added the Test case New test case label Aug 27, 2024
@averevki averevki self-assigned this Aug 27, 2024
@trepel
Copy link
Contributor

trepel commented Aug 28, 2024

@averevki this test looks good but I failed to make it pass on kua-stage-single today. First the DNSPolicy message was 'DNSPolicy has encountered some issues: policy is not enforced on any DNSRecord: no routes attached for listeners'

So I made sure that HTTPRoute is also created as part of the test. Then the record condition looked like:

  recordConditions:
    '*.5qdpemu.aws.kua.app-services-dev.net':
      - lastTransitionTime: '2024-08-28T13:36:02Z'
        message: "Unable to find suitable zone in provider: failed to list hosted zones, InvalidClientTokenId: The security token included in the request is invalid.\n\tstatus code: 403, request id: 548c40ce-29aa-4b24-ac3b-77a495590b66"
        observedGeneration: 1
        reason: DNSProviderError
        status: 'False'
        type: Ready

So I think that DNSProviderError should be used as Reason.

However, I was not able to verify this because I was unable to get to that point again for some reason. No idea why. Both Gateway and DNSPolicy were updated constantly by operators and DNSPolicy was saying that:
'DNSPolicy has encountered some issues: reconcile DNSRecords error error reconciling dns records for gateway gw-trepel--f41m: Operation cannot be fulfilled on dnsrecords.kuadrant.io "gw-trepel--f41m-api": the object has been modified; please apply your changes to the latest version and try again'

@trepel
Copy link
Contributor

trepel commented Aug 28, 2024

Also, when running the test I noticed that the GW reporting errors related to TLS setup - TLS policy is not committed at all. Not sure if that makes a difference, I fixed this too but it did not help me make this test pass.

@trepel
Copy link
Contributor

trepel commented Aug 29, 2024

However, I was not able to verify this because I was unable to get to that point again for some reason. No idea why. Both Gateway and DNSPolicy were updated constantly by operators and DNSPolicy was saying that:
'DNSPolicy has encountered some issues: reconcile DNSRecords error error reconciling dns records for gateway gw-trepel--f41m: Operation cannot be fulfilled on dnsrecords.kuadrant.io "gw-trepel--f41m-api": the object has been modified; please apply your changes to the latest version and try again'

I was not able to reproduce this today. Strange. But my other two comments seem to be valid:

  1. HTTPRoute needs to be created
  2. DNSProviderError should be set as 'Reason' parameter to has_record_condition

@trepel
Copy link
Contributor

trepel commented Aug 29, 2024

Slightly unrelated but can you think of an easy way to implement so that there is clear what the DNSPolicy status was and what the expected status is when the last assertion fails? I can only see what is expected but not really the actual status.

@averevki
Copy link
Contributor Author

Slightly unrelated but can you think of an easy way to implement so that there is clear what the DNSPolicy status was and what the expected status is when the last assertion fails? I can only see what is expected but not really the actual status.

Do you mean to check for successful enforcement first and then provoke the error status? Or just add comments to the assertions?

@trepel
Copy link
Contributor

trepel commented Aug 29, 2024

Slightly unrelated but can you think of an easy way to implement so that there is clear what the DNSPolicy status was and what the expected status is when the last assertion fails? I can only see what is expected but not really the actual status.

Do you mean to check for successful enforcement first and then provoke the error status? Or just add comments to the assertions?

What I meant was that the test failed for me on that last assertion and it was not clear from the test log what the actual dnspolicy status was. It would be nice if we could see both what the expected status of dns policy and actual status of dns policy were.

@averevki
Copy link
Contributor Author

@trepel assert message should be clearer now. Although I see we have this problem in more places where it can be improved later

trepel
trepel previously approved these changes Aug 30, 2024
Copy link
Contributor

@trepel trepel left a comment

Choose a reason for hiding this comment

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

LGTM now. Not sure how stable this test will be given the constant DNSPolicy CR status updates but there is no way of knowing beforehand. It passed for me, also the assertion output in case of failure is much more informative. I think this is good to go, we can skip it if it fails in nightlies too frequently.

@averevki
Copy link
Contributor Author

averevki commented Sep 9, 2024

LGTM now. Not sure how stable this test will be given the constant DNSPolicy CR status updates but there is no way of knowing beforehand. It passed for me, also the assertion output in case of failure is much more informative. I think this is good to go, we can skip it if it fails in nightlies too frequently.

Constant status updates should be fixed now 👍 (Kuadrant/dns-operator#218)

jsmolar
jsmolar previously approved these changes Sep 12, 2024
@averevki
Copy link
Contributor Author

rebase^

@jsmolar jsmolar merged commit aa8594d into Kuadrant:main Sep 17, 2024
3 checks passed
@averevki averevki deleted the zone-provider-test branch September 17, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Test case New test case
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad DNS provider configuration test
3 participants