-
Notifications
You must be signed in to change notification settings - Fork 37
Adding top level useDualstackEndpoint configuration #254
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
Conversation
| */}} | ||
| {{- define "fluent-bit.add-dualstack-endpoints" -}} | ||
| {{- $config := .config -}} | ||
| {{- if and .Values.useDualstackEndpoint (not (contains "endpoint" $config)) -}} |
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 is to make sure we do not replace endpoint if it already exist in the config (for example for adc regions). ADC is out of scope for this change.
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.
do we have a test that validates this logic?
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 to pr description, endpoint is not overwritten if it already exists in [OUTPUT] section of fluent bit configuration. So adc region endpoint override would be respected.
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.
it would be helpful to add before/after FB config with ipv6 enabled similar to how you show agent config in the description
| {{- $agent := set $configCopy "agent" $agentRegion }} | ||
| {{- end }} | ||
|
|
||
| {{- if .Values.useDualstackEndpoint }} |
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.
We're adding the agent section if does not exist and then adding "use_dualstack_endpoint" to true in that config.
| k8s-app: fluent-bit | ||
| data: | ||
| fluent-bit.conf: | | ||
| {{- if .Values.useDualstackEndpoint }} |
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.
We'll set dns prefer ipv6 to true even if environment is ipv4 it should still works as it will fallback to ipv4 as shown by this passing test:
https://github.com/aws/amazon-cloudwatch-agent/actions/runs/19839229481/job/56845933521
Ran test by enabling dualstack endpoint for ipv4 and ipv6 and they both pass.
the-mann
left a comment
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.
please add manual test to description that tests the case where the endpoint is manually set and dualstackendpoint is enabled. the test should validate that the custom endpoint that is set is used instead of the dual stack endpoint. #254 (comment)
|
It would be worth adding some test scenarios in https://github.com/aws-observability/helm-charts/tree/main/integration-tests/amazon-cloudwatch-observability/terraform/minikube/scenarios for the logic in here imo |
579fc89 to
811fdac
Compare
| {{- define "fluent-bit.add-dualstack-endpoints" -}} | ||
| {{- $config := .config -}} | ||
| {{- if and .Values.useDualstackEndpoint (not (contains "endpoint" $config)) -}} | ||
| {{- $config = replace "region ${AWS_REGION}" (printf "region ${AWS_REGION}\n endpoint logs.${AWS_REGION}.api.aws\n sts_endpoint sts.${AWS_REGION}.api.aws") $config -}} |
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.
Hmm, this seems like it could break at any point depending on the spacing. I think we could use a regexReplaceAll or something
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.
yup that makes sense, using regex now instead.
7fe19c1 to
fe58e20
Compare
fe58e20 to
8d305bb
Compare
| */}} | ||
| {{- define "fluent-bit.add-dualstack-endpoints" -}} | ||
| {{- $config := .config -}} | ||
| {{- if and .Values.useDualstackEndpoint (not (contains "endpoint" $config)) -}} |
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.
it would be helpful to add before/after FB config with ipv6 enabled similar to how you show agent config in the description
| {{- range $key, $val := .Values.containerLogs.fluentBit.configWindows.extraFiles }} | ||
| @INCLUDE {{ $key }} | ||
| {{- end }} | ||
| parsers.conf: | |
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.
has this been also tested on an windows instance?
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.
removing this altogether as windows nodes don't support ipv6 yet so there is no need for dualstack endpoints: https://docs.aws.amazon.com/eks/latest/userguide/windows-support.html
| {{- define "fluent-bit.add-dualstack-endpoints" -}} | ||
| {{- $config := .config -}} | ||
| {{- if and .Values.useDualstackEndpoint (not (contains "endpoint" $config)) -}} | ||
| {{- $config = mustRegexReplaceAll "(region\\s+\\$\\{AWS_REGION\\})" $config "$1\n endpoint logs.$${AWS_REGION}.api.aws\n sts_endpoint sts.$${AWS_REGION}.api.aws" -}} |
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 think custom endpoint logic also applies to sts_endpoint? if so, please add or update minukube test case to include sts_endpoint
| {{- end }} | ||
| parsers.conf: | | ||
| {{- .Values.containerLogs.fluentBit.config.customParsers | nindent 4 }} | ||
| {{- if hasPrefix "us-iso-" .Values.region }} |
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.
not an expert with FB config but what happens if a custom fluentbit config doesn't have Parsers_File for some reasons? would it be better to just appned right after [SERVICE]?
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.
So custom fluent bit configs are out of scope as that's what we also putting in public docs, but it does make sense to append right after [SERVICE], will change it to this instead.
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.
what do you mean custom config is out of scope? the public doc should have some instructions on how to setup ipv6 with custom configs, right? The new function fluent-bit.add-ipv6-preference will skip if a custom config already has dual stack configred.
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.
Yup we are providing customers using custom configuration of a way to update the fluent bit configuration, meant to say that if customer are using customer config they would need to update their fluent bit config manually. This top level field is only for customer using default config.
...n-cloudwatch-observability/validations/minikube/scenarios/dualstack_endpoint_enabled_test.go
Show resolved
Hide resolved
9456e18 to
c152d57
Compare
c152d57 to
96a6fcb
Compare
Issue #, if available
Currently customers can not use our default configuration to have agent and fluent bit use dualstack endpoints. We want to add a top level field to allow using dualstack endpoints.
Passing tests: https://github.com/aws/amazon-cloudwatch-agent/actions/runs/19836119811
Description of changes:
If customer sets the new top level
useDualstackEndpointfield to true, we'll adduse_dualstack_endpointto true in the agent configuration so that it uses dualstack endpoints(this is the current way to enable cloudwatch agent to use dualstack endpoints.)As for fluent bit, we'll update
endpoints/sts_endpointfor fluent bit configuration (under[OUTPUT]section) to use dualstack endpoints and then have fluent bit prefer ipv6 dns using net.dns.prefer_ipv6 true. (Note settinguseDualstackEndpointtotrueon an ipv4 instance would NOT break the cluster as dualstack endpoints also work on ipv4 instances and net dns prefer ipv6 does fall back to ipv4. (Applied helm on ipv4 instance and setuseDualstackEndpoint, and it works.Also we do aren't replacing endpoints if there is already an endpoint field (for example adc regions). ADC regions are out of scope of this change.
Testing
All testing was done on IPv6 EKS cluster, we also tested that dualstack endpoints work on K8s so we can make this helm chart change.
Below are logs from fluent bit and the agent configuration showcasing what happens if we set the top level field
useDualstackEndpointto true or false.Setting
useDualstackEndpointto trueHelm upgrade command:
Resulting agent configuration
As you can setting useDualstackEndpoint field to
truesets agent configuration to use dualstack endpoint.Resulting fluent bit config
Resulting fluent bit logs
As for Fluent bit, here are some log output showcasing successfully sending logs using dualstack endpoints:
Setting
useDualstackEndpointto falseHelm upgrade command:
Resulting agent configuration
As you can see seeting
useDualstackEndpointtofalsedoes not set theuse_dualstack_endpointto true nor adds it to the agent configuration.Resulting fluent bit config
Resulting fluent bit logs
As for Fluent bit, here are some log output showcasing successfully sending logs using regular ipv4 endpoints.
Test to ensure we don't override endpoint if it is already given
In the fluent bit configuration I added endpoint
logs.us-east-1.api.awsto see if the endpoint is overwritten withuseDualstackEndpointis true and it isn't overridden and the fluent bit configuration does take precedence making sure that we don't override adc region endpoints.Added scenario tests
Added minikube-based integration tests to validate the useDualstackEndpoint configuration:
What each test validates:
default_test.go: Confirms FluentBit configs don't contain dualstack endpoints or IPv6 preference when disableddualstack_endpoint_enabled_test.go: Confirms FluentBitOUTPUTsections have endpoint:logs.${AWS_REGION}.api.awsand sts_endpoint:sts.${AWS_REGION}.api.aws,SERVICEsection hasnet.dns.prefer_ipv6true, and CloudWatch Agent hasuse_dualstack_endpoint: truedualstack_endpoint_with_custom_endpoint_test.go: Confirms custom endpoints inapplication-log.confare NOT overwritten, while other configs still get dualstack endpoints