-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fixing issues with GCR #34
Conversation
39c9aa9
to
44333b5
Compare
44333b5
to
6836de8
Compare
I'm hitting this right now. Is there no way currently to use this addon without having AWS creds... I don't have any and am trying to use minikube with gcr. |
This seems to not try GCR after failing the authentication with AWS creds that are invalid. |
@@ -307,7 +307,7 @@ func validateParams() providerConfig { | |||
var ecrEnabled bool | |||
|
|||
awsAccountID = os.Getenv("awsaccount") | |||
if len(awsAccountID) == 0 { | |||
if len(awsAccountID) == 0 || awsAccountID == "changeme" { |
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.
Do you think we should merge the secrets? Then make it more explicit which cloud is desired? I want to get everyone working, but may refactor to have it try and work with any cloud at the same time (or both).
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.
Yes, I think that merging the secrets is the best approach as all secrets are required for the pod to load. I am debating what is the best way to configure the "mode" though. A config map perhaps?
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.
Oh you did merge to the same! I missed that! I wonder if there's a simple way to "walk" the user through setting it up. Like when you enable the add-on from minikube, it would prompt for the right info and create the secret / configmap behind the scenes.
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.
That is a great idea for minikube, would make it much easier to use! As far as using the addon standalone, I think that adding a config map w/ "mode" is the best solution for now.
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.
Ok I can work on moving that into a configmap, and cleaning up the code, unless you've got something started or want to tackle.
I want to get this working @tsloughter first in this repo, then will look to incorporate into minikube which is what we had originally discussed.
kubernetes.io/minikube-addons: gcr-creds | ||
type: Opaque | ||
``` | ||
2. Input your application_default_credentials.json information into the secret.yaml template located [here](k8s/secret.yaml#L17): |
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.
On my mac, the application_default_credentials.jsonis empy, but my creds are in
credentials`. Is this consistent?
@aaron-prindle I'm going to merge this then fix one more thing, I think I see why when AWS fails then the whole loop bombs. Just need to handle the error better. I'd rather just have it try both and if one fails then it will still process the other. Just curious about the location of the default credentials mentioned above for the docs. |
The replicationController.yaml was not properly updated in my original PR. I have added the necessary changes to make the secret mount properly with GCR. For now both secrets need to be added for using either AWS or GCR, we can try to support these being optional with:
https://kubernetes.io/docs/user-guide/secrets/#optional-secrets-from-environment-variables
but this is relatively new so the workaround is that both secrets are required. Currently if either the registry-creds or gcr-creds secret is missing, the pod will not initialize. I have changed the secret.yaml template file to have dummy data for both.