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 gke & aws auth plugins to prow images #28277

Merged
merged 8 commits into from
Jan 20, 2023

Conversation

upodroid
Copy link
Member

Part of #28142
Part of #27896

I might run in to problems with multi-arch builds. aws-iam-authenticator doesn't have s390x and ppc64le releases :(

The google cloud sdk docker image is only available for amd64 :(
https://hub.docker.com/r/google/cloud-sdk/tags

/cc @ameukam @BenTheElder @dims

My plan is:

  • merge this PR
  • bump the alpine version the git image version is built from
  • bump all the images defined in .ko.yaml
  • Start changing the kubeconfig that we use and get rid of gencred

@k8s-ci-robot k8s-ci-robot added area/images sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2022
@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 19, 2022
@dims
Copy link
Member

dims commented Dec 19, 2022

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2022
@ameukam
Copy link
Member

ameukam commented Dec 19, 2022

Deferring to @chaodaiG & @cjwagner for LGTM.

@cjwagner
Copy link
Member

We were going to switch to using a different base image. If we want to include more providers I think it makes sense to update the alpine (and indirectly git) image directly like you are doing, but lets align these two efforts. #28192 @dieseldesai

@dieseldesai
Copy link
Contributor

There's a lot of components that use the alpine build, either directly or through the git image. AFAICT only 4 of them need access to the build cluster and so would need the gke/aws authentication plugins. Do we still want to update all the component images?

For gke-gcloud-auth-plugin, I ran into issues building for s390x. However, since I am able to build the image locally, my guess is the gcb-docker-gcloud image needs to have its docker version updated. I'll experiment locally and see if I can confirm this hypothesis

@upodroid
Copy link
Member Author

Hmm

How about modifying @dieseldesai image instead but just build amd64 and arm64 for the components that actually interact with external Kubernetes clusters? I can rename the image to git-custom-k8s-auth. We can insert all the binaries for the k8s clusters used by prow admins.

We can blame the limited platform support on the tools not releasing them.

@dieseldesai
Copy link
Contributor

That sounds reasonable to me, but would probably want @cjwagner or @chaodaiG to sign off. I'm not sure if those other platforms are being actively used or if there would need to be a deprecation period for them.

@dieseldesai
Copy link
Contributor

I've got the GKE auth plugin working now for all 4 architectures:
https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/post-test-infra-push-git-gke-gcloud-auth/1605613943705833472.

For this PR, maybe we can rebase and update the git-gke-gcloud-auth image to also install aws authenticator? Perhaps rename it to git-gke-aws-auth or something like that

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 22, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 22, 2022
@upodroid
Copy link
Member Author

This can be merged now.

I used @dieseldesai's image and added the aws-iam-authenticator to it.

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Dec 22, 2022
@cjwagner
Copy link
Member

This can be merged now.

Were the platform issues addressed? If this change will limit the platforms supported by Prow component images we cannot merge this without more discussion as I'd expect some sort of deprecation period or a reasonable assurance that these platforms are not in use.

@BenTheElder
Copy link
Member

Switching to these is not optional as kubectl will not work without auth plugins going forward. If people are interested in these platforms they need to go work with the auth plugin projects.

@upodroid
Copy link
Member Author

upodroid commented Jan 7, 2023

This is ready now. The images are built successfully. I ran the cloudbuild job in my dev project.

I still think we shouldn't build images for platforms that the tools don't have native builds.

I found this doc helpful https://github.com/projectcalico/go-build/blob/master/README.md#cross-runnning-binaries-binfmt

@dims
Copy link
Member

dims commented Jan 18, 2023

/assign @cjwagner @BenTheElder

@cjwagner
Copy link
Member

cjwagner commented Jan 18, 2023

Switching to these is not optional as kubectl will not work without auth plugins going forward.

Well this is almost as good as an assurance that the problematic platforms are not in use. If they are in use they'll be broken soon anyways so this doesn't seem like as big of a deal.

I'm a bit concerned about gcloud being needed somewhere.

@dieseldesai
Copy link
Contributor

Can we verify that the gke-gcloud-auth-plugin works without the gcloud binary? It would be nice to spin up a test container and just verify if you can auth to a GKE cluster.

@upodroid
Copy link
Member Author

Yes it does.

 REDACTED  C02CW0CDP3YY  ~  Desktop  Git  serving   use-adc-dns-autotls  USAGE  $  gke-gcloud-auth-plugin --use_application_default_credentials
{
    "kind": "ExecCredential",
    "apiVersion": "client.authentication.k8s.io/v1beta1",
    "spec": {
        "interactive": false
    },
    "status": {
        "expirationTimestamp": "2023-01-20T19:29:20Z",
        "token": "REDACTED"
    }
}

@upodroid
Copy link
Member Author

Can I get lgtm from someone please?

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, dims, upodroid

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 merged commit d66b9eb into kubernetes:master Jan 20, 2023
@upodroid
Copy link
Member Author

upodroid commented Mar 7, 2023

The image failed to build :(

I opened #28931 to try it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/images 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. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants