t6pc-bot
commented
5 years ago Can one of the admins verify this patch?
Closed mperniola closed 5 years ago
t6pc-bot
commented
5 years ago Can one of the admins verify this patch?
alberto-brigandi
commented
5 years ago Also please fix the header in these files as reported here https://travis-ci.org/indigo-dc/orchestrator/builds/566831699#L761
mperniola
commented
5 years ago Il 06.08.2019 01:43 Alberto Brigandì ha scritto:
@ALBERTO-BRIGANDI requested changes on this pull request.
In pom.xml [1]:
@@ -1,6 +1,6 @@
Is it needed? Why you didn't use the native Spring RestTemplate? I know how to use that, I don't know Spring and Spring RestTemplate
In src/main/java/it/reply/orchestrator/service/ToscaService.java [4]:
@@ -1,5 +1,5 @@ /*
- Copyright © 2015-2019 Santer Reply S.p.A.
- Copyright © 2015-2018 Santer Reply S.p.A.
Shouldn't it be 2019? if you say so... done
In src/main/java/it/reply/orchestrator/service/ToscaService.java [5]:
@@ -177,6 +178,9 @@ public void validateUserInputs(Map<String, PropertyDefinition> templateInputs,
public Optional
getNodeCapabilityByName(NodeTemplate node, String propertyName);
- public Optional
getNodePropertyByName(NodeTemplate node, This method have been removed My code was implemented when those methods were available and therefore I expected to restore them. If it is necessary to remove them, please provide me with an alternative.
In src/main/java/it/reply/orchestrator/service/ToscaService.java [6]:
@@ -244,6 +248,9 @@ public void validateUserInputs(Map<String, PropertyDefinition> templateInputs, public DirectedMultigraph<NodeTemplate, RelationshipTemplate> buildNodeGraph( Map<String, NodeTemplate> nodes, boolean checkForCycles);
- public
Optional getTypedNodePropertyByName(NodeTemplate node, This method have been removed My code was implemented when those methods were available and therefore I expected to restore them. If it is necessary to remove them, please provide me with an alternative.
In src/main/java/it/reply/orchestrator/service/ToscaServiceImpl.java [7]:
@@ -780,6 +780,19 @@ public void removeRemovalList(NodeTemplate node) { return CommonUtils.getFromOptionalMap(node.getArtifacts(), artifactName); }
- @Override
This method have been removed My code was implemented when those methods were available and therefore I expected to restore them. If it is necessary to remove them, please provide me with an alternative.
In src/main/java/it/reply/orchestrator/service/ToscaServiceImpl.java [8]:
@@ -780,6 +780,19 @@ public void removeRemovalList(NodeTemplate node) { return CommonUtils.getFromOptionalMap(node.getArtifacts(), artifactName); }
- @Override
- public Optional
getNodePropertyByName(NodeTemplate node, - String propertyName) {
- return CommonUtils.getFromOptionalMap(node.getProperties(), propertyName);
- }
- @Override
This method have been removed My code was implemented when those methods were available and therefore I expected to restore them. If it is necessary to remove them, please provide me with an alternative.
In src/main/java/it/reply/orchestrator/service/ToscaServiceImpl.java [9]:
@@ -1,5 +1,5 @@ /*
- Copyright © 2015-2019 Santer Reply S.p.A.
- Copyright © 2015-2018 Santer Reply S.p.A.
Shouldn't it be 2019? if you say so... done
In src/main/java/it/reply/orchestrator/service/VaultServiceImpl.java [10]:
+import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.stereotype.Service; +import org.springframework.vault.VaultException; +import org.springframework.vault.authentication.TokenAuthentication; +import org.springframework.vault.client.VaultEndpoint; +import org.springframework.vault.core.VaultTemplate; +import org.springframework.vault.support.VaultResponse; + +@Service +@EnableConfigurationProperties(VaultProperties.class) +public class VaultServiceImpl implements VaultService { +
- @Autowired
- private VaultProperties vaultProperties;
- private String token;
Token should not be a field. VaultServiceImpl is a Spring bean with default scope; therefore it is a singleton and it's shared by all request coming from different users Finally I come to know what a Spring Bean is :) (really :)). Indeed being declared as "Service" I should have imagined that it was a Singleton ... Done
In src/main/java/it/reply/orchestrator/service/VaultServiceImpl.java [11]:
- httpPost.setEntity(stringEntity);
- CloseableHttpResponse response = httpclient.execute(httpPost);
+
- StringBuilder responseStrBuilder = new StringBuilder();
- BufferedReader reader = new BufferedReader(
- new InputStreamReader((response.getEntity().getContent())));
- String inputStr;
- while ((inputStr = reader.readLine()) != null) {
- responseStrBuilder.append(inputStr);
- }
- final int status = response.getStatusLine().getStatusCode();
- reader.close();
Resources should be closed in finally blocks The "close" method can throw an IoException exception. I have inserted empty catches to ensure that all resources are closed. However in that case it returns an empty token that generates a subsequent VaultException on a higher level
In src/main/java/it/reply/orchestrator/service/VaultServiceImpl.java [12]:
+
- token = (String) auth.get("client_token");
- } else {
- String message = responseStrBuilder.toString();
- if (status == 400) {
- if (message.toLowerCase().contains("token is expired")) {
- throw new VaultTokenExpiredException("Unable to retrieve token for Vault:"
- " accessToken is expired");
- }
- }
- throw new VaultException(String.format("Unable to retrieve token for Vault:"
- " %d (%s).", status, message));
- }
- } catch (VaultException ve) {
- throw ve;
- } catch (Exception e) {
Please do not catch generic Exceptions. It with catch also eventual InterruptedExceptions without proper handling done
In src/main/java/it/reply/orchestrator/service/VaultServiceImpl.java [13]:
- return getTemplate().read(path).getData();
- }
- public void deleteSecret(String path) {
- getTemplate().delete(path);
- }
- public List
listSecrets(String path) { - return getTemplate().list(path);
- }
- /**
- Retrieve Vault token starting from access token.
- */
- @SuppressWarnings("unchecked")
- public VaultService retrieveToken(String accessToken) {
Why did you not use a RestTemplate? Or implemented the a ClientAuthentication like here https://github.com/spring-projects/spring-vault/blob/1.1.3.RELEASE/spring-vault-core/src/main/java/org/springframework/vault/authentication/AwsIamAuthentication.java There are utilities to simply extract the token from the response, like LoginTokenUtil.from(response.getAuth()) I don't understand what you mean; the token is retrieved from the Iam access token
In src/main/java/it/reply/orchestrator/service/commands/PrefilterCloudProviders.java [14]:
@@ -1,5 +1,5 @@ /*
- Copyright © 2015-2019 Santer Reply S.p.A.
- Copyright © 2015-2018 Santer Reply S.p.A.
Shouldn't it be 2019? if you say so... done
In src/main/java/it/reply/orchestrator/service/commands/PrefilterCloudProviders.java [15]:
@@ -100,13 +111,46 @@ public void execute(DelegateExecution execution, ArchiveRoot ar = toscaService .prepareTemplate(deployment.getTemplate(), deployment.getParameters());
- DeploymentType type = rankCloudProvidersMessage.getDeploymentType();
- //secrets handling
- boolean hasSecrets = false;
- if (type != null && type.equals(DeploymentType.MARATHON)) {
- Map<String, NodeTemplate> nodes = Optional
- .ofNullable(ar.getTopology())
- .map(Topology::getNodeTemplates)
- .orElseGet(HashMap::new);
- DirectedMultigraph<NodeTemplate, RelationshipTemplate> graph =
Why is it needed to build an ordered graph? I don't know, the code block was copied from another location. what I really need is to understand if the "secrets" node exists in the template
In src/main/java/it/reply/orchestrator/service/deployment/providers/AbstractMesosDeploymentService.java [16]:
@@ -1,5 +1,5 @@ /*
- Copyright © 2015-2019 Santer Reply S.p.A.
- Copyright © 2015-2018 Santer Reply S.p.A.
Shouldn't it be 2019? if you say so... done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [17]:
@@ -1,5 +1,5 @@ /*
- Copyright © 2015-2019 Santer Reply S.p.A.
- Copyright © 2015-2018 Santer Reply S.p.A.
Shouldn't it be 2019?
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [18]:
- vaultService.retrieveToken(vtoken);
- } catch (VaultTokenExpiredException e) {
- vtoken = oauth2TokenService.getRefreshedAccessToken(requestedWithToken);
- vaultService.retrieveToken(vtoken);
- }
- if (StringUtils.isEmpty(vaultService.getVaultToken())) {
- throw new VaultException("Vault token not defined.");
- }
- for (String depentry:depentries) {
- List
entries = vaultService.listSecrets(spath + "/" + depentry); - for (String entry:entries) {
- try {
- vaultService.deleteSecret(spath + "/" + depentry + "/" + entry);
- } catch (Exception ex) {
Please do not catch Exception done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [19]:
- } catch (VaultTokenExpiredException e) {
- vtoken = oauth2TokenService.getRefreshedAccessToken(requestedWithToken);
- vaultService.retrieveToken(vtoken);
- }
- if (StringUtils.isEmpty(vaultService.getVaultToken())) {
- throw new VaultException("Vault token not defined.");
- }
- for (String depentry:depentries) {
- List
entries = vaultService.listSecrets(spath + "/" + depentry); - for (String entry:entries) {
- try {
- vaultService.deleteSecret(spath + "/" + depentry + "/" + entry);
- } catch (Exception ex) {
- System.out.println(ex.getMessage());
Please never use System.out; use the logging utilities instead was for debugging, removed, done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [20]:
@Override
- @Deprecated
Please, do not leave this method now working silently; either:
- throw an UnsupportedOperationException
- or keep the logic functioning without these new parents
- or update the inherited interface and just remove this newly deprecated method
done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [21]:
protected App generateExternalTaskRepresentation(MarathonApp marathonTask) { App app = new App();
- return app;
- }
- public static class VaultSecret {
Please move this class in a standalone class inside the dto package done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [22]:
- enventry.put("secret", entry.getKey());
- marathonEnv.put(entry.getKey(), enventry);
- SecretSource source = new SecretSource();
- source.setSource(entry.getKey() + "@value");
- secrets.put(entry.getKey(), source);
- //write secret on service
- String spath = "secret/private/" + deploymentId + "/" + marathonTask.getId() + "/"
- entry.getKey();
- try {
- VaultResponse response = vaultService.writeSecret(spath,
- (new VaultSecret()).setValue(entry.getValue()));
- if (response != null) {
- System.out.println(response.toString());
- }
- } catch (Exception ex) {
Please do not catch Exception done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [23]:
- marathonEnv.put(entry.getKey(), enventry);
- SecretSource source = new SecretSource();
- source.setSource(entry.getKey() + "@value");
- secrets.put(entry.getKey(), source);
- //write secret on service
- String spath = "secret/private/" + deploymentId + "/" + marathonTask.getId() + "/"
- entry.getKey();
- try {
- VaultResponse response = vaultService.writeSecret(spath,
- (new VaultSecret()).setValue(entry.getValue()));
- if (response != null) {
- System.out.println(response.toString());
- }
- } catch (Exception ex) {
- System.out.println(ex.getMessage());
Please use the logging utility done
In src/main/java/it/reply/orchestrator/service/deployment/providers/MarathonServiceImpl.java [24]:
- for (Map.Entry<String, String> entry : marathonTask.getSecrets().entrySet()) {
- Map<String,String> enventry = new HashMap<String,String>();
- enventry.put("secret", entry.getKey());
- marathonEnv.put(entry.getKey(), enventry);
- SecretSource source = new SecretSource();
- source.setSource(entry.getKey() + "@value");
- secrets.put(entry.getKey(), source);
- //write secret on service
- String spath = "secret/private/" + deploymentId + "/" + marathonTask.getId() + "/"
- entry.getKey();
- try {
- VaultResponse response = vaultService.writeSecret(spath,
- (new VaultSecret()).setValue(entry.getValue()));
- if (response != null) {
- System.out.println(response.toString());
Please use the logging utility done
In src/main/java/it/reply/orchestrator/service/security/CustomOAuth2Template.java [25]:
@@ -82,6 +82,7 @@ public AccessGrant exchangeToken(String accessToken, Set
scopes) { generateParams(clientConfiguration.getTokenEndpointAuthMethod()); params.set("subject_token", accessToken); params.set("grant_type", "urn:ietf:params:oauth:grant-type:token-exchange");
- params.set("audience", clientConfiguration.getClientId());
What is this needed for? to retrieve vault token (see note up)
In src/main/java/it/reply/orchestrator/service/security/CustomOAuth2Template.java [26]:
@@ -99,6 +100,7 @@ public AccessGrant refreshToken(String refreshToken, Set
scopes) { generateParams(clientConfiguration.getTokenEndpointAuthMethod()); params.set("refresh_token", refreshToken); params.set("grant_type", "refresh_token");
- params.set("audience", clientConfiguration.getClientId());
What is this needed for? to retrieve vault token (see note up)
In src/test/java/it/reply/orchestrator/service/VaultServiceTest.java [27]:
+ +/**
- This integration test makes real request to the Vault APIs.
- *
- @author Michele Perniola
- *
- */ +@RunWith(SpringJUnit4ClassRunner.class) +@SpringBootTest(classes = Application.class) +public class VaultServiceTest {
- @Autowired
- private VaultService vaultService;
- @Test
- @Ignore
Please add some functioning unit test in a future update, at this time the testbed is not available
You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [28], or mute the thread [29].
Links:
[1] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310828242 [2] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310828330 [3] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310828571 [4] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310828713 [5] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829116 [6] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829137 [7] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829163 [8] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829174 [9] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829242 [10] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829624 [11] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310829764 [12] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310830076 [13] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310830317 [14] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310830873 [15] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310831015 [16] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310831237 [17] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310831266 [18] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310831364 [19] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310831515 [20] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832231 [21] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832381 [22] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832516 [23] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832591 [24] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832621 [25] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832692 [26] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832703 [27] https://github.com/indigo-dc/orchestrator/pull/334#discussion_r310832771 [28] https://github.com/indigo-dc/orchestrator/pull/334?email_source=notifications&email_token=AEYH5QLZERY6I4S2YHIHNYTQDC3KTA5CNFSM4II3IK22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCAT6INY#pullrequestreview-271049783 [29] https://github.com/notifications/unsubscribe-auth/AEYH5QPQ6XSILVHRINCA7TTQDC3KTANCNFSM4II3IK2Q
-- Michele Perniola, M.S.C.S. INFN (National Institute for Nuclear Physics) - Bari Via Orabona, 4 70125 – Bari email: michele.perniola@ba.infn.it skype: michele.perniola
codecov-io
commented
5 years ago Merging #334 into master will decrease coverage by
0.6%. The diff coverage is31.68%.
@@ Coverage Diff @@
## master #334 +/- ##
============================================
- Coverage 62.11% 61.51% -0.61%
- Complexity 830 836 +6
============================================
Files 169 171 +2
Lines 5050 5145 +95
Branches 304 312 +8
============================================
+ Hits 3137 3165 +28
- Misses 1807 1871 +64
- Partials 106 109 +3| Flag | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| #integration | 8.99% <4.95%> (-0.08%) |
161 <1> (+1) |
|
| #unittests | 57.35% <31.68%> (-0.53%) |
763 <6> (+6) |
| Impacted Files | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| ...t/reply/orchestrator/dto/cmdb/MarathonService.java | 70% <ø> (ø) |
1 <0> (ø) |
:arrow_down: |
| ...y/orchestrator/dto/mesos/marathon/MarathonApp.java | 0% <ø> (ø) |
0 <0> (ø) |
:arrow_down: |
| .../service/security/CustomOAuth2TemplateFactory.java | 12.5% <0%> (-1.79%) |
1 <0> (ø) |
|
| ...strator/service/security/CustomOAuth2Template.java | 0% <0%> (ø) |
0 <0> (ø) |
:arrow_down: |
| ...orchestrator/config/properties/OidcProperties.java | 29.41% <0%> (-1.84%) |
6 <0> (ø) |
|
| ...ator/service/commands/PrefilterCloudProviders.java | 46.75% <0%> (-4%) |
25 <0> (ø) |
|
| .../service/security/OAuth2ConfigurationsService.java | 13.63% <0%> (-4.02%) |
1 <0> (ø) |
|
| ...vice/deployment/providers/MarathonServiceImpl.java | 51.69% <27.77%> (-3.6%) |
33 <1> (+1) |
|
| ...rator/exception/VaultJwtTokenExpiredException.java | 50% <50%> (ø) |
1 <1> (?) |
|
| ...t/reply/orchestrator/service/VaultServiceImpl.java | 64.51% <64.51%> (ø) |
4 <4> (?) |
|
| ... and 2 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update bbc39c1...f8b9d34. Read the comment docs.
alberto-brigandi
commented
5 years ago Hi @mperniola
I know how to use that, I don't know Spring and Spring RestTemplate
it is unfortunate that you don't know Spring, since this project is completely based on it. Anyway, this is a nice guide on how to use the RestTemplate https://www.baeldung.com/rest-template
My code was implemented when those methods were available and therefore I expected to restore them. If it is necessary to remove them, please provide me with an alternative.
You can use the ToscaUtils.extractMap() method that you already used in another part of your PR
I don't understand what you mean; the token is retrieved from the Iam access token
I mean that instead of mapping the Vault response to a generic map. you can map it to a VaultResponse object (class provided by the library that you imported), like is done here https://github.com/spring-projects/spring-vault/blob/df0ea9aac78547d43099fa2aafe3b40930cf20b5/spring-vault-core/src/main/java/org/springframework/vault/authentication/AwsIamAuthentication.java#L128
I don't know, the code block was copied from another location. what I really need is to understand f the "secrets" node exists in the template
I think that you should understand what the code that you're coping is doing before pasting it somewhere else; In any case I think that you only need to iterate on the map of the node templates without generating an ordered iterator
in a future update, at this time the testbed is not available
You don't need a testbed to implement unit tests; you can use Mockito to mock the responses
alberto-brigandi
commented
5 years ago t6pc-bot test this
alberto-brigandi
commented
5 years ago t6pc-bot test this
These compilation errors remain but they are not my problem
Description Resource Path Location Type Unhandled exception type Exception ChronosServiceTest.java /orchestrator/src/test/java/it/reply/orchestrator/service/deployment/providers line 116 Java Problem Unhandled exception type Exception DynafedServiceTest.java /orchestrator/src/test/java/it/reply/orchestrator/service line 100 Java Problem Unhandled exception type Exception GetSlamTest.java /orchestrator/src/test/java/it/reply/orchestrator/service/commands line 63 Java Problem Unhandled exception type Exception ImServiceTest.java /orchestrator/src/test/java/it/reply/orchestrator/service/deployment/providers line 125 Java Problem Unhandled exception type Exception MarathonServiceTest.java /orchestrator/src/test/java/it/reply/orchestrator/service/deployment/providers line 99 Java Problem Unhandled exception type Exception OneDataServiceTest.java /orchestrator/src/test/java/it/reply/orchestrator/service line 110 Java Problem