-
Notifications
You must be signed in to change notification settings - Fork 127
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
feat(admission-controller): Accept URL for custom region customers [SSPROD-32839] #1492
Conversation
…er url or apiEndpoint. Requiring a known region or apiEndpoint to be provided. Requiring .Values.webhook.v2.nats.url when url is used instead of apiEndpoint because we are not able to compute it. Preventing user to input apiEndpoint with either http:// or https:// as prefix.
…onController enabled and using sysdig.url instead of sysdig.apiEndpoint
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.
PR title does not comply with regex: ^(\w*)(?:\(([\w\$\.\,\-\*\s]*)\))?\:\s?(.*)$
!
Check PR guidelines at https://github.com/sysdiglabs/charts/blob/master/README.md#pull-requests
{{- required "A valid Sysdig API endpoint (.sysdig.apiEndpoint) is required" .Values.sysdig.apiEndpoint -}} | ||
{{- else if hasKey ((include "sysdig.regions" .) | fromYaml) .Values.global.sysdig.region }} | ||
{{- include "sysdig.secureApiEndpoint" . }} | ||
{{ include "chart.validateSysdigApiEndpoint" . }} |
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.
Shouldn't this include
be inside the first if?
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 being validated for the entire define because, in the else scenario, it also should be validated, so instead of validating inside both ifs I'm validating before them.
{{- $apiEndpoint := coalesce .Values.sysdig.url .Values.sysdig.apiEndpoint -}} | ||
{{- required "A valid Sysdig API endpoint (.apiEndpoint or .url) is required for custom region" $apiEndpoint -}} | ||
{{- else }} | ||
{{- if .Values.sysdig.apiEndpoint -}} |
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.
Since .Values.sysdig.apiEndpoint
should be used only if custom
region has been chosen, i'm not sure why we are still checking it here 🤔
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 don't know if I properly understood your comment, but apiEndpoint isn't only used if custom region has been chosen.
If custom region has been chosen we require either url or apiEndpoint, but for non-custom regions we still provide the option to receive apiEndpoint, then in the last case we fallback to default endpoint from _regions.tpl.
@AlbertoBarba I'm moving our thread to here so we can take a look at this in the future if needed :) So, with this code, we achieve the following:
If the region is custom, allow customers to specify either url or apiEndpoint -> if region is custom, previous behavior only allows customers to specify apiEndpoint, not url. cc: @airadier |
Looks good to me. We are keeping the existing behavior, adding additional checks, and solving the problem reported in SSPROD-32839 that wouldn't allow using "url" with "custom" region. |
@IgorEulalio @airadier My only concern is the fact that customers should now provide a region by design. |
@AlbertoBarba isn't this the current and expected behavior?
|
@airadier AFAIK when we introduces the common region resolution the flow became:
While in this PR we'll add the "Set a valid region, but be able to override the endpoint anyway". I'll wait for @aroberts87 opinion since he was the one who introduced the common region resolution |
If that's the case, then I agree, let's align the behavior with the rest of the components. So if I understand correctly the main difference is that |
This isn't something that is being introduced in this PR, the only change regarding the entire region definition workflow is that if we define custom region, we expect either url or apiEndpoint, whereas in the old behavior we were expecting only apiEndpoint. The ability to override the apiEndpoint without using custom region is already there: I can see that agent uses the exactly same mechanism to define collectorEndpoint: |
Hey all, sorry for the delay. Just returning from holiday. To address the intent/priority of the For my own sake, I guess I am still unclear about what this change is attempting to fix and why it's only been discovered now, as the region code introduction is quite old at this point. Am I correct in understanding that we sorta-kinda made a breaking change six months ago (#1118) where we migrated from |
I think so. Not really making it first-class citizen again, but that change, according to the ticket, breaks the backwards configuration when using Also @IgorEulalio introduced an additional check to make sure that apiEndpoint doesn't include the redundant https:// prefix, as it is already added by the template or the component if needed. |
TBH is also unclear for me how much impact this is bringing to customers, I definitely agree that this is engineering solution to save customer from a migration. Though, if we take a look at the change, we are not exactly making it available again, it is already there, customers can today specify only url and that will replace the SECURE_URL value in configmap:
So at the end of today we only deprecated it for scenarios where customers are using custom regions, besides removing it from the docs. |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
This PR has been closed due to inactivity. |
What this PR does / why we need it:
Checklist