Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1021
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158903497
Saw the changes, LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158901685
LGTM based on these tests (may not have tested the actual change):
```
nosetests --with-marvin --marvin-config=${marvinCfg} -s -a
tags=advanced,requ
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158617888
@bhaisaab @jlk want to review give a second lgtm?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as wel
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158617403
[1021.network.results.txt](https://github.com/apache/cloudstack/files/40781/1021.network.results.txt)
[1021.vpc.results.txt](https://github.com/apache/clou
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158401181
also running the regressions suites on it now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. I
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158374179
code reviewed. LGTM awaiting tests. it is running on one of the INFRA-10703
hosts but I know @abayer did some work on that issue...
---
If your project is set
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r45461347
--- Diff: server/src/com/cloud/api/response/ApiResponseSerializer.java ---
@@ -127,53 +151,74 @@ public static String
toJSONSerializedString(Respons
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158366425
@DaanHoogland I have force pushed again. Hopefully a good jenkins slave
will pick this up :)
---
If your project is set up for it, you can reply to this email
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158347545
@koushik-das sorry, no. I have not much test infra and run a lot of tests.
Could you force push your changes, please. I'll review your last changes.
---
If yo
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-158336847
@DaanHoogland did you get a chance to test it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. I
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154990742
great @koushik-das I'll rerun the tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If you
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154971789
@DaanHoogland Updated based on our discussion last week
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub a
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r44210903
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -71,4 +83,19 @@ public boolean shouldSkipField(FieldAttributes f) {
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r44210897
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -71,4 +83,19 @@ public boolean shouldSkipField(FieldAttributes f) {
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154062799
I am not fine with this change but must be honest and report regressions
tests from the bubble: they all passed.
```
Test router internal advanced
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154035894
@DaanHoogland If you look at ApiResponseGsonHelper.java in this PR there
are GSon builders
private static final GsonBuilder s_gBuilder;
private s
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154032129
@koushik-das you said 'LogLevel is used in the latter one only.'. That is
not true, I think. see com.cloud.agent.transport.RequestTest
---
If your project is
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154030678
@DaanHoogland That's right. I had already mentioned about the agent
commands in one of my previous comments
---
If your project is set up for it, you can reply
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154027199
@koushik-das I don't think you are right, command serialisation uses
LogLevel as well. Look at com.cloud.agent.transport.RequestTest and tell me if
you agree.
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-154025365
@DaanHoogland About the issue you pointed out in com.cloud.vm.VmWorkStart
serialization. Nice find but these are already existing issues. They surely
needs fixi
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43997043
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -71,4 +83,19 @@ public boolean shouldSkipField(FieldAttributes f) {
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43996820
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -27,30 +29,40 @@
import com.google.gson.GsonBuilder;
/**
- *
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43996309
--- Diff: api/src/org/apache/cloudstack/api/response/UserVmResponse.java ---
@@ -256,6 +259,7 @@
@SerializedName(ApiConstants.SSH_KEYPA
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43996196
--- Diff: api/src/org/apache/cloudstack/api/response/SslCertResponse.java
---
@@ -38,6 +40,7 @@
@SerializedName(ApiConstants.CERTIFICA
Github user koushik-das commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43996031
--- Diff:
api/src/org/apache/cloudstack/api/response/SSHKeyPairResponse.java ---
@@ -40,6 +42,7 @@
@SerializedName("fingerprint")
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153783852
I think we should add isSensitive() to @Parameter and use that.
Also we should base this on gson 2+ and not on the version we are presently
using.
--
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43898186
--- Diff: api/src/org/apache/cloudstack/api/response/UserVmResponse.java ---
@@ -256,6 +259,7 @@
@SerializedName(ApiConstants.SSH_KEYP
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43899212
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -71,4 +83,19 @@ public boolean shouldSkipField(FieldAttributes f) {
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43898594
--- Diff: server/src/com/cloud/api/ApiResponseGsonHelper.java ---
@@ -27,30 +29,40 @@
import com.google.gson.GsonBuilder;
/**
- *
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43897789
--- Diff: api/src/org/apache/cloudstack/api/response/SslCertResponse.java
---
@@ -62,10 +65,12 @@
@SerializedName(ApiConstants.CERTIF
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43897730
--- Diff: api/src/org/apache/cloudstack/api/response/SslCertResponse.java
---
@@ -62,10 +65,12 @@
@SerializedName(ApiConstants.CERTIF
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43892103
--- Diff: api/src/org/apache/cloudstack/api/response/SslCertResponse.java
---
@@ -38,6 +40,7 @@
@SerializedName(ApiConstants.CERTIFIC
Github user DaanHoogland commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1021#discussion_r43891649
--- Diff:
api/src/org/apache/cloudstack/api/response/SSHKeyPairResponse.java ---
@@ -40,6 +42,7 @@
@SerializedName("fingerprint")
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153755733
@koushik-das On second thought LogLevel can not be used I think: It is used
for json serialization and one might have to serialize a password to be send to
an
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153749695
```
2015-11-04 14:23:51,076 DEBUG [c.c.v.VmWorkJobHandlerProxy]
(Work-Job-Executor-34:ctx-0d2ee513 job-104/job-105 ctx-0d5c6252) Done executing
VM work job
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153704575
@koushik-das makes sense so we have a policy that for any sensitive field
LogLevel(LogLevel.OFF) be used. We should add that as a comment to the enum to
docume
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153695015
@bhaisaab Agree that the existing JSON and XML serialization can be
improved but it is better to do it as a separate PR. For JSON, the standard
Gson library is
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153690547
@DaanHoogland I think LogLevel is a better/generic name to suppress a field
from getting logged. Currently only sensitive field is getting annotated with
it. Go
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153690595
@DaanHoogland if you look at the changes or the code that takes in Object
(Response object) and serializes it; we create the json or xml response string
by manuall
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153686801
@koushik-das You don't agree that the name LogLevel obfuscates that it is
used to hide sensitive data?
---
If your project is set up for it, you can reply to
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153686114
@DaanHoogland @bhaisaab requestHasSensitiveInfo/responseHasSensitiveInfo
can be removed once this PR is accepted. I don't see any other use of these
parameters.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153682474
@bhaisaab no we shouldn't remove those both are used. I didn't understand
the {, <, >, } part.
The @LogLevel is used in the gson serialization and we
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153681687
@DaanHoogland we can simply use @LogLevel I simply shared that there is a
slight chance on confusion in future, and do we then remove the
requestHasSensitiveInfo/r
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153677208
@bhaisaab can you explain ? I like your comment about the annotation not
being descriptive on the sensitivity of the field. So for the best solution we
would h
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153668149
Also, it is possible to avoid serializing the objects by hand (like
adding/appending {, >, <, } etc by hand?). Probably use any available library,
and if that's no
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153667726
@koushik-das I think it's inappropriate as we're not marking those fields
to note that they have hold sensitive information; but anyway that works
---
If your pro
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153338874
The field level annotation was discussed as a prefered solution but in the
end a regexp on field names was used. I am fine with re-using the LogLevel
annotatio
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153337620
@DaanHoogland @bhaisaab I also couldn't find any annotation on API fields
for this. So ended up using the LogLevel which is also used in the agent
commands.
-
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153330895
@bhaisaab @koushik-das Isn't there an annotation on field level for APIs as
well? A change was accepted for that. I am looking for it and will update this
comm
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153327157
@bhaisaab What are your reasons for saying that @LogLevel(Log4jLevel.Off)
is inappropriate?
By sensitive param/annotation are you referring to the exist
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153309944
Usage of @LogLevel(Log4jLevel.Off) seems inappropriate to avoid logging
sensitive information, why not still check the sensitive param/annotation to
log or not log
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153003710
@koushik-das I've seen it before and the actual error message is
```
#
# A fatal error has been detected by the Java Runtime Environment:
#
S
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-153001614
Jenkins build failed with the following error. I saw some other PRs with
the same failure. So looks like some random issue.
[ERROR] SLF4J: Class path co
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-152996985
registerUserKeys log based on this change
2015-11-02 17:15:19,061 INFO [a.c.c.a.ApiServer]
(2001334745@qtp-678426242-0:ctx-608cad64 ctx-6e00b6fc) (logi
Github user koushik-das commented on the pull request:
https://github.com/apache/cloudstack/pull/1021#issuecomment-152971700
I have tried to look for the existing API response objects based on some
keyword search for sensitive fields. Please let me know if some response
objects are le
GitHub user koushik-das opened a pull request:
https://github.com/apache/cloudstack/pull/1021
CLOUDSTACK-8485: listAPIs are taking too long to return results
- Removed regex. based search/replace of sensitive data on API response
introduced as part of commit b0c6d4734724358df97b6fa4
57 matches
Mail list logo
