Skip to content

Conversation

@xWink
Copy link
Member

@xWink xWink commented Dec 22, 2023

What type of PR is this?

feature + tests

Which issue does this PR fix:
#567

What does this PR do / Why do we need it:

  • Updated Gateway Controller to appropriately set the Programmed status of Gateways as Conflicted when there is a name conflict
    • Previously, the status would be left as Pending
  • Added new cross-account e2e test suite, named sharing
    • Not executed as part of normal e2e-test suite due to different environment expectations and testing goals
  • Added new environment variable SECONDARY_ACCOUNT_TEST_ROLE_ARN, which is used by the new test suite to assume the secondary account, which currently just shares resources to the primary account
    • In the future, the secondary account may be expected to have its own cluster, which would enable multi-cluster e2e testing in the new suite
  • Added new makefile target sharing-test, which runs the new suite
  • Added docs explaining how to use the sharing suite

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
N/A

Testing done on this change:

Ran 3 of 3 Specs in 118.312 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (118.31s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/sharing       119.038s

Automation added to e2e:

sharing suite

Will this PR introduce any new dependencies?:

No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this PR introduce any user-facing change?:

No

Added cross-account e2e test suite

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@mikhail-aws mikhail-aws left a comment

Choose a reason for hiding this comment

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

Status updates for gateway require more work and thought, existing code is not in good shape and new code does not make it better. I suggest move it into different PR.

Integ-tests have lots of duplicated code starting from Makefile, framework setup, and tests cases it-self. If you abstract same code into well defined functions, and re-use existing code as much as you can this PR would be under 200 lines of code.

@mikhail-aws
Copy link
Contributor

Special highlight on duplicated code - last time when I did refactoring for FindServiceNetwork I had hard times with accesslog policy tests, surprisingly. We should take it more serious.

…ing Gateway name conflict, general code cleanliness improvements
@xWink
Copy link
Member Author

xWink commented Dec 27, 2023

New E2E tests still pass:

Ran 3 of 61 Specs in 124.753 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 58 Skipped
--- PASS: TestIntegration (124.76s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   126.001s

Copy link
Contributor

@zijun726911 zijun726911 left a comment

Choose a reason for hiding this comment

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

Thanks to add new e2e tests! overall LGTM!

@xWink xWink force-pushed the ram-e2e-test branch 3 times, most recently from 60c4f7d to 12148b2 Compare December 29, 2023 19:43
Copy link
Contributor

@mikhail-aws mikhail-aws left a comment

Choose a reason for hiding this comment

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

Nice, thank you

@xWink xWink merged commit e77468b into aws:main Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants