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.

Sign up for GitHub

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

Jump to bottom

Integration test framework #395

Merged
merged 1 commit into from
Sep 27, 2021
Merged

Integration test framework #395

merged 1 commit into from
Sep 27, 2021

Conversation

nckturner
Copy link
Contributor

@nckturner nckturner commented Sep 16, 2021 •
edited
Loading

Framework to run integration tests:

  • Uses kubernetes integration test framework
  • Shallow clones k/k into a local directory for importing into tests to avoid complexity
    with generated code.
  • Adds a test which adds a config map entry and attempts to authenticate.
  • The test currently expects to be run on an EC2 instance because the authenticator server cannot start otherwise. I'd like to get rid of this dependency.
  • The diff currently does not include go.mod or vendor changes. Should we continue vendoring dependencies? I'm not sure what what the original reasoning for vendoring was but I'd like to avoid it if we don't have any hard dependencies on it.
  • I made some changes to config/certs and config/kubeconfig to make it more testable, and added pkg/testutils with helper functions to set up the test environment.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 16, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nckturner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 16, 2021
@wongma7
Copy link
Contributor

wongma7 commented Sep 16, 2021

The test currently expects to be run on an EC2 instance because the authenticator server cannot start otherwise. I'd like to get rid of this dependency.

why is this, do we need instance metadata for some reason?

wongma7
wongma7 reviewed Sep 16, 2021
View reviewed changes
Copy link
Contributor

@wongma7 wongma7 left a comment

Choose a reason for hiding this comment

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

I don't understand the cert loading changes otherwise lgtm

.gitignore Outdated Show resolved Hide resolved
cmd/aws-iam-authenticator/server.go Outdated Show resolved Hide resolved
tests/integration/server/main_test.go Outdated Show resolved Hide resolved
tests/integration/server/server_test.go Outdated Show resolved Hide resolved
pkg/testutils/testserver.go Outdated
t.Fatal(err)
}

if _, err := cfg.GetOrCreateCertificate(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

where do these files get written? to pwd ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They get written to the "stateDir".

pkg/testutils/testserver.go
return cfg, errors.New("Invalid partition")
}

if errs := mapper.ValidateBackendMode(cfg.BackendMode); len(errs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this and checking cfg.ClusterID necessary, shouldn't the server do these checks for us when you start it? when you call mapper.Start it should crap out if you provide an invalid one right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It calls ValidateBackendMode() in cmd/aws-iam-authenticator/root.go, so while I'm not sure if its strictly necessary for a test server, I'm just copying the getConfig() function there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to move BuildMapperChain() from cmd to pkg, as I'm not using cmd in the integration tests at this point and I'd rather not. Maybe I can do the same for ValidateBackendMode().

pkg/testutils/testserver.go Outdated Show resolved Hide resolved
@nckturner
Copy link
Contributor Author

why is this, do we need instance metadata for some reason?

I should say we need some type of AWS credentials for the initialization of the ec2Provider, if I remember correctly. Haven't looked too closely at the dependency yet though.

@nckturner nckturner force-pushed the integration-simpler branch 4 times, most recently from 815982f to 0afdcd0 Compare September 25, 2021 01:17
@nckturner
Integration test framework
08c6ce5 
Framework to run integration tests:
- Uses kubernetes integration test framework
- Shallow clones k/k into a local directory
  for importing into tests to avoid complexity
  with generated code.
- Adds a test which adds a config map entry and
  attempts to authenticate.
- The test currently expects to be run on an EC2
  instance because the authenticator server cannot
  start otherwise.  I'd like to get rid of this dependency.
- Makes config/certs and config/kubeconfig
  more testable, and adds pkg/testutils
  with helper functions to set up the test environment.
@wongma7
Copy link
Contributor

wongma7 commented Sep 27, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit 63457f0 into kubernetes-sigs:master Sep 27, 2021
@nckturner nckturner mentioned this pull request Sep 27, 2021
Merged
@jyotimahapatra jyotimahapatra mentioned this pull request Feb 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wongma7 wongma7 wongma7 left review comments

@justinsb justinsb Awaiting requested review from justinsb

@micahhausler micahhausler Awaiting requested review from micahhausler

Assignees

@wongma7 wongma7

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nckturner @k8s-ci-robot @wongma7