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

Always update auth method #282

Merged
merged 4 commits into from
Jun 24, 2020
Merged

Always update auth method #282

merged 4 commits into from
Jun 24, 2020

Conversation

ishustava
Copy link
Contributor

Previously, we would only update auth method in Consul
if Consul namespaces are enabled. This was necessary to make sure
that if namespace configuration changes, it is reflected in the auth method.

However, running Consul servers externally is another use case why it's important
to update the auth method. That is because if you run a Helm install, then uninstall
and install again, the auth method saved in Consul is out-of-date:
the service account JWT token saved in the auth method refers to the service account token
that has been deleted from Kubernetes when you ran helm uninstall.

This change proposes to always update the auth method to solve for the
external servers use case.

Previously, we would only update auth method in Consul
if Consul namespaces are enabled. This was necessary to make sure
that if namespace configuration changes, it is reflected in the auth method.

However, running Consul servers externally is another use case why it's important
to update the auth method. That is because if you run a Helm install, then uninstall
and install again, the auth method saved in Consul will be out-of-date: it will have
the service account JWT token saved in the auth method refers to the service account token
that has been deleted from Kubernetes when you ran helm uninstall.

This change proposes to always update the auth method to solve for the
external servers use case.
@ishustava ishustava added area/acls Related to ACLs theme/external-servers Related to Consul servers running outside the cluster labels Jun 23, 2020
@ishustava ishustava requested review from a team, lkysow and kschoche and removed request for a team June 23, 2020 22:42
.circleci/config.yml Outdated Show resolved Hide resolved
@lkysow
Copy link
Member

lkysow commented Jun 23, 2020

This one took me a bit to grok. For other reviewers, what's happening is when running with external servers, e.g. Consul on Azure, when you helm install for the first time, we create a Consul auth method that Consul uses to exchange service account tokens for consul acl tokens. The auth method works by Consul servers calling the Kubernetes API and checking if the service account is valid. In order for consul to call the kube api, it itself needs a service account token. On first install, this service account token is stored in the Consul servers.

When the consul installation is deleted, we don't delete the auth method from the consul servers since we're just uninstalling the consul clients in kube. However we do delete the service account resource from Kubernetes. This means the consul servers now have an auth method that's trying to use a token that was deleted from Kubernetes.

On re-install, prior to this PR, we would recognize that the auth method exists and not update it. It would then still have the now-deleted service account token and so any consul login commands (used by all connect services) would fail because when consul tried to make the API call to kube to check if the service account was valid, kube would reject that call.

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looks good, I haven't run the code, lmk if you'd like me to.

subcommand/server-acl-init/connect_inject.go Show resolved Hide resolved
subcommand/server-acl-init/command_test.go Show resolved Hide resolved
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

looks great!

@ishustava
Copy link
Contributor Author

lmk if you'd like me to.

thanks! I think it's OK for just myself to do the testing. I've tested this with HCS, and it works nicely!

@ishustava ishustava merged commit df99da1 into master Jun 24, 2020
@ishustava ishustava deleted the always-update-auth-method branch June 24, 2020 20:56
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Prevent incorrect connectInject config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acls Related to ACLs theme/external-servers Related to Consul servers running outside the cluster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants