-
Notifications
You must be signed in to change notification settings - Fork 704
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
feat(secrets): add support for decrypting kube configs #1207
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.
I think the name is what's throwing me here. That, and not following all of the specifics in the changes thus far.
@@ -51,13 +46,14 @@ | |||
import java.util.Map; | |||
|
|||
@Slf4j | |||
@Component |
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'm personally not a fan of making a class with Utils
in the name a field. I worked on a previous project that had objects, each with their own Utils object and it didn't mesh well in my head.
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.
This seemed like the cleanest workaround to avoid the static autowire issue explained above. Another kind of clunky alternative is to leave secretSessionManager
as static and add a class that creates an autowired secretSessionManager
and sets the static one in KubernetesV2Utils
like this:
@Component
public class StaticSecretSessionManagerInitializer {
@Autowired
private SecretSessionManager secretSessionManager;
@PostConstruct
public void init() {
KubernetesV2Utils.setStaticSecretSessionManager(secretSessionManager);
}
}
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.
As for the specifics in the changes: So far all secrets are being decrypted by autowiring a SecretSessionManager
which contains the decrypt methods. This works well in all cases but kubeconfig decryption. The problematic spot is in KubernetesV2Utils
, since the methods are all static and since we can't call the autowired SecretSessionManager
from a static context, we settled on making the methods non-static.
In most of the places KubernetesV2Utils
is used, we could just replace the static calls with an autowired kubernetesV2Utils
. The one where this didn't work (and where it probably gets confusing) is in KubernetesV2Service
since it's an interface. So there, we use the autowired SecretSessionManager
that's already hooked up in KubernetesV2Executor
.
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.
Agree that having Utils
in the name is not ideal but that predates the secrets work and fixing the naming should be out of scope for this PR.
Part of secrets management project: spinnaker/spinnaker#3649
So far, we've been adding an autowired SecretSessionManager to deal with decryption, but kube configs are handled in static methods in KubernetesV2Utils. Since you can't static autowire, it seems the cleanest solution is to make those methods non-static and make adjustments in the couple classes that call them.
We also have to remove the static autowire in Kubernetesv1Utils, so secrets in v1 kubernetes is unsupported for now.