LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd #1397

cahartma · 2022-03-17T19:45:43Z

Description

For STS enabled clusters we needed to modify a few things in the collector pod and fluentd config, in order to change from long-lived credentials to using sts role and token. This required:

read the secret and check for "sts format" (includes a 'credentials' key specifying a valid 'role_arn' and 'web_identity_token_file')
mount a projected SA token to the pod at the specified path in the secret (or use default path if none specified in secret)
modify the fluentd config (configmap/collector) to add a section for <web_identity_credentials> containing role and token path specified in the secret

Links

https://issues.redhat.com/browse/LOG-1977
subtask comments containing more details: https://issues.redhat.com/browse/LOG-1979
dependent on updated fluentd cloudwatch plugin: https://issues.redhat.com/browse/LOG-2279

jcantrill · 2022-03-21T13:53:14Z

internal/generator/fluentd/output/cloudwatch/cloudwatch.go

+		return AWSKey{
+			KeyRoleArn:          ParseRoleArn(secret),
+			KeyRoleSessionName:  constants.AWSRoleSessionName,
+			KeyWebIdentityToken: mountPath + "/" + filePath,


you can use path.Join

Is there any reason we can not rely on the podspec volume/volumemount path abstraction? The path to the token file is from the container's perspective. We can mount it anywhere meaning we can make it a fixed location. Doing so means we just need to map the volume and volume mount correctly when setting up the podspec.

The user specifies the path for the token. A refactor was done after speaking with the ROSA team. This is so that our add-on functions the same as all other operators, when working in sts mode. More details are in the ticket.

Upon further testing and attempts at accomplishing a fixed location, we must keep the functionality as is. We are required to create a mount in the container, at the path provided in the secret. The pods' projected SA volume specifies a relative file path to this container mount. Therefor we must utilize both the mount and the pod volume spec to build the specified path.

After our long discussion yesterday, (including Victor K as well) the logic has been reverted and we are back to using our opinionated path:
KeyWebIdentityToken: path.Join(constants.AWSWebIdentityTokenMount, constants.AWSWebIdentityTokenFilePath),

jcantrill · 2022-03-21T13:57:57Z

internal/generator/fluentd/output/cloudwatch/cloudwatch.go

+func ParseRoleArn(secret *corev1.Secret) string {
+	credentials := security.GetFromSecret(secret, constants.AWSCredentialsKey)
+	if credentials != "" {
+		roleIndex := strings.Index(credentials, "arn:aws:iam::")


We should consider replacing with a regex

jcantrill · 2022-03-21T13:59:01Z

internal/generator/fluentd/output/cloudwatch/cloudwatch.go

+func ParseIdentityToken(secret *corev1.Secret) (mountPath, filePath string) {
+	credentials := security.GetFromSecret(secret, constants.AWSCredentialsKey)
+	if credentials != "" {
+		split := strings.Split(credentials, "web_identity_token_file = ")


we shoudl consider replacing with a regex

function has been removed and we are back to using our opinionated (constant) path

internal/generator/fluentd/output/cloudwatch/cloudwatch.go

jcantrill · 2022-03-21T14:02:59Z

internal/generator/fluentd/output/cloudwatch/cloudwatch.go

+}
+
+// AppendVolumeActions Add mount and volumes based on attributes from the secret
+func AppendVolumeActions(secret *corev1.Secret) (mount corev1.VolumeMount, volume corev1.Volume) {


Consider adding an isolated unit test for this helper function

Further review compared to the pod setup. This should be moved to maybe some path under k8shandler since its not generator code

There have been several discussions and I am open to suggestions as to where this should live. Please explain, is this not part of the generation of the cloudwatch configuration?

After our long discussion yesterday, (including Victor K as well) the logic has been refactored again and this func removed.

jcantrill · 2022-03-21T14:14:12Z

internal/k8shandler/fluentd.go

@@ -182,6 +183,12 @@ func newFluentdPodSpec(cluster *logging.ClusterLogging, trustedCABundleCM *v1.Co
 		fluentdContainer.VolumeMounts = append(fluentdContainer.VolumeMounts, v1.VolumeMount{Name: name, ReadOnly: true, MountPath: path})
 	}

+	// Append any additional volumes based on attributes of the secret and forwarder spec
+	appendMounts, appendVolumes := GetAppendVolumes(secrets, &clfspec)


I'm actually in the process of reworking this section but until that lands, a more flexible pattern is to do something like:

func(*core.PodSpec, map[string]*Secret,*logging.ClusterLogForwarderSpec) {}

in newFluentdPodSpec pass an equivalent of GetAppendVolumes to update the podSpec

How close are you with your changes? I will leave mine as is for now. It may be easier for us to have me rebase onto your changes.

Much simpler now, so is on you :)

jcantrill · 2022-03-21T14:16:01Z

internal/generator/fluentd/outputs.go

+
+// GetAppendVolumes Append any additional volumes based output type and values from secret and forwarder spec
+// otherwise return empty
+func GetAppendVolumes(secrets map[string]*corev1.Secret, clfspec *logging.ClusterLogForwarderSpec) (mount corev1.VolumeMount, volume corev1.Volume) {


Same comment here. This is not generator code but podspec code. We should find a home for it there

removed based on discussion yesterday. The logic to append now lives within fluentd.go

cahartma · 2022-03-30T20:53:26Z

/retest

cahartma · 2022-03-31T14:15:46Z

/retest

jcantrill · 2022-03-31T14:49:32Z

/approve
/lgtm

openshift-ci · 2022-03-31T15:00:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cahartma, jcantrill

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-03-31T17:00:26Z

@cahartma: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

…-log-forwarding LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd

openshift-ci bot requested review from alanconway and pmoogi-redhat March 17, 2022 19:46

cahartma force-pushed the role-based-cloudwatch-log-forwarding branch 2 times, most recently from 1c2ecf9 to 978f63e Compare March 17, 2022 22:31

jcantrill requested changes Mar 21, 2022

View reviewed changes

cahartma force-pushed the role-based-cloudwatch-log-forwarding branch 3 times, most recently from 0539f8e to e933703 Compare March 25, 2022 20:12

cahartma requested a review from jcantrill March 28, 2022 15:39

cahartma force-pushed the role-based-cloudwatch-log-forwarding branch from e933703 to ab885ea Compare March 30, 2022 18:41

role based cloudwatch log forwarding

b516426

cahartma force-pushed the role-based-cloudwatch-log-forwarding branch from ab885ea to b516426 Compare March 30, 2022 18:52

openshift-ci bot assigned jcantrill Mar 31, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2022

openshift-merge-robot merged commit 5a72070 into openshift:master Mar 31, 2022

cahartma deleted the role-based-cloudwatch-log-forwarding branch April 5, 2022 21:40

pmoogi-redhat pushed a commit to pmoogi-redhat/cluster-logging-operator that referenced this pull request Apr 26, 2022

Merge pull request openshift#1397 from cahartma/role-based-cloudwatch…

acbda94

…-log-forwarding LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd #1397

LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd #1397

cahartma commented Mar 17, 2022

jcantrill Mar 21, 2022

jcantrill Mar 21, 2022

cahartma Mar 21, 2022

cahartma Mar 24, 2022

cahartma Mar 30, 2022

jcantrill Mar 21, 2022

cahartma Mar 24, 2022

jcantrill Mar 21, 2022

cahartma Mar 24, 2022

cahartma Mar 30, 2022

jcantrill Mar 21, 2022

jcantrill Mar 21, 2022

cahartma Mar 21, 2022

cahartma Mar 30, 2022

jcantrill Mar 21, 2022

cahartma Mar 23, 2022

cahartma Mar 30, 2022

jcantrill Mar 21, 2022

cahartma Mar 30, 2022

cahartma commented Mar 30, 2022

cahartma commented Mar 31, 2022

jcantrill commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd #1397

LOG-1977: Enable STS role support for cloudwatch forwarding via fluentd #1397

Conversation

cahartma commented Mar 17, 2022

Description

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahartma commented Mar 30, 2022

cahartma commented Mar 31, 2022

jcantrill commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022