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

Make AWS credentials configurable easily #10868

Merged
merged 1 commit into from
Jan 22, 2019
Merged

Make AWS credentials configurable easily #10868

merged 1 commit into from
Jan 22, 2019

Conversation

shyamjvs
Copy link
Member

Follow-up of #10866
This change standardizes way we pass credentials to kops/eks jobs - allowing to do testing in newer accounts.

/cc @gyuho

@k8s-ci-robot k8s-ci-robot requested a review from gyuho January 21, 2019 23:13
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 21, 2019
@gyuho
Copy link
Member

gyuho commented Jan 21, 2019

Ok this is more readable 👍

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 21, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 543fbdf5bec90392425c253c18c3ed61636e29ec

shyamjvs
shyamjvs commented Jan 21, 2019
View reviewed changes
config/jobs/kubernetes-security/generated-security-jobs.yaml Outdated
@@ -10,7 +10,7 @@ presubmits:
context: pull-security-kubernetes-e2e-aws-eks-1-11-correctness
labels:
preset-kubernetes-e2e-aws-eks-1-11: "true"
preset-kubernetes-e2e-aws-eks-common: "true"
preset-aws-credential: "607362164682"
Copy link
Member Author

Choose a reason for hiding this comment

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

Btw - the label value is our testing account's ID. We can add new presets with different accounts.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this will be error prone as nearly all of us (including me) don't have access to and have no idea what these are so we'll just be copying around mysterious number strings. I'd strongly suggest giving these a human readable name and commenting with the ID# on the preset instead.

Doing that would also mean you can swap accounts without mass updating all of the job config files, which is a big reason presets are useful.

Copy link
Member

Choose a reason for hiding this comment

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

Something like:
preset-aws-credential: cncf-prow
preset-aws-credential: aws-eks

Which will also be useful because eventually @kubernetes/k8s-infra-wg will probably want to track usage on the CNCF credentials.

Copy link
Member

Choose a reason for hiding this comment

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

Naming them with intent should also help job writers to figure out which they should be using from example jobs. The opaque strings will probably lead to someone creating a charts job on the wrong account or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

@BenTheElder Sounds reasonable.. Will change it to something more meaningful, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, PTAL.

@shyamjvs shyamjvs force-pushed the make-aws-creds-configurable branch from f866af5 to 60a9ee0 Compare January 21, 2019 23:28
@k8s-ci-robot k8s-ci-robot added area/prow Issues or PRs related to prow and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 21, 2019
@shyamjvs
Copy link
Member Author

Presubmit failed due to duplicated preset label : preset-aws-credential. I changed the validation function to check uniqueness of (key, value) pair instead of uniqueness of just key - which is what should be done FMU.

/cc @BenTheElder

@shyamjvs shyamjvs force-pushed the make-aws-creds-configurable branch 7 times, most recently from e0b6b36 to dafe48e Compare January 22, 2019 20:15
BenTheElder
BenTheElder reviewed Jan 22, 2019
View reviewed changes
config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-presets.yaml
labels:
preset-kubernetes-e2e-aws-eks-common: "true"
preset-aws-credential: "aws-oss-testing"
Copy link
Member

Choose a reason for hiding this comment

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

can we put a comment describing what the usage should be?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Added comment above preset definition.

Copy link
Member

Choose a reason for hiding this comment

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

thanks, this should be a nice step towards being easier to follow the config :-)

@shyamjvs shyamjvs force-pushed the make-aws-creds-configurable branch from dafe48e to 9d607b5 Compare January 22, 2019 22:39
BenTheElder
BenTheElder reviewed Jan 22, 2019
View reviewed changes
config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-presets.yaml
- name: AWS_K8S_TESTER_EKS_AWS_IAM_AUTHENTICATOR_DOWNLOAD_URL
value: https://amazon-eks.s3-us-west-2.amazonaws.com/1.11.5/2018-12-06/bin/linux/amd64/aws-iam-authenticator
# AWS test account credential mounted path, required for AWS API call
# Credentials for using AWS test account 607362164682.
Copy link
Member

Choose a reason for hiding this comment

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

will leave for a follow-up, but perhaps the intention of who should be using this account for what kind of testing? :^)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm soon planning to organize all aws testing account creds at a central place under jobs/sig-aws/. Working with @krzyzacy to get the prow secrets right. I'll clarify it in follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #10886

BenTheElder
BenTheElder approved these changes Jan 22, 2019
View reviewed changes
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, gyuho, shyamjvs

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
Copy link
Contributor

LGTM label has been added.

Git tree hash: 650d8d00cb430b39462c8e900d9a8e862aff351a

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2019
@k8s-ci-robot k8s-ci-robot merged commit f1358bc into kubernetes:master Jan 22, 2019
@k8s-ci-robot
Copy link
Contributor

@shyamjvs: Updated the job-config configmap using the following files:

  • key k8s-aws-eks-presets.yaml using file config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-presets.yaml
  • key k8s-aws-eks-presubmits.yaml using file config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-presubmits.yaml
  • key generated-security-jobs.yaml using file config/jobs/kubernetes-security/generated-security-jobs.yaml
  • key k8s-aws-eks-periodics.yaml using file config/jobs/kubernetes/sig-aws/eks/k8s-aws-eks-periodics.yaml

In response to this:

Follow-up of #10866
This change standardizes way we pass credentials to kops/eks jobs - allowing to do testing in newer accounts.

/cc @gyuho

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@shyamjvs shyamjvs deleted the make-aws-creds-configurable branch January 22, 2019 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenTheElder BenTheElder BenTheElder approved these changes

@gyuho gyuho Awaiting requested review from gyuho

Assignees

@BenTheElder BenTheElder

@gyuho gyuho

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/prow Issues or PRs related to prow 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shyamjvs @gyuho @k8s-ci-robot @BenTheElder