Initial merge of Development to Master #1

nabuskey · 2020-08-10T22:11:59Z

Initial merge of Development to Master

Fix typos

reformat code

Fix typos

nabuskey · 2020-08-12T00:14:59Z

...on/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AmazonPollingSynchronizer.java

+        try {
+            // Cannot autowire EcsAccountMapper for some reason. It returns every field wth null values.
+            // Doing it this way is a problem.
+            if (this.ecsAccountMapper == null) {
+                this.ecsAccountMapper = (EcsAccountMapper) applicationContext.getBean("ecsAccountMapper");
+            }
+            EcsProviderUtils.synchronizeEcsCredentialsMapper(ecsAccountMapper, lazyLoadCredentialsRepository);
+        } catch (IllegalAccessException | NoSuchFieldException e) {
+            log.error("Error encountered while updating ECS credentials mapper: {}", e.getMessage());
+            e.printStackTrace();
+            return;
+        } catch (BeansException e) {
+            log.error("Error obtaining EcsAccountMapper bean from Spring context.");
+            return;
+        }


I really do not like this. This is guaranteed to fail on @PostConstruct . But I am not sure how to go about resolving this. @nimakaviani

hmmmm I wonder if the problem you are seeing is somewhat similar to the issue discussed here: https://spinnakerteam.slack.com/archives/CK9FK4XDF/p1597093660354200

Have a look and let me know what you think.

Unfortunately that doesn't work. The issue is that EcsAccountMapper depends on AccountCredentialsProvider which depends on CredentialsRepository. This plugin replaces CredentialsRepository and it needs to be able to update EcsAccountMapper when updates to accounts are made. So circular dependency. Note that this (BeansException) happens only at startup. It works fine once started.

We can take it out of this class and do it in another class but it may cause ECS account state inconsistency.
Is there anything similar to Go's channels in Java? Something light weight to communicate between objects.

Kotlin has the concept of coroutines and channels similar to golang (here) but nothing in Java that I know of. Let me check out the plugin code later today and play with it a bit to see if I can find any alternatives.

nimakaviani

some minor suggestions as I was looking at the code.

nimakaviani · 2020-08-12T06:31:21Z

account-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/Response.java

+        CredentialsConfig.Account ec2Account = new CredentialsConfig.Account() {{
+            setName(account.getName());
+            setAccountId(account.getAccountId());
+            setAssumeRole(account.getAssumeRole());
+            setRegions(regions);
+            setPermissions(account.getPermissions());
+            setEnvironment(account.getEnvironment());
+        }};
+        return ec2Account;


Suggested change

CredentialsConfig.Account ec2Account = new CredentialsConfig.Account() {{

setName(account.getName());

setAccountId(account.getAccountId());

setAssumeRole(account.getAssumeRole());

setRegions(regions);

setPermissions(account.getPermissions());

setEnvironment(account.getEnvironment());

}};

return ec2Account;

return new CredentialsConfig.Account() {{

setName(account.getName());

setAccountId(account.getAccountId());

setAssumeRole(account.getAssumeRole());

setRegions(regions);

setPermissions(account.getPermissions());

setEnvironment(account.getEnvironment());

}};```

account-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/Response.java

nimakaviani · 2020-08-12T06:35:06Z

.../main/java/com/amazon/aws/spinnaker/plugin/registration/AmazonAwsProviderSynchronizer.groovy

+                def accountToDelete = reservationReportAccounts.find { it.name == accountNameToDelete }
+
+                if (accountToDelete) {
+                    reservationReportAccounts.remove(accountToDelete)
+                }


Suggested change

def accountToDelete = reservationReportAccounts.find { it.name == accountNameToDelete }

if (accountToDelete) {

reservationReportAccounts.remove(accountToDelete)

}

reservationReportAccounts.removeIf { it.name == accountNameToDelete }

Co-authored-by: Nima Kaviani <17132353+nimakaviani@users.noreply.github.com>

ncknt · 2020-08-13T17:22:35Z

...rc/main/java/com/amazon/aws/spinnaker/plugin/registration/LazyLoadCredentialsRepository.java

+            synchronizer.sync();
+            cred = super.getOne(key);
+            if (cred != null) {
+                save(key, cred);


That doesn't seem necessary.

dkirillov · 2020-08-13T20:50:32Z

...on/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AccountRegistrationPlugin.java

+        List<Class> classes = new ArrayList<>(Arrays.asList(AmazonPollingSynchronizer.class,
+                AmazonEC2InfraCachingAgentScheduler.class,
+                AmazonAWSCachingAgentScheduler.class, AmazonECSCachingAgentScheduler.class));
+        for (Class calssToAdd : classes) {


calssToAdd typo?

Yes it is. It's fixed in feature/refactor-account-response. Thanks!

@PostConstruct

* Remove Account constructor and add AccountsStatus unit test * Remove unused code and imports * Fix formatting * remove @PostConstruct since it's guaranteed to fail

ncknt · 2020-08-17T17:57:20Z

...on/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AmazonPollingSynchronizer.java

+    // agent removal
+    private final CredentialsLoader<? extends NetflixAmazonCredentials> credentialsLoader;
+    private final CredentialsConfig credentialsConfig;
+    private final LazyLoadCredentialsRepository lazyLoadCredentialsRepository;


LazyLoadCredentialsRepository is not required here. I'd just use AccountCredentialsRepository.

ncknt · 2020-08-17T17:58:59Z

...on/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AmazonPollingSynchronizer.java

+            }
+            EcsProviderUtils.synchronizeEcsCredentialsMapper(ecsAccountMapper, lazyLoadCredentialsRepository);
+        } catch (IllegalAccessException | NoSuchFieldException e) {
+            log.error("Error encountered while updating ECS credentials mapper: {}", e.getMessage());


log.error(..., e), don't print stack trace.

ncknt · 2020-08-17T18:57:44Z

...-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AccountsStatus.java

+        return new ArrayList<>(ecsAccounts.values());
+    }
+
+    public Boolean getDesiredAccounts() {


Can it return a boolean instead of Boolean? Otherwise we have to deal with value == null in the caller.

changed to boolean. Thanks!

kevinawoo

I'm cool with it as long as everyone else is

* Implement IAM auth for API gateway

coleduclos · 2020-08-18T23:51:56Z

...istration/src/test/java/com/amazon/aws/spinnaker/plugin/registration/AccountsStatusTest.java

+        CredentialsConfig.Account ec2Account = new CredentialsConfig.Account() {{
+            setName("test1");
+            setAccountId("1");
+            setAssumeRole("role/role1");


Are we expecting the account registry to return SpinnakerAssumeRole with role/ prefixed to the role name? Seems like from the Response class in the code above, this is not a hard requirement.

It handles both cases. Spinnaker does require role name to start with role/
https://github.com/awslabs/aws-account-registration-plugin-spinnaker/blob/78b5c5d5a97543c6d6a5a231d7b80690d8f07f14/account-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/Response.java#L75

coleduclos · 2020-08-19T00:17:42Z

account-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/Response.java

+            if (exists != null) {
+                continue;
+            }
+            if ("SUSPENDED".equals(account.getStatus()) || account.getProviders() == null || account.getProviders().isEmpty()) {


We may want to make the status check case insensitive... If for whatever reason AWS changes the casing returned from the Orgs API.

Good idea. I will do that.

dkirillov · 2020-08-19T20:04:31Z

...-registration/src/main/java/com/amazon/aws/spinnaker/plugin/registration/AccountsStatus.java

+
+    private Response getResources(String url) {
+        if (lastSyncTime != null) {
+            url = String.format("%s?after=%s", url, lastSyncTime.toString());


If the base URL contains parameters, for instance https://somewhere[.]com/api?param=1234, will this work? Is it supported?

* implement nextUrl support * Add debug logs * Add support for query strings in configuration * Finalize JSON schema

nabuskey · 2020-08-24T18:23:11Z

Coverage for non-utility classes.

Package	Class, %	Method, %	Line, %
com.amazon.aws.spinnaker.plugin.registration	100% (8/ 8)	78.8% (52/ 66)	74.9% (239/ 319)
com.amazon.aws.spinnaker.plugin.registration.auth.iam	100% (3/ 3)	100% (6/ 6)	100% (28/ 28)

* Add validity check for remote host response * Add support for whitespaces in regions * Handle cases where only deleted accounts are returned by remote host.

Initial merge of development branch to master.

mccloman added 30 commits August 5, 2020 16:43

Rename project

c8964c8

adding modified docker files for testing

5d1fde2

clean up

4b4bed4

Clean up imports

4cbb54e

Clean up gralde files

80157d9

Remove unused class

64a2fdb

clean up gradle file

bd87af0

Add null checking

b5fc21c

Fix typos

Make url and sync frequency configurable

4a9aa90

Separate agent schedulers

383f34e

Remove unnecessary imports

ff1d909

reformat code

fix default url

4ce1301

Clean up sync()

751208c

Update comment

591e398

change switch statement to ifs

cc27093

Position literals first in String comparisons

639d764

reformat code

9bf3e9a

update copyright

045dd3a

remove unnecessary files

17a5d72

Add README

2a57ea9

Rename project

a7b526c

adding modified docker files for testing

25528d3

clean up

18956a4

Clean up imports

7626367

Clean up gralde files

f2d6473

Remove unused class

9ee096e

clean up gradle file

814fe13

Add null checking

eb4e479

Fix typos

Make url and sync frequency configurable

e3d2bee

Separate agent schedulers

bcf1fe9

mccloman added 4 commits August 11, 2020 17:45

Update gradle to enable tests

9dc5fb9

Correct property name

1745688

Remove AccountRegistrationProperties to remove hardcoded default values

158e0ea

Fix README

f637690

nabuskey commented Aug 12, 2020

View reviewed changes

nimakaviani reviewed Aug 12, 2020

View reviewed changes

nabuskey and others added 6 commits August 12, 2020 09:36

Cleanup Response.java

c742a8b

Co-authored-by: Nima Kaviani <17132353+nimakaviani@users.noreply.github.com>

Add pagination support in JSON deserialization

12811d5

Add support for suspended accounts and role names without role/ prefix

c510aac

Update provider list handling

263f149

Make agent schedulers private

bcb604b

Update README to include known issues

c117989

ncknt reviewed Aug 13, 2020

View reviewed changes

Add default constructor to Account

f35d964

dkirillov reviewed Aug 13, 2020

View reviewed changes

refactor account response

13c607b

* Remove Account constructor and add AccountsStatus unit test * Remove unused code and imports * Fix formatting * remove @PostConstruct since it's guaranteed to fail

ncknt reviewed Aug 17, 2020

View reviewed changes

kevinawoo approved these changes Aug 18, 2020

View reviewed changes

Feature/sig4 auth (#3)

78b5c5d

* Implement IAM auth for API gateway

coleduclos reviewed Aug 18, 2020

View reviewed changes

coleduclos reviewed Aug 19, 2020

View reviewed changes

dkirillov reviewed Aug 19, 2020

View reviewed changes

finalized account schema (#5)

541f27d

* implement nextUrl support * Add debug logs * Add support for query strings in configuration * Finalize JSON schema

Bugfix: response validity check (#7)

4ce653f

* Add validity check for remote host response * Add support for whitespaces in regions * Handle cases where only deleted accounts are returned by remote host.

nabuskey merged commit c0996b7 into master Aug 24, 2020

nabuskey mentioned this pull request Aug 24, 2020

Use AccountCredentialsRepository instead of LazyLoadCredentialsRepository #8

Closed

jasoncoffman pushed a commit that referenced this pull request Aug 24, 2020

Initial merge of Development to Master (#1)

68a8bfe

Initial merge of development branch to master.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial merge of Development to Master #1

Initial merge of Development to Master #1

nabuskey commented Aug 10, 2020

nabuskey Aug 12, 2020

nimakaviani Aug 12, 2020

nabuskey Aug 12, 2020

nimakaviani Aug 12, 2020

nimakaviani left a comment

nimakaviani Aug 12, 2020

nimakaviani Aug 12, 2020

ncknt Aug 13, 2020

dkirillov Aug 13, 2020

nabuskey Aug 14, 2020

ncknt Aug 17, 2020

ncknt Aug 17, 2020

ncknt Aug 17, 2020

nabuskey Aug 19, 2020

kevinawoo left a comment

coleduclos Aug 18, 2020

nabuskey Aug 19, 2020

coleduclos Aug 19, 2020

nabuskey Aug 19, 2020

dkirillov Aug 19, 2020

nabuskey commented Aug 24, 2020

Initial merge of Development to Master #1

Initial merge of Development to Master #1

Conversation

nabuskey commented Aug 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nimakaviani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinawoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabuskey commented Aug 24, 2020