Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Adds SSO control via individual namespaces. Fixes #6916 #6990
feat: Adds SSO control via individual namespaces. Fixes #6916 #6990
Changes from all commits
7536b52
dd1ef75
72fc702
2344eb8
3bf16d3
0af8f03
c3e4d65
bb1262c
125cd19
8982818
2453b33
d9953d8
458578c
1241358
0a77201
eefb63b
dedeca6
d0b0675
040ca2f
1c256b7
fbbaced
0da0fe0
cbc135d
58e2142
2c2f801
26d777a
24c9594
ae0625f
dc583f2
6431071
e47c142
7c8ad7c
d8ae5bb
dcc1992
04be874
c51e0a8
90ea1b7
0d870cf
e1eeba0
ec40b61
7a40f16
51fd13e
74b95ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 you always want to use opts.managedNamespace?
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 thought about it.
Case 1
NamespaceAll
for thisCase 2
argo
)opt.Namespace
for thisCase 3
argo
namespace for installation, anduser-namespace
for usageargo
namespaceopt.Namespace
for thisSince for case 2 and 3, we only need service accounts of a single namespace,
we will just use the installation namespace which is the
namespace
opt, and notmanagedNamespace
optThere 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.
Case 1:
cacheNamespace == managedNamespace == "" == NamespaceAll
namespace == "argo"
Case 2:
cacheNamespace == managedNamespace == namespace == "argo"
Case 3:
managedNamespace == userNamespace
namespace == "argo"
cacheNamespace == ???
So it is case 3 that is odd. It should be whatever it is today, which I think is "argo", which seems wrong to me. I would actually think you'd want the user namespace. But history is history.
Lets make this overridable with config, so if we are wrong, then we can change out mind.
Maybe call it
ssoNamespace
?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.
ok, we can use
ssoNamespace
so for case 1 we will set
ssoNamespace
= NamespaceAllFor Case 2 and 3 we will set
ssoNamespace
= namespace (typically argo)Will make these changes
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.
Seems like it would be better to do this with a serializer/deserlisizer that returns Go structs. This code is smelly/hacky
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.
True that.
I just didn't change it since it existed from before.
I can try to change it in a follow up PR
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.
Same 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.
Yup, Just reused the code which existed from before
Can change it in a follow up PR
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 like this clearer error