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

queryservice: add allowlist configuration #165

Merged
merged 4 commits into from
Oct 24, 2024
Merged

Conversation

AndrewKostka
Copy link
Contributor

@AndrewKostka AndrewKostka commented Oct 15, 2024

Bug: T375384

@@ -30,6 +30,10 @@ spec:
volumeMounts:
Copy link
Contributor

Choose a reason for hiding this comment

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

is the volume mount of allowlist-static missing here?

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 query service doesn't directly use allowlist-static.txt. In the past, we manually created allowlist.txt by combining the endpoints in allowlist-static.txt with our Wikibase Cloud endpoints. It's still in the ConfigMap because this process will now be automated by the Platform API.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, if I understand correctly:

  • the static list is set here
  • this static list is added to the dynamic list generated by the api and stored in the main allowlist

@@ -2,7 +2,7 @@ apiVersion: v2
name: argocd-config
description: Chart to deploy ArgoCD configuration (including the argocd-apps chart)
type: application
version: 1.0.5
version: 1.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this was inadvertently bumped?

@tarrow
Copy link
Contributor

tarrow commented Oct 22, 2024

Approve but assuming you'll fix this argcd-config oversight (can't even merge without doing so :) )

name: {{ include "wdqs.fullname" . }}-allowlist
data:
allowlist-static.txt:
{{- .Values.allowListStatic | toYaml | indent 2 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't have been surprised if you'd either hardcoded the list here but I think having the list in the deploy repo is way cleaner for when we need to update it.

I could also have seen the static list in it's own configmap separate configmap rather than a key of this map. No need to do this though.

This reverts commit 8808044.
@AndrewKostka AndrewKostka added this pull request to the merge queue Oct 24, 2024
Merged via the queue into main with commit b265609 Oct 24, 2024
2 checks passed
@AndrewKostka AndrewKostka deleted the queryservice-allowlist branch October 24, 2024 10:53
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