feat(k8s): dynamic account config #3824

ethanfrogers · 2019-06-27T16:00:53Z

implement credential synchronization for kubernetes accounts. defines
CredentialsInitializerSynchronizable on the
KubernetesV*ProviderConfig classes. Each of these classes is
responsible for handling accounts for each provider version.

ethanfrogers · 2019-06-27T16:02:53Z

@scottfrederick @ezimanyi submitting a new PR for feedback. It was easier to implement @scottfrederick suggestions on a wholly new branch.

Per his feedback, I've implemented credential synchronization on the 2 provider config classes to prevent the circular dependency between the synchronizer and CatsModule. This seems much cleaner to me.

I'm still not sure what the ScheduledExecutorService is used for in KubernetesV2ProviderConfig as that class does not implement Runnable. Perhaps it's unnecessary? Any insight here would be helpful!

ethanfrogers · 2019-06-27T16:08:04Z

Oh, one more thing I forgot to mention - each time credentials are refreshed we go through the whole check permissions process unless checkPermissionsOnStartup is set to false. As noted in the previous PR, this is a pretty lengthy process which may prove troublesome with a large number of accounts. One one hand, we should do this in the event that permissions on the service account have changed.

scottfrederick · 2019-06-27T20:31:00Z

This seems much cleaner to me.

Me too! Aside from some cleanup (which I assume is why it's still WIP), this looks like it should work.

It looks like you'll need equals() and hashcode() methods in KubernetesNamedAccountCredentials to get the detection of changed accounts to work.

ethanfrogers · 2019-06-28T12:05:08Z

@scottfrederick I started working on that in the next commit but got distracted by something else. I'll either have it up today or next week.

ethanfrogers · 2019-06-28T14:35:45Z

I ran into a bit of a blocker while implementing the equals and hashCode methods. In order to properly determine if the account has changed you need to compare the underlying credentials (KubernetesV2Credentials) which means that you need the full list of omitKinds and namespaces which, unless defined explicitly, are collected from the Kubernetes API, which is a lengthy process today. In fact, this is one of the exact problems this feature is meant to overcome - the long startup time of Clouddriver. I think we need to address that before this really makes sense. If we don't then it will take the same amount of time it takes Clouddriver to boot.

cc @ezimanyi

ethanfrogers · 2019-06-28T16:56:40Z

With respect to the above comment - while the initialization of credentials is a performance problem it isn't necessarily a blocker to implementing this functionality (dynamic accounts) but more of a tradeoff that can be addressed in a subsequent PR. I'll ensure that this PR is cleaned up and ready to merge before addressing that issue.

ethanfrogers · 2019-06-28T19:01:23Z

@ezimanyi @scottfrederick I think this is ready for an official review!

scottfrederick

@ethanfrogers I ran into something when testing this. When kubeconfigFile is a reference to an external file like configserver:kubeconfig (after the PR to support that was merged), KubernetesV2Credentials#equals() will never return true, and therefore KubernetesNamedAccountCredentials#equals() will never return true, and therefore the account sync logic sees the account as "changed" on each refresh.

The root cause of this is that in this scenario each new KubernetesNamedAccountCredentials results in a new temp file being generated with a unique name.

It seems like it would be important for the account sync to know if the kubeconfigFile value were changed on refresh. Maybe ConfigFileService should create a file with a known name in the system tmp dir instead of generating a random file name each time? Kubernetes provider is the only thing using the code path through ConfigFileService that writes the config server file contents to a local file in the container.

scottfrederick · 2019-07-01T16:12:58Z

...y/com/netflix/spinnaker/clouddriver/kubernetes/v1/provider/KubernetesV1ProviderConfig.groovy

-                                            AccountCredentialsRepository accountCredentialsRepository,
-                                            KubernetesV1CachingAgentDispatcher kubernetesV1CachingAgentDispatcher) {
-    this.kubernetesV1Provider = new KubernetesV1Provider(kubernetesCloudProvider, Collections.newSetFromMap(new ConcurrentHashMap<Agent, Boolean>()))
+class KubernetesV1ProviderConfig implements CredentialsInitializerSynchronizable {


A name like KubernetesV1ProviderSynchronizable would be better, since this is no longer an @Configuration class.

scottfrederick · 2019-07-01T16:23:06Z

...oovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/caching/KubernetesV2ProviderConfig.java

-  @DependsOn("kubernetesNamedAccountCredentials")
-  KubernetesV2Provider kubernetesV2Provider(
-      KubernetesCloudProvider kubernetesCloudProvider,
+public class KubernetesV2ProviderConfig implements CredentialsInitializerSynchronizable {


KubernetesV2ProviderSynchronizable would be better

scottfrederick · 2019-07-01T21:15:36Z

See #3833 to fix the issue of temp files on refresh.

ezimanyi

Overall this looks really good! I left a number of inline comments about specific things.

One more general thing is I'd like to see some more tests validating this; at a minimum the tests that were deleted should be recreated in the right place to ensure the logic they were testing is not broken.

.../main/groovy/com/netflix/spinnaker/clouddriver/kubernetes/security/KubeconfigFileHasher.java

...com/netflix/spinnaker/clouddriver/kubernetes/security/KubernetesNamedAccountCredentials.java

...tflix/spinnaker/clouddriver/kubernetes/v1/provider/KubernetesV1ProviderSynchronizable.groovy

ezimanyi · 2019-07-02T21:44:24Z

...tflix/spinnaker/clouddriver/kubernetes/v1/provider/KubernetesV1ProviderSynchronizable.groovy

+    this.kubernetesSpinnakerKindMap = kubernetesSpinnakerKindMap
+    this.catsModule = catsModule
+
+    ScheduledExecutorService poller = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory(KubernetesV1ProviderSynchronizable.class.getSimpleName()))


Is this being used at all? It looks like we just create it and let it go out of scope.

That's something I was confused about. We were doing this in the previous version of this file but I could not figure out what it was actually used for.

...groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java

ethanfrogers · 2019-07-03T14:26:20Z

@ezimanyi I've addressed most of the feedback so far. I only need to address the bit about the metrics flag and add tests. Thanks for the thorough review and the perf recommendations!

ezimanyi

Changes look good! Three tiny comments inline.

The only remaining question from the first review is if we can remove the poller objects...I'm pretty sure they're not needed (unless they are doing something crazy that I don't understand) but also am fine leaving them if they were there before and we don't want to add to this change.

ezimanyi · 2019-07-03T22:55:17Z

...com/netflix/spinnaker/clouddriver/kubernetes/config/KubernetesConfigurationProperties.groovy

@@ -66,12 +67,17 @@ class KubernetesConfigurationProperties {
 }

 @ToString(includeNames = true)
+@EqualsAndHashCode(onlyExplicitlyIncluded = true)


For something like this where you're going to include everything, might be easier to leave out the onlyExplicitlyIncluded (though this may just be leftover from when you were testing this as I think you said this class was behaving strangely).

ezimanyi · 2019-07-03T23:03:27Z

...groovy/com/netflix/spinnaker/clouddriver/kubernetes/v2/security/KubernetesV2Credentials.java


-  @Getter private boolean metrics;
-  @Getter private final boolean debug;
+  @Include @Getter private boolean metrics;


I think we could remove the @Getter on the metrics now to make it more clear that consumers shouldn't care. (I think it's used in one test, but that test should probably just check the computed one.)

ezimanyi · 2019-07-03T23:06:14Z

...x/spinnaker/clouddriver/kubernetes/v1/provider/KubernetesV1ProviderSynchronizableSpec.groovy

@@ -0,0 +1,126 @@
+/*
+ * Copyright 2019 Netflix, Inc.


s/Netflix/Armory/

implement credential synchronization for kubernetes accounts. defines `CredentialsInitializerSynchronizable` on the `KubernetesV*ProviderConfig` classes. Each of these classes is responsible for handling accounts for each provider version.

renames provider config classes to synchronizable since there is no longer an `@Configuration` annotation.

adds a `kubeconfigFileHash` property which enables us to determine if the file contents of the kubeconfig file changed even if the path hasn't changed. this kind of change warrants an account refresh becuase the credentials used by the account may have possibly changed.

previously, we were modifying omitKinds when an account was initialized. this caused calls to `equals` to fail because they didn't match configured omitKinds. this change defers to `omitKindsComputed` so that checking new and existing credentials is accurate according to how the account it configured. this allows us to only initialize modified or new accounts rather than every credential.

addressing PR feedback

ethanfrogers changed the title ~~feat(k8s): dynamic account config~~ WIP feat(k8s): dynamic account config Jun 27, 2019

ethanfrogers added the do not merge label Jun 27, 2019

ethanfrogers requested a review from ezimanyi June 27, 2019 17:33

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch 2 times, most recently from 8256d87 to e639263 Compare June 28, 2019 19:00

ethanfrogers removed the do not merge label Jun 28, 2019

ethanfrogers changed the title ~~WIP feat(k8s): dynamic account config~~ feat(k8s): dynamic account config Jun 28, 2019

scottfrederick reviewed Jul 1, 2019

View reviewed changes

scottfrederick mentioned this pull request Jul 1, 2019

fix(kubernetes): Use deterministic temp file names #3833

Closed

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch from e639263 to 567c91e Compare July 2, 2019 12:27

ezimanyi reviewed Jul 2, 2019

View reviewed changes

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch 3 times, most recently from 60c999f to 6a2be6d Compare July 3, 2019 14:24

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch 4 times, most recently from ab26920 to 68bdb72 Compare July 3, 2019 20:02

ezimanyi approved these changes Jul 3, 2019

View reviewed changes

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch 2 times, most recently from 7125fa1 to 89e12e1 Compare July 8, 2019 15:27

ethanfrogers added 10 commits July 8, 2019 12:09

feat(k8s): dynamic account config

88c50db

implement credential synchronization for kubernetes accounts. defines `CredentialsInitializerSynchronizable` on the `KubernetesV*ProviderConfig` classes. Each of these classes is responsible for handling accounts for each provider version.

feat(k8s): detemine if account has changed

35208fb

refactor(k8s): rename provider config classes

0bc1c07

renames provider config classes to synchronizable since there is no longer an `@Configuration` annotation.

refactor(k8s): address feedback

30d3708

addressing PR feedback

refactor(k8s): add tests for synchronizable class

935301b

fix(k8s): fix incorrect rebase

76b9422

refactor(k8s): address feedback

9a20dbe

refactor(k8s): add getter back

00c7d65

ethanfrogers force-pushed the kubernetes-cloud-config-2 branch from 89e12e1 to 00c7d65 Compare July 8, 2019 16:10

ethanfrogers merged commit 8b941c7 into spinnaker:master Jul 8, 2019

ethanfrogers deleted the kubernetes-cloud-config-2 branch July 8, 2019 17:10

spinnakerbot added the target-release/1.15 label Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(k8s): dynamic account config #3824

feat(k8s): dynamic account config #3824

ethanfrogers commented Jun 27, 2019 •

edited

Loading

ethanfrogers commented Jun 27, 2019 •

edited

Loading

ethanfrogers commented Jun 27, 2019

scottfrederick commented Jun 27, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

scottfrederick left a comment

scottfrederick Jul 1, 2019

scottfrederick Jul 1, 2019

scottfrederick commented Jul 1, 2019

ezimanyi left a comment

ezimanyi Jul 2, 2019

ethanfrogers Jul 3, 2019

ethanfrogers commented Jul 3, 2019

ezimanyi left a comment •

edited

Loading

ezimanyi Jul 3, 2019

ezimanyi Jul 3, 2019

ezimanyi Jul 3, 2019

feat(k8s): dynamic account config #3824

feat(k8s): dynamic account config #3824

Conversation

ethanfrogers commented Jun 27, 2019 • edited Loading

ethanfrogers commented Jun 27, 2019 • edited Loading

ethanfrogers commented Jun 27, 2019

scottfrederick commented Jun 27, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

ethanfrogers commented Jun 28, 2019

scottfrederick left a comment

Choose a reason for hiding this comment

scottfrederick Jul 1, 2019

Choose a reason for hiding this comment

scottfrederick Jul 1, 2019

Choose a reason for hiding this comment

scottfrederick commented Jul 1, 2019

ezimanyi left a comment

Choose a reason for hiding this comment

ezimanyi Jul 2, 2019

Choose a reason for hiding this comment

ethanfrogers Jul 3, 2019

Choose a reason for hiding this comment

ethanfrogers commented Jul 3, 2019

ezimanyi left a comment • edited Loading

Choose a reason for hiding this comment

ezimanyi Jul 3, 2019

Choose a reason for hiding this comment

ezimanyi Jul 3, 2019

Choose a reason for hiding this comment

ezimanyi Jul 3, 2019

Choose a reason for hiding this comment

ethanfrogers commented Jun 27, 2019 •

edited

Loading

ethanfrogers commented Jun 27, 2019 •

edited

Loading

ezimanyi left a comment •

edited

Loading