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

Fix snapshot/diagnostic url parsing #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Alice-Lilith
Copy link

@Alice-Lilith Alice-Lilith commented Aug 16, 2023

Description

The current version of the agent spits out tons of messages about not being able to get the diagnostics. The problem is that when you have a Kubernetes Deployment with:

env:
- name: AES_DIAGNOSTICS_URL
          value: http://edge-stack-admin.ambassador:8877/ambassador/v0/diag/?json=true

Then after you apply it, Kubernetes automatically escapes ? and = with \ characters. This prevents the current library from successfully parsing the environment variables. This updates the function that parses the environment variable struct to override these two URL member fields with manual parsing that removes any \ characters first. There are likely other public libraries that would handle this edge case correctly, and a longer-term goal I have is to remove the envonfig package from Emissary everywhere since it is incredibly over-complicated. Still, for now, this will ensure that these URLs can be correctly parsed regardless of whether they have escaped special characters or not. This was technically only impacting the diagnostics URl, but I added the same logic for the snapshots URL just in case it ever changes before we get around to removing envconfig (unlikely but possible).

Example error prior to this fix:

Error getting diagnostics from ambassador Cannot fetch diagnostics from url: http://edge-stack-admin.ambassador:8877/ambassador/v0/diag/%5C?json\\=true

Fill ALL sections

Documentation

  • I updated the relevant .md documentation.
  • I updated the relevant website documentation.
  • This PR does not require documentation updates.

Dependencies

  • This PR contains new dependencies
  • This PR does not have any change in dependencies.

Testing

  • I provided sufficient test coverage for the modified code.
  • The existing tests capture the requirements for the modified code.
  • The bulk of my code covered by unit tests.
  • I executed manual tests to validate my changes and to avoid regressions.
Testing Strategy

A few words describing your testing strategy, e.g., manual tests, automated tests, extra validations, etc.

Dependencies, related issues, and other notes

  • List change dependencies, if any.
  • List related issues, if any.

@Alice-Lilith Alice-Lilith marked this pull request as ready for review August 17, 2023 15:25
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.

1 participant