feat: support data transfer protection in HDFS artifacts. Fixes #13482

[Looka149]

Fixes #13482

Motivation

As shown in https://github.com/argoproj/argo-workflows/blob/main/examples/hdfs-artifact.yaml, I am working on storing artifacts in HDFS. In my environment, the HDFS cluster has dfs.data.transfer.protection set to authentication. When I run the example, I encounter a read: connection reset by peer error.

Modifications

The HDFS client supports the data transfer protection option, which can be configured to authentication, integrity, privacy, or left empty, as detailed in this pull request. With this configuration, the issue described above is resolved.

This feature is available in versions v2.4.0, v2.3.0, v2.2.1, and v2.2.0 (see commit). Since we are using version v2.4.0 of the HDFS client (as specified here), we can utilize this feature.

Verification

Referring to the hdfs-artifact.yaml example, you can specify the value of artifacts.hdfs.dataTransferProtection as 'authentication', 'integrity', 'privacy', or leave it empty.

  - name: print-message
    inputs:
      artifacts:
      - name: message
        path: /tmp/message
        hdfs:
          dataTransferProtection: authentication

If an incorrect value is provided, the following error will occur.

$ kubectl get workflow
NAME                  STATUS   AGE     MESSAGE
hdfs-artifact-td82h   Failed   33m     invalid spec: templates.artifact-example.steps[1].consume-artifact templates.print-message.inputs.artifacts.message.hdfs.dataTransferProtection must be one of `authentication`, `integrity`, `privacy`, or empty

[Looka149]

@terrytangyuan @sarabala1979
Hi, is there anything else needed to proceed with the approval? Thanks.

[juliev0]

Thanks for adding the validation here, although I'm torn on whether we should validate this or not. On one hand, if the ClientOptions ever change for DataTransferProtection then we'd have to update this when the version in go.mod gets updated. I'm wondering if it may be easy for someone to update the go.mod and then forget to check this part - what do you think?

[Looka149]

I agree that your concern is valid. I initially added validation based on the three modes specified in https://github.com/colinmarc/hdfs/blob/master/client.go#L24-L28. While I don't expect these options to change frequently, I agree that if they do change in the future, this validation might be easily overlooked.

[juliev0]

@Looka149 Your PR looks good. I can merge it. Only thing I'm thinking is that it may be better not to validate on these specific strings which could theoretically change, per my comment, but I may be convinced otherwise. Thanks.

[Looka149]

@juliev0 Thank you for reviewing my PR, I agreed with your feedback and have removed the validation. Thank you.

[juliev0]

/retest

[juliev0]

@Looka149 assuming the CI failures aren't related to your change, would you mind adding empty commits until the CI passes?

[Looka149]

@juliev0 Hi. I have merged the main branch, and the CI tests have completed successfully. Thanks.

[agilgur5]

To make artifacts more sustainable (other than plugins #5862), I've been arguing we should perhaps accept arbitrary YAML string output more often. E.g. in this case:

clientOptions: |
  dataTransferProtection: true

That way we don't have to add a new feature every time a user wants to configure something else

Although it doesn't apply in every case, to be fair