-
Notifications
You must be signed in to change notification settings - Fork 430
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
Update allowedNamespaces type in AzureClusterIdentity #1331
Update allowedNamespaces type in AzureClusterIdentity #1331
Conversation
// Namespaces can be selected either using an array of namespaces or with label selector. | ||
// An empty allowedNamespaces object indicates that AzureClusters can use this identity from any namespace. | ||
// If this object is nil, no namespaces will be allowed (default behaviour, if this field is not provided) | ||
// A namespace should be either in the NamespaceList or match with Selector to use the identity. |
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.
If both namespaceList and selectors are specified, which one takes precedence?
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 see that namespaceList takes precedence. Maybe worth pointing this out somewhere?
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.
good point, will add in desc here and docs
return err | ||
} | ||
|
||
if len(src.Spec.AllowedNamespaces) > 0 { |
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 conversion be from the restored spec?
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.
restored is the v1alpha4 type
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.
IIUC, we must restore the extra fields (selector in this case) that will get lost in down conversion here, right?
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.
you mean in an annotation? doesn't it get saved automatically?
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.
yeah from the data annotation, something like this:
dst.Spec.NetworkSpec.PrivateDNSZoneName = restored.Spec.NetworkSpec.PrivateDNSZoneName |
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.
oh i get it now, my bad, just fixed it
6029f04
to
b3b822f
Compare
@shysank please take another look |
lgtm with one clarification on the conversion logic. |
b3b822f
to
0c29ef4
Compare
@@ -161,3 +163,45 @@ func getAzureIdentityType(identity *infrav1.AzureClusterIdentity) (aadpodv1.Iden | |||
return 0, errors.New("AzureIdentity does not have a vaild type") | |||
|
|||
} | |||
|
|||
// IsClusterNamespaceAllowed indicates if the cluster namespace is allowed | |||
func IsClusterNamespaceAllowed(ctx context.Context, k8sClient client.Client, allowedNamespaces *infrav1.AllowedNamespaces, namespace string) bool { |
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.
@devigned can you take a look at this and see if it makes sense based on all the previous conversations?
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.
/lgtm
@CecileRobertMichon or @shysank any other feedback?
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devigned The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
0c29ef4
to
24c352a
Compare
did a rebase because the check jobs were stuck, will need a new lgtm :) |
@nader-ziada: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/test pull-cluster-api-provider-azure-e2e |
/lgtm |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #1288
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs:
Release note: