Skip to content
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

feat(client): Allow agent to send cluster description #508

Conversation

tfadeyi
Copy link
Contributor

@tfadeyi tfadeyi commented Feb 5, 2024

Allows the user to configure a cluster description to be used during the cluster registration in the Venafi control plane.

Changes:

  • Add cluster_description to the agent configuration.
  • Update the client interface and concrete structs to support PostDataReadingsWithOptions.

@tfadeyi tfadeyi force-pushed the VC-30745-update-the-agent-to-send-the-cluster-info-name-description-on-upload branch from e827acf to fdcae84 Compare February 5, 2024 12:29
Allows the user to configure a cluster description to be used during the
cluster registration in the Venafi control plane.

Signed-off-by: Oluwole Fadeyi <oluwole.fadeyi@venafi.com>
@tfadeyi tfadeyi force-pushed the VC-30745-update-the-agent-to-send-the-cluster-info-name-description-on-upload branch from fdcae84 to 78a1d3b Compare February 5, 2024 15:49
@tfadeyi tfadeyi merged commit 5067343 into master Feb 5, 2024
8 checks passed
@tfadeyi tfadeyi deleted the VC-30745-update-the-agent-to-send-the-cluster-info-name-description-on-upload branch February 5, 2024 16:09
Comment on lines +182 to +184
if opts.ClusterName != "" {
query.Add("name", stripHTML.Sanitize(opts.ClusterName))
}
Copy link
Member

@maelvls maelvls Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Passing by) Just for future reference, I was wondering this: why do we pass the query param name set to the config's cluster_id? The cluster_id is already present in the request body of /v1/tlspk/upload/clusterdata/:uploaderId, more specifically under agent_metadata. For example:

POST https://api.venafi.cloud/v1/tlspk/upload/clusterdata/no?name=mvalais+Kind+Cluster HTTP/2.0

content-type: application/json
authorization: Bearer ...
content-length: 536
accept-encoding: gzip
user-agent: Go-http-client/2.0
{
  "agent_metadata": {
    "version": "development",
    "cluster_id": "mvalais Kind Cluster"
  },
  "data_gather_time": "2024-07-26T13:03:39.831129Z",
  "data_readings": [
    {
      "cluster_id": "mvalais Kind Cluster",
      "data-gatherer": "k8s-discovery",
      "timestamp": "2024-07-26T15:03:09+02:00",
      "data": {
        "server_version": {
          "major": "1",
          "minor": "29",
          "gitVersion": "v1.29.2",
          "gitCommit": "4b8e819355d791d96b7e9d9efe4cbafae2311c88",
          "gitTreeState": "clean",
          "buildDate": "2024-02-14T22:25:42Z",
          "goVersion": "go1.21.7",
          "compiler": "gc",
          "platform": "linux/arm64"
        }
      },
      "schema_version": "v2.0.0"
    }
  ]
}

When I don't pass the query param name=mvalais+Kind+Cluster, I get the following error:

[{"message":"could not find a Kubernetes cluster linked to the client in the Venafi Control Plane"}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The payload body could be quite large, several MB depending on the cluster where the agent is installed, in the backend entrypoint we wanted to be able to get this information without having to parse the entire upload before the actual processing pipeline kicks in. (Note: we could stream the upload JSON but we thought query params would work well instead).

The error can be improved, but essentially you have 2 options to onboard a cluster:

  • venctl installed which creates a service account and cluster_connection (relation of cluster and service account) in VCP.
  • Helm installation which requires the backend entrypoint to create a cluster_connection in VCP for your cluster and service account.

The error is saying that the cluster and service account are not linked in VCP.

In the venafi docs we tell users following the helm installation that they are required to set the cluster name, which will also be present in the UI. We could introduce an auto-onboard field to the config but since the cluster name field is required for the helm installation we thought we could use it rather than introducing another field.

If the cluster name is set in the config it will be used in the query param. The user wouldn't interact with the query params.

Copy link
Member

@maelvls maelvls Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't think of the body lenght of the request, and since the server doesn't parse JSON as a stream, it makes sense to use a query parameter. Super useful insight, thanks!

Comment on lines +17 to +18
ClusterID string
ClusterName string
Copy link
Member

@maelvls maelvls Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between ClusterName and ClusterID? It seems like ClusterName is set to ClusterID in run.go:

err := preflightClient.PostDataReadingsWithOptions(readings, client.Options{
	ClusterName:        config.ClusterID,
	ClusterDescription: config.ClusterDescription,
})

and ClusterID isn't set? I would have expected something like:

err := preflightClient.PostDataReadingsWithOptions(readings, client.Options{
	ClusterName:        config.ClusterID,
	ClusterID:          config.ClusterID,
	ClusterDescription: config.ClusterDescription,
})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking up earlier in the file run.go:

// orgID and clusterID are not required for Venafi Cloud auth

Sounds like we could comment the Options struct like this:

	Options struct {
		OrgID              string // Only used with platform.jetstack.io
		ClusterID          string // Only used with platform.jetstack.io
		ClusterName        string // Only used with Venafi Cloud
		ClusterDescription string // Only used with Venafi Cloud
	}

Comment on lines +181 to +184
stripHTML := bluemonday.StrictPolicy()
if opts.ClusterName != "" {
query.Add("name", stripHTML.Sanitize(opts.ClusterName))
}
Copy link
Member

@maelvls maelvls Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Passing by) Why was stripHTML.Sanitize needed here? Isn't overkill in this context? query.Encode already URL-encodes values, and this value isn't meant to be interpreted by any HTML parsers

I'd expect this type of sanitization to happen on the server side, since users may use the API directly (without us knowing) and they may inject things in the cluster name, such as HTML tags.

Comment on lines +185 to +187
if opts.ClusterDescription != "" {
query.Add("description", base64.RawURLEncoding.EncodeToString([]byte(stripHTML.Sanitize(opts.ClusterDescription))))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why base 64 encode this? Query parameters are already URL-encoded by query.Encode, which should be enough to pass that data to the server.

Same question as above regarding stripHTML.Sanitize too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants