-
Notifications
You must be signed in to change notification settings - Fork 25
[VC-43753] CyberArk Discovery and Context: Upload data in the JSON format required by the API #684
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
…unmarshaled Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
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 deleted this file because it was causing import cycle.
$ go test -v ./pkg/datagatherer/...
# github.com/jetstack/preflight/pkg/datagatherer/k8s
package github.com/jetstack/preflight/pkg/datagatherer/k8s
imports github.com/jetstack/preflight/pkg/testutil from fieldfilter_test.go
imports github.com/jetstack/preflight/pkg/client from envtest.go
imports github.com/jetstack/preflight/pkg/internal/cyberark/dataupload from client_cyberark.go
imports github.com/jetstack/preflight/pkg/datagatherer/k8s from dataupload.go: import cycle not allowed in test
FAIL github.com/jetstack/preflight/pkg/datagatherer/k8s [setup failed]
? github.com/jetstack/preflight/pkg/datagatherer [no test files]
? github.com/jetstack/preflight/pkg/datagatherer/local [no test files]
FAIL
Resource interface{} | ||
DeletedAt Time | ||
Resource interface{} `json:"resource"` | ||
DeletedAt Time `json:"deleted_at,omitempty"` | ||
} |
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.
Added json annotations here so that I can unmarshal date readings from a file, for testing.
The agent already has an --input-file
option, but stops decoding the input at api.DataReading.Data
, leaving the actual data as interface{}
.
In the test in this PR I need to decode the Data, so that it has the same types as the DataGatherer.Fetch return values.
type DiscoveryData struct { | ||
ServerVersion *version.Info `json:"server_version"` | ||
} | ||
|
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 created a data type instead of the ad-hoc map that was previously returned by this data gatherer,
to make it easier to do a type conversion in ConverDatareadingsToCyberarkSnapshot function.
@@ -307,14 +307,17 @@ func (g *DataGathererDynamic) WaitForCacheSync(ctx context.Context) error { | |||
return nil | |||
} | |||
|
|||
type DynamicData struct { | |||
Items []*api.GatheredResource `json:"items"` | |||
} |
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 created a data type instead of the ad-hoc map that was previously returned by this data gatherer,
to make it easier to do a type conversion in ConverDatareadingsToCyberarkSnapshot function.
@@ -16,6 +16,9 @@ var SecretSelectedFields = []FieldPath{ | |||
{"metadata", "ownerReferences"}, | |||
{"metadata", "selfLink"}, | |||
{"metadata", "uid"}, | |||
{"metadata", "creationTimestamp"}, | |||
{"metadata", "deletionTimestamp"}, | |||
{"metadata", "resourceVersion"}, |
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.
The Cyberark backend needs this extra metadata to produce its reports.
I think it makes sense to upload this metadata for TLSPK too.
I can't see any harm in it and it will be difficult to implement different field filters for the TLSPK vs CyberArk uploaded data.
@@ -41,10 +42,7 @@ func (mds *mockDataUploadServer) ServeHTTP(w http.ResponseWriter, r *http.Reques | |||
mds.handlePresignedUpload(w, r) | |||
return | |||
case "/presigned-upload": | |||
mds.handleUpload(w, r, false) | |||
return | |||
case "/presigned-upload-invalid-json": |
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 mock endpoint was unused, and was passing exactly the same arguments to handleUpload.
Probably a legacy of a previous version of these tests.
So I deleted it.
_, err := w.Write([]byte(`{"message":"method not allowed"}`)) | ||
if err != nil { | ||
panic(err) | ||
} |
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 added some extra error checking here and below. These errors should normally be nil
w.WriteHeader(http.StatusOK) | ||
w.Header().Set("Content-Type", "application/json") | ||
_, _ = w.Write([]byte(`{"url":`)) // invalid JSON | ||
return |
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 branch wasn't used.
The AWS presigned URL endpoint only returns JSON for errors. Not for StatusOK.
w.WriteHeader(http.StatusOK) | ||
w.Header().Set("Content-Type", "application/json") | ||
_, _ = w.Write([]byte(`{"success":true}`)) |
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.
The AWS endpoint returns an empty response body on success.
d.DisallowUnknownFields() | ||
if err := d.Decode(&snapshot); err != nil { | ||
panic(err) | ||
} |
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.
Verifies that the new Snapshot format is used in the request body.
Fixes: https://venafi.atlassian.net/browse/VC-43753
The client still deviates from the API requirements, as follows:
...but I prefer to address those in followup PRs, as time permits.
Testing
I ran the Cyberark dataupload client Integration test manually (it's not yet automated):
This PR touches some code shared with the TLSPK agent uploads so I ran the e2e test script manually and verified that the test certificate was uploaded and processed by the TLSPK backend: