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

pin aws-sdk-go-v2 dependency versions for ClusterAuth presign breaking change #1251

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

erhancagirici
Copy link
Collaborator

@erhancagirici erhancagirici commented Apr 3, 2024

Description of your changes

Fixes #1248
Recent patch version bumps at the following aws-sdk-go-v2 modules seems to break the request signature of the token for the generated kubeconfig for ClusterAuth resource.

github.com/aws/aws-sdk-go-v2
github.com/aws/aws-sdk-go-v2/internal/configsources
github.com/aws/aws-sdk-go-v2/internal/endpoints/v2

This change reverts the version bumps and pins the versions.
Adds a separate standalone example manifest for ClusterAuth resource and added an Uptest post-assert hook for testing the resulting kubeconfig, by executing simple kubectl commands to ensure the cluster authentication.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

  • Tested manually with ClusterAuth MR example, that fails on 1.3.0
  • With Uptest using the new post-assert check for the resulting kubeconfig

…g change

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@jeanduplessis
Copy link
Collaborator

Is it possible to add a test to catch a regression here in the future?
Is ClusterAuth already uptestable and we should consider it a critical component that should always be tested before a release?

@erhancagirici
Copy link
Collaborator Author

Is it possible to add a test to catch a regression here in the future? Is ClusterAuth already uptestable and we should consider it a critical component that should always be tested before a release?

ClusterAuth is already uptestable, and included in the examples/v1beta1/cluster.yaml. The MR actually gets created, switches to ready & synced state, and the connection details are published. In order to catch the regression here, the resulting kubeconfig in the connection details should be consumed.

We might consider utilizing https://github.com/upbound/configuration-aws-eks, some test MRs utilizing provider-kubernetes or some post-test script utilizing kubectl. cc @ulucinar @sergenyalcin

@ulucinar
Copy link
Collaborator

ulucinar commented Apr 3, 2024

Hi @erhancagirici,
We may consider utilizing an uptest post-assert-hook for ClusterAuth.eks in which we run a script that:

  • Dumps the kubeconfig from the K8s connection secret onto the file system
  • Runs a kubectl command against that kubeconfig

Could you please also trigger an uptest on ClusterAuth.eks with these modifications? We may also consider adding the example manifest examples/eks/v1beta1/clusterauth.yaml.

…nfig

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/eks/v1beta1/clusterauth.yaml"

Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/eks/v1beta1/clusterauth.yaml"

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @erhancagirici, lgtm.

examples/eks/v1beta1/testhooks/check-clusterauth.sh Outdated Show resolved Hide resolved
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
@erhancagirici
Copy link
Collaborator Author

/test-examples="examples/eks/v1beta1/clusterauth.yaml"

@erhancagirici erhancagirici merged commit d02d728 into main Apr 4, 2024
11 of 12 checks passed
Copy link

github-actions bot commented Apr 4, 2024

Successfully created backport PR #1257 for release-1.3.

@ulucinar ulucinar deleted the fix-clusterauth-wrong-signature branch April 4, 2024 14:19
@mbbush
Copy link
Collaborator

mbbush commented Apr 21, 2024

It looks like this was probably caused by aws/aws-sdk-go-v2#2438. I see a bunch of other projects citing that PR as an issue too. I don't understand why yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: EKS ClusterAuth v1.3.0 kubeconfig results in Unauthorized
4 participants