-
Notifications
You must be signed in to change notification settings - Fork 102
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added Ability to use Access Tokens #461
Added Ability to use Access Tokens #461
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, @bradkwadsworth-mw!
I've added some small comments and questions, but its already looking pretty good to me, congrats!
After these fixes, we can merge it 🙂
Also, we need to merge it into master instead of the 0.21 release. This feature will be included in the 0.22 release (that are coming probably in the next week) |
f4e27ab
to
884ac1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to run the tutorial with a local cluster and it didn't work (I think that's expected because in the tutorial we are using the GKE workload identity).
Thinking about that, isn't it better to build a tutorial agnostic of which managed kubernetes service is powering the cluster? In this way, the same tutorial could be used for provider-gcp running locally, in EKS, AKS and GKE. Probably if someone wants to use provider-gcp and authenticate with Access Token without using GKE, he will need to discover how to do the same kind of setup in this tutorial in another cloud provider. We could offer that solution before hand. What do you think?
We don't need to remove the current tutorial, instead we can split the "Authenticating with Access Tokens" topic in two subtopics: "Using GKE as control plane" and "Without using GKE as control plane".
One thing that I think would be beneficial to add to the "Using GKE as control plane" subtopic is a brief note describing the minimal configuration of the GKE cluster to work with the Access Token authentication and the CronJob setup that you made. For example:
The GKE cluster should have the following configuration: Workload Identity=Enabled and [...]
I ask for that because I tried to run the tutorial with a sample GKE cluster, but I haven't sorted it out how it should be configured to work and I couldn't test the functionally. The error that caught me was when I were trying to create any managed resource referencing the ProviderConfig, the Events were throwing 403 error with the message ACCESS_TOKEN_SCOPE_INSUFFICIENT
(but the GCP ServiceAccount had enough IAM roles to create it). By giving this brief note of minimal configuration of the GKE cluster, I think this error will be solved.
I understand that building this subtopic "Without using GKE as control plane" isn't strictly necessary to ship this feature, so I'm happy to merge this PR when the current tutorial is 100% if you don't want to write this additional subtopic right now. We can create another issue to track the status of this new subtopic.
cmd/provider/main.go
Outdated
@@ -40,6 +40,7 @@ import ( | |||
scv1alpha1 "github.com/crossplane-contrib/provider-gcp/apis/v1alpha1" | |||
gcp "github.com/crossplane-contrib/provider-gcp/pkg/controller" | |||
"github.com/crossplane-contrib/provider-gcp/pkg/features" | |||
_ "k8s.io/client-go/plugin/pkg/client/auth/gcp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this import? It isn't clear to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIthout this import, running make run
failed for me. I believe this is due to google changing the authentication mechanism for GKE. After some research, I found that other people used this solution to get their controllers to work while testing outside of the cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the tutorial comment, I agree we should specify the requirements for GKE. I think this solution will only work if your controlplane is running in GKE with workload identity. Otherwise, the CronJob would need some sort of GCP service account key I assume which kind of defeats the whole purpose. I will work on some disclaimers in the documentation and push another commit to this PR.
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
This will allow make run to work without errors Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
f2026ed
to
5866d0e
Compare
@Feggah pushed a fix for Go lint. |
Hey @bradkwadsworth-mw, I was waiting for the part of the documentation that you said you would add:
After that I believe we can merge the PR. I just want to test it out, as I wasn't able to setup the GKE properly to test this feature |
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Oops, your are right. Just pushed a new commit with a disclaimer doc update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just want to ask if you can add minimal configuration for the GKE (prerequisites) for this feature to work.
I tried to create a GKE cluster with default values in the GCP interface, and by doing this I couldn't manage to make this work. Can you share a gcloud container clusters create
command or a list of requirements in the AUTHENTICATION.md documentation to make it easier to test this PR?
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Signed-off-by: Brad Wadsworth <brad.wadsworth@mavenwave.com>
Added some info for standing up a GKE cluster with Workload identity enabled. I tested it a few times and had another person test as well so I'm fairly confident it is repeatable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, @bradkwadsworth-mw! Thanks for your effort
Signed-off-by: Brad Wadsworth brad.wadsworth@mavenwave.com
Description of your changes
This change will read the secret key in a Kubernetes secret and if it is not in a JSON format, it will treat it as a Access Token. The documentation has also been updated to demonstrate how a CronJob resource can keep the Access Token up to date
Fixes #460
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
Tested the change in a Kind cluster using the update
docs/AUTHENTICATION.md
.